diff --git a/.claude/skills/mechanical-refactor-verify/SKILL.md b/.claude/skills/mechanical-refactor-verify/SKILL.md index 98f9720d8..786339771 100644 --- a/.claude/skills/mechanical-refactor-verify/SKILL.md +++ b/.claude/skills/mechanical-refactor-verify/SKILL.md @@ -1,134 +1,50 @@ --- name: mechanical-refactor-verify -description: Verify mechanical refactoring commits by requiring a reproducible transform script (gist) in the PR description. Use when doing or reviewing file splits, function moves, or module extractions. -user_invocable: true -argument: "[verify ] — verify an existing PR, or omit to see the workflow guide" +description: Make mechanical refactoring (file splits, function moves, module extractions, renames) machine-checkable instead of eyeballed. Reproduce a relocation commit byte-for-byte from faithful primitives, and split an extraction into a verifiable prepare + move + postpare. Use when doing or reviewing such changes. --- -# Mechanical Refactor — Reproducible Verification +# Mechanical Refactor — Machine-Checkable Verification -## Core Principle +## 1. Overview -The deliverable of a mechanical move (file split, function move, module extraction) is NOT the diff — it is **the script that produces the diff**. -A script is auditable; a diff is not. +- The correctness of a mechanical change (file split, function move, module extraction, + rename) must be **machine-checkable, not eyeballed** — the proof is something anyone can + re-run, whoever made the change and whenever. +- **One property**: *a commit is a pure relocation*. **One proof**: **reproduce** — + regenerate the move from the base commit with faithful primitives, run the formatter, + byte-diff against the target. +- Empty diff = the proof. Any residual = a bundled non-move change, surfaced for review. +- A reshape must not ride along: split into optional **prepare** + certified **move** + + optional **postpare** (`guide-split.md`). -## Workflow +## 2. What do you want to do? -Regardless of who did the move (human or agent) and when (before or after committing), the workflow is the same: +- **Split a change into commits** (extract, move, file split) → `guide-split.md`: the + prepare + move + postpare rule, the case recipes, and the anti-patterns. +- **Construct the proof for a move commit** → `guide-construct-proof.md`: run + `scripts/mechanical_refactor_proof_generator.py`, or hand-write a `Repro` when the + generator reports `UNSUPPORTED`. +- **Verify someone's proof** → `guide-verify-proof.md`: re-run it, read the verdict, audit + the authored surfaces. +- **Decide whether a change counts as a clean move** → `spec-reproduction-utils.md`: the + property, the whole whitelist / not-allowed list, and each primitive's contract. The + source of truth for the reproduction module; if any other file disagrees, it wins. -### Step 1: Write the transform script to /tmp/ +## 3. Files -Write the script to `/tmp/transform_.py`. **Never write it inside the repo.** - -The scaffold (worktree creation, diff check, ruff format, result reporting) lives in `mechanical_refactor_verify_utils.py` next to this skill. - -**MANDATORY**: The transform script MUST use `verify_mechanical_refactor()` from the utils module. Do NOT reimplement the verification scaffold — no hand-written worktree management, no hand-written diff checking. The script only defines `transform()` and calls `verify_mechanical_refactor`. - -Script template (follow this structure exactly): - -```python -#!/usr/bin/env python3 -"""Reproducible transform for: - -Run from the repo root: python3 /tmp/transform_.py -""" -import sys -from pathlib import Path - -sys.path.append(".claude/skills/mechanical-refactor-verify") -from mechanical_refactor_verify_utils import verify_mechanical_refactor, exec_command, git_add_and_commit, dedent - -BASE_COMMIT = "" -TARGET_COMMIT = "" - - -def transform(dir_root: Path) -> None: - """Perform the mechanical transformation and commit each step. - - Args: - dir_root: Path to the worktree (checked out at BASE_COMMIT). - """ - # --- Step 1: Split source file --- - source = dir_root / "path/to/source.py" - content = source.read_text() - lines = content.splitlines(keepends=True) - - splits = [ - ("path/to/pkg/target_a.py", 1, 50), - ("path/to/pkg/target_b.py", 51, 120), - ] - for target_path, start, end in splits: - target = dir_root / target_path - target.parent.mkdir(parents=True, exist_ok=True) - target.write_text("".join(lines[start - 1 : end])) - - source.unlink() - (dir_root / "path/to/pkg/__init__.py").touch() - - git_add_and_commit("mechanical: split source.py", cwd=str(dir_root)) - - # --- Step 2: Fix imports --- - # - # git_add_and_commit("fix imports", cwd=str(dir_root)) - - # Note: pre-commit run --all-files is run automatically after transform() returns - - -if __name__ == "__main__": - verify_mechanical_refactor( - base_commit=BASE_COMMIT, - target_commit=TARGET_COMMIT, - transform=transform, - ) -``` - -### Step 2: Run the script from the repo root - -```bash -cd -python3 /tmp/transform_.py -# Expected: "PASS: transform reproduces the commit exactly." -``` - -If FAIL, fix the script and re-run until PASS. - -### Step 3: Upload gist, delete local file, update PR description - -One gist per PR. Do all three: - -```bash -# 1. Create gist (or update existing) -gh gist create --public -d "Mechanical refactor transform: " /tmp/transform_.py -# Or update: gh gist edit -a /tmp/transform_.py - -# 2. Delete local file -rm /tmp/transform_.py - -# 3. Update PR description (paste the block below) -``` - -PR description must include: - -````markdown -## Mechanical Move - -Transform script: - -### One-click verification - -```bash -python3 <(curl -sL ) -``` -```` - -### Step 4: PR scope - -A mechanical refactor PR must contain **only** mechanical changes (moves, splits, renames, import fixes, formatting). All of these must be reproducible by the transform script. - -Semantic changes (new logic, API restructuring, behavior changes) belong in a **separate PR**. - -## Verifying an existing PR (`/mechanical-refactor-verify verify`) - -1. Find the gist URL and one-click command in the PR description -2. Run the one-click command from the repo root -3. Report: PASS or show the diff +- [`guide-split.md`](guide-split.md) — split a change into prepare + move + postpare: the + case recipes, what stays mechanical, and the anti-patterns. +- [`guide-construct-proof.md`](guide-construct-proof.md) — produce the proof: the + generator, the hand-written `Repro`, and publishing the proof with the PR. +- [`guide-verify-proof.md`](guide-verify-proof.md) — consume the proof: re-run, verdicts, + and the audit checklist for authored surfaces. +- [`spec-reproduction-utils.md`](spec-reproduction-utils.md) — the normative spec of the + clean-move property and the reproduction primitives. +- [`scripts/mechanical_refactor_proof_generator.py`](scripts/mechanical_refactor_proof_generator.py) — + the **generator**: infers a reproduce recipe from a commit's diff and emits/runs a + standalone, auditable script per commit, with a `PASS` / `RESIDUAL` / `UNSUPPORTED` verdict. +- [`scripts/mechanical_refactor_reproduction_utils.py`](scripts/mechanical_refactor_reproduction_utils.py) — the + **proof engine**: the `Repro` builder's faithful relocation primitives plus the worktree + + pre-commit + byte-diff scaffold. Self-contained — only git and the standard library. +- [`scripts/tests/`](scripts/tests/) — pytest suites, one folder per module: + `reproduction_utils/` for the proof engine, `proof_generator/` for the generator. diff --git a/.claude/skills/mechanical-refactor-verify/guide-construct-proof.md b/.claude/skills/mechanical-refactor-verify/guide-construct-proof.md new file mode 100644 index 000000000..8c773fb8f --- /dev/null +++ b/.claude/skills/mechanical-refactor-verify/guide-construct-proof.md @@ -0,0 +1,157 @@ +# Construct a proof for a move commit + +## 1. What a proof is + +- A runnable script that regenerates the commit from its base with the faithful relocation + primitives and byte-diffs the result against it. +- The property and primitive contracts: `spec-reproduction-utils.md`. Splitting the change + so the move is provable: `guide-split.md`. Consuming the proof: `guide-verify-proof.md`. + +## 2. Auto-generate the script (primary path) + +- `mechanical_refactor_proof_generator.py` infers the recipe from a commit's diff and + before-state AST. +- It emits and runs a standalone, auditable script — no one hand-writes it. + +### 2.1 Commands + +```bash +# one commit: print the inferred script and run it (non-zero exit unless PASS) +python3 .claude/skills/mechanical-refactor-verify/scripts/mechanical_refactor_proof_generator.py + +# a range: write a self-contained folder +python3 .claude/skills/mechanical-refactor-verify/scripts/mechanical_refactor_proof_generator.py \ + .. --match -move: --out repro_out +``` + +### 2.2 The range product + +Self-contained, auditable without the skill installed: + +- `repro_scripts/.py` — one script per commit; +- `output.log` + `output.html` — the verdicts; +- a copy of `mechanical_refactor_reproduction_utils.py` — the scripts' only dependency. + +### 2.3 What the inference covers + +- **Method → existing class**: call sites lowered (`Owner.m(recv, …)` → `recv.m(…)`), the + orphaned local import removed. +- **Method → module-level free function**: call sites requalified (`Owner.m(…)` → `m(…)`). +- **Free function → existing module**: the call stays bare; callers repath their import + (`repath_import` when function-scoped; module-level repoints realised as remove-old + + add-new). +- **New-module extract of scattered defs**: `extract_symbols_to_new_module` under the + audited header; a constant that relocated into the header is dropped from the source. + A contiguous-tail source still uses `extract_to_new_module`. +- **A source file the commit deletes** once its defs relocated: `delete_file`. +- **The module-level import diff**, realised directly from the target: gained names added + (a wholly new module's statement verbatim, wrapping kept), lost names removed with + `remove_imported_name`. +- Non-Python files in the commit do not block inference; their diff is noted and left to + the residual. + +### 2.4 What it reports `UNSUPPORTED` + +- Single-commit mode prints the verdict with notes and exits non-zero; range mode + records it in the outputs. +- Review such a commit as prepare, or hand-write the `Repro` (§3). +- The cases: + - **no definition relocated** — a rename (even a privacy flip `_foo` → `foo`) or a + statement-level reorder; reshapes belong in prepare; + - **a new-module extract whose symbols are not all top-level in the source** — a + method still inside a class; prepare must de-self it out first; + - **an extract drawing from more than one source file**, and an **inline-block + extract-function** — compose `extract_function` by hand (the body must be unchanged; + a de-self / restructure is a separate semantic commit). + +## 3. Hand-write the `Repro` when inference falls short + +- Compose the transform from the same primitives (`spec-reproduction-utils.md` §3). +- The same byte-diff then certifies it. + +```python +import sys +from pathlib import Path + +sys.path.append(".claude/skills/mechanical-refactor-verify/scripts") +from mechanical_refactor_reproduction_utils import Repro + +r = Repro(base="", target="") +# Adapt call sites / repath imports BEFORE moving, so a call to a moved method from inside +# another moved method is lowered while still in the source and travels with the body. +r.lower_call_sites("update_weights_from_ipc", "ModelRunner", paths=["a.py", "b.py"]) +r.remove_import("a.py", "from x import ModelRunner", in_function="update_weights_from_ipc") +r.move_symbol("update_weights_from_ipc", src="a.py", dst="dst.py", into_class="WeightUpdater", dedent=0) +r.add_import("dst.py", "import gc") +r.run() # PASS = byte-identical; otherwise prints the residual +``` + +## 4. A hand-written transform for a non-relocation mechanical change + +- For a whole-file split or rename — no single symbol relocates — write a `transform()` + and call `verify_mechanical_refactor`. +- The scaffold (worktree, pre-commit, diff, reporting) lives in the skill's utils. + +```python +import sys +from pathlib import Path + +sys.path.append(".claude/skills/mechanical-refactor-verify/scripts") +from mechanical_refactor_reproduction_utils import verify_mechanical_refactor, git_add_and_commit + +BASE_COMMIT = "" +TARGET_COMMIT = "" + +def transform(dir_root: Path) -> None: + source = dir_root / "path/to/source.py" + lines = source.read_text().splitlines(keepends=True) + for target_path, start, end in [("path/to/a.py", 1, 50), ("path/to/b.py", 51, 120)]: + target = dir_root / target_path + target.parent.mkdir(parents=True, exist_ok=True) + target.write_text("".join(lines[start - 1 : end])) + source.unlink() + git_add_and_commit("split source.py", cwd=str(dir_root)) + # A rename is just: for each file, write content.replace(OLD, NEW); commit. + +if __name__ == "__main__": + verify_mechanical_refactor(BASE_COMMIT, TARGET_COMMIT, transform) +``` + +## 5. Publish the proof with the PR + +### 5.1 What to share + +- Share the scripts **plus** the copied `mechanical_refactor_reproduction_utils.py` — the + scripts import it, so a lone raw file is not runnable. +- Never a `python3 <(curl ...)` one-liner: process substitution gives the script no real + directory, so the import breaks. +- Flat layouts work: Python puts the script's own directory on `sys.path`, so the utils + module can sit either next to the script or one level up (the `--out` layout). + +### 5.2 Author: create a gist + +```bash +cd repro_out +gh gist create --desc "mechanical-move proof for PR #NNNN" \ + repro_scripts/*.py mechanical_refactor_reproduction_utils.py output.log +# prints https://gist.github.com// -- put it in the PR description +``` + +- `gh gist create` flattens paths — fine per §5.1. +- Alternatives: a PR attachment (zip the `--out` folder) or a branch holding it. + +### 5.3 Reviewer: download and re-run + +```bash +gh gist clone /tmp/proof # or: git clone https://gist.github.com/.git /tmp/proof +cd # the run resolves the repo from the cwd +python3 /tmp/proof/.py # PASS = byte-identical to this commit +``` + +- Include exactly these commands in the PR description under a + "Mechanical move — reproducible" heading. + +### 5.4 Keep the PR mechanical + +- A mechanical PR contains **only** mechanical changes (moves, splits, renames, import + fixes, formatting). Semantic changes go in a separate PR. diff --git a/.claude/skills/mechanical-refactor-verify/guide-split.md b/.claude/skills/mechanical-refactor-verify/guide-split.md new file mode 100644 index 000000000..3d60925be --- /dev/null +++ b/.claude/skills/mechanical-refactor-verify/guide-split.md @@ -0,0 +1,243 @@ +# Split a mechanical change: prepare, move, postpare + +## 1. Why split + +- A "move a method/function" change is really **two operations with different + correctness criteria**: + +| Operation | What it does | How you check it | +|---|---|---| +| **Semantic reshape** | method → free function or method; `self.X` → a parameter, or `self` retyped to the target class; signature / typing change | behavior unchanged: lint + tests pass | +| **Physical move** | cut from the source, paste into the target, fix imports | the moved body is byte-identical, line for line; the only other changes are move artifacts | + +- Put both in one commit and the criteria contaminate each other: + - one hunk then holds the reshape **and** an indentation shift **and** a cross-file + relocation; + - neither a human nor a tool can mechanically confirm "the body that landed is the + body that left" — you must re-read the logic. + +## 2. The rule — up to three commits, in this order + +- **prepare (optional)** — a **minimal** in-place reshape the relocation needs (de-self a + method, retype `self`). Human-reviewed, so: small, **no cross-file def relocation, no + body relocation** — the code stays where it is. +- **move** — the pure relocation; carries the **bulk**; certified by the reproduce proof + (`guide-construct-proof.md`; property: `spec-reproduction-utils.md`). +- **postpare (optional)** — a **minimal** tail fixup the move cannot do mechanically (a + module path inside a string literal, a doc reference). Human-reviewed. + +Hard lines ("prep" below = the prepare phase): + +- Both ends are optional, minimal, and covered by tests; neither ever relocates a def + across files or moves a body. +- The move-artifact whitelist is what a relocation *forces* — **not** a licence to fold + reshape work into the move. Anything outside the artifacts in the move's diff = the + reshape leaked; push it back into prep. +- **A large semantic refactor is not a phase.** Consolidating bookkeeping, deduplicating + logic, restructuring control flow, redesigning an API → its **own commit**, reviewed for + **equivalence** (tests or a written argument). Never smuggled into prep as a "small + reshape". + +- The prep's shape depends on the destination: a module-level function (§3.1) or a class + (§3.2). +- The move is the same idea in both: a pure relocation, body byte-identical. + +## 3. Cases + +### 3.1 Case 1: method → free function + +#### 3.1.1 Commit 1 — prep: de-self in place (no relocation) + +Reshape the method **in its original file and position** so it no longer needs `self`. +The body stays put: + +- `self.X` (read) → pass `X` in as a parameter. +- `self.X = v` (write) → `return v`; the caller assigns. (Or pass an explicit mutable + object.) +- `self.other_method(...)` → prep that method in the same commit, or inject it as a + `Callable` argument. +- Once `self` is gone → mark `@staticmethod`; the body **does not move**. +- Call site: `self.foo(args)` → `TheClass.foo(args)`. + +- The decorator and the qualifier are the only artifacts the move will carry — exactly + what the whitelist (`spec-reproduction-utils.md` §2.1) forgives. + +**Check:** lint + tests pass; the diff is the body reshape plus the call-site qualifier; +nothing moved. + +#### 3.1.2 Commit 2 — move: relocate to the module + +- Cut the `@staticmethod` block; paste into the target module. +- Drop `@staticmethod`, dedent to module level — body **unchanged, line for line**. +- Source file: import the moved symbol; drop now-unused imports. +- Call site: `TheClass.foo(args)` → `foo(args)` (args untouched). + +**Check:** `mechanical_refactor_proof_generator.py ` reports `PASS`. Cross-check: +`git show --color-moved=dimmed-zebra --color-moved-ws=allow-indentation-change` +marks the whole block as moved. + +### 3.2 Case 2: method → method on a class + +- For pulling **several methods and the fields they touch** into a new (or existing) + class. +- Prep does **not** de-self — it builds the class and retypes `self`, body untouched. + +#### 3.2.1 Commit 1 — prep: build the class, retype `self` + +1. Create the target class with the fields the moved methods touch (a frozen dataclass is + simplest; drop `frozen` only if they mutate). +2. Wire an instance into the call path — composition (`self.component = Target(...)` in + the source ctor), construction at the call site, or temporarily both. +3. Retype each moved method as a `@staticmethod` whose parameter is still **named** `self` + but **typed** as the target class — body unchanged: + + ```python + class Source: + component: Target + + @staticmethod + def foo(self: Target) -> None: + ... # body still reads self.field_a / self.field_b + ``` + +4. Caller: `self.foo(...)` → `Source.foo(self.component, ...)`. + +Why keep the name `self`: + +- it is an ordinary parameter name, so every `self.X` resolves against the target class + statically and at runtime (the argument *is* a target-class instance); +- renaming it would rewrite every `self.X` and destroy the "body unchanged across both + commits" invariant. + +Boundaries: + +- **Prep stays minimal.** Signature redesign, helper extraction, parameter objects, + mutate→return, renames, method splits, dead-branch removal → later non-mechanical + commits, never prep. +- **Runtime-mutable state → inject a `Callable` getter (still prep).** State that changes + every step (counters, the current batch, running stats): inject `Callable[[], T]` into + the target ctor; rewrite `self.X` → `self.get_X()`. Do **not** thread it per call and do + **not** reach back into the source object — per-call kwargs make every call site noisy, + the API non-self-contained, and the threading a caller chore. + + ```python + class Target: + def __init__(self, *, static_field, get_running_state: "Callable[[], State]"): + self.static_field = static_field + self.get_running_state = get_running_state + + @staticmethod + def check(self: "Target") -> None: + running = self.get_running_state() # was self.running_state + ... + ``` + + ```python + # source ctor + self.component = Target( + static_field=..., + get_running_state=lambda: self.running_state, + ) + ``` + +**Check:** lint + tests pass; body unchanged; types check (`self: Target` matches the +instance the caller passes). + +#### 3.2.2 Commit 2 — move: relocate into the class + +- Cut `foo` into the target class; drop `@staticmethod` — body **unchanged, line for + line**. +- Header: `def foo(self: Target)` → `def foo(self)` (type redundant inside the class). +- Caller: `Source.foo(self.component, ...)` → `self.component.foo(...)` — the receiver + moves out of the argument list (replayed by `lower_call_sites`). + +**Check:** `mechanical_refactor_proof_generator.py ` reports `PASS`. The split +paid off: prep left the body untouched, so the move is a clean cut/paste. + +### 3.3 Case 3: extract to a new module — one move commit, no prep + +- The move gathers the defs **from wherever they sit** — no prep staging at the source + tail. Replayed by `extract_symbols_to_new_module`. +- Each def/class is cut **verbatim** (the byte diff certifies the bodies); the new file's + small header (imports, a logger, constants, a `TYPE_CHECKING` block) is authored from + the target and audited (`spec-reproduction-utils.md` §2.1). +- A module-level constant that moved into the header (e.g. `_is_hip = is_hip()`) is + dropped from the source too. +- The only work outside the move: a non-mechanical reference the move cannot derive (a + string-literal module path) — a one-line **postpare**. +- A symbol **not top-level** in the source (a method still in a class): prepare de-selfs + it out first (§3.1); the proof reports `UNSUPPORTED` until then. + +### 3.4 Case 4: extract-function — the bulk goes in the move + +- The relocated body belongs in a certified move, not buried in a prep: the + `extract_function` primitive cuts the inline block **verbatim** and authors only the + interface (signature, optional `return`, the replacing `call`). +- Faithful **only when the body moves unchanged.** De-self, control-flow restructure, or a + bookkeeping change folded in → do that as a separate semantic commit (reviewed for + equivalence) **first**, then move the now-unchanged body. +- An extraction that rewrites the body *as* it extracts is a semantic commit, not a + certifiable move — do not dress it up as one. + +## 4. Remarks + +### 4.1 A move never renames + +- The moved symbol keeps the **same name on both sides**. +- A rename — even a privacy flip `_foo` → `foo` — is its own single-purpose commit + *before* the move (rename in place, update call sites). +- A move that also renames cannot be machine-certified: split it — rename first, then + move. + +### 4.2 Anti-pattern: prep adds the body, move deletes it + +- Symptom: prep **adds** a large block to the target; the move **deletes** the same block + from the source. The order is reversed. +- Correct order: prep leaves the body in the source (target skeleton, header retype, + caller qualification only); the move does the cut/paste. +- The body appears and disappears exactly once — on the move side. Fix by pushing the + "add the body" work out of prep into the move. + +### 4.3 When NOT to split (single commit) + +- Moving an **already** module-level free function. +- Pure file rename / whole-file move. +- Trivial field deletion, or `getattr(obj, "x", ...)` → direct attribute access. +- A class-internal helper relocated next to another helper in the same module. + +### 4.4 Which actions are mechanical vs not + +- Boundary: building the component correctly the first time is mechanical; reshaping it + *after* it exists is not. + +| Action | Bucket | +|---|---| +| target class skeleton + ctor + fields | mechanical (prep) | +| `@dataclass(frozen=True, slots=True, kw_only=True)` decoration | mechanical (prep) | +| composition wiring (`self.component = Target(...)`) | mechanical (prep) | +| `Callable` getter injection for runtime-mutable state | mechanical (prep) | +| platform conditionals carried along with the body | mechanical (prep / move) | +| cross-file import path rewrites | mechanical (move) | +| field-ownership migration into the component ctor | mechanical (a single pre-step) | +| inlining an `init_*` method body into a ctor | mechanical (a single pre-step) | +| privacy flip (`_x` ↔ `x`) | mechanical (a single rename) | +| signature redesign (new kwargs, changed defaults, positional → kw-only) | **not** mechanical | +| body simplification / dead-branch removal / logic rewrite | **not** mechanical | +| semantic method rename | **not** mechanical | + +- The smaller the prep, the easier "behavior unchanged" is to confirm. +- Many small, independently reviewable commits beat one big prep mixing ten flavors of + change. +- Review order = commit order: prep → move → non-mechanical follow-ups. + +### 4.5 Naming + +- Consecutive commits with reserved suffixes; short kebab ``: + +``` +-prepare: # optional: minimal in-place reshape (de-self, or retype-self) +-move: # pure relocation, certified by the reproduce proof +-postpare: # optional: minimal tail fixup (e.g. a string-literal path) +``` + +- The `:` form is what the range command's `--match -move:` regex keys on. diff --git a/.claude/skills/mechanical-refactor-verify/guide-verify-proof.md b/.claude/skills/mechanical-refactor-verify/guide-verify-proof.md new file mode 100644 index 000000000..ced8160f2 --- /dev/null +++ b/.claude/skills/mechanical-refactor-verify/guide-verify-proof.md @@ -0,0 +1,62 @@ +# Verify a proof for a move commit + +- How the reviewer of a claimed-mechanical commit consumes its proof. +- The certified property and primitive contracts: `spec-reproduction-utils.md`. +- How the proof was produced and the folder it arrives in: `guide-construct-proof.md`. + +## 1. Re-run it + +- From the repo root: + + ```bash + python3 /repro_scripts/.py + ``` + +- The run *is* the proof — it replays the primitives from the base commit and byte-diffs + against the target in a throwaway worktree. +- Do not trust a pasted verdict you did not re-run. + +## 2. Read the verdict + +- **PASS** — byte-identical: the commit is exactly the relocations listed in the script, + nothing else. +- **RESIDUAL** — a non-empty diff: precisely the bundled non-move change. Review it as + semantic content; a legitimate tail fixup (string-literal module path, doc reference) + belongs in a postpare commit, not the move. +- **UNSUPPORTED** — no recipe inferred (cases: `guide-construct-proof.md` §2.4). Not + thereby wrong, but not machine-certified: review by hand as a prepare-style reshape, or + ask the author for a hand-written `Repro`. + +## 3. Audit the authored surfaces + +- A PASS certifies the relocated bytes; the small **authored** surfaces are reproduced + from the target and need human eyes. +- In the script, check: + - the `header=` of `extract_symbols_to_new_module` — the module audits its content + (imports / docstring / TYPE_CHECKING imports / logger / relocated `drop_assigns` + copies only); what remains for you: should those assignments move at all? + - a `leave_delegate=` on `move_symbol` — the forwarding stub is authored code in the + source file; + - the `signature=` / `return_text=` / `call=` of `extract_function` — the new + function's interface is authored; only its body is certified; + - the `drop_assigns=` list — each named constant leaves the source file. + +## 4. Know what a PASS does and does not assert + +- Requalification / lowering / repath in a script is tied to symbols the same script + relocates; a consumer-only call or import rewrite (no relocated definition) cannot + reproduce as a move — it surfaces as a residual. +- Whatever the repo's pre-commit hooks auto-fix is absorbed on both sides + (`spec-reproduction-utils.md` §4) — the hook set is part of what you trust. +- A PASS judges the **shape of a relocation**, not **intent**: "this commit is exactly + these relocations", not "this relocation was a good idea". Confirm the commit's subject + matches what the script actually moves before approving. + +## 5. Why the mechanism is trustworthy + +- It runs the real formatter and compares bytes — no diff-shape heuristic to fool + (`spec-reproduction-utils.md` §4). +- The proof is the few primitive calls in the script; auditing them (plus §3) is the + whole human surface. +- The folder is self-contained and re-runnable by anyone — a CI step or a reviewer — + without the skill installed. diff --git a/.claude/skills/mechanical-refactor-verify/mechanical_refactor_verify_utils.py b/.claude/skills/mechanical-refactor-verify/mechanical_refactor_verify_utils.py deleted file mode 100644 index 5896bcb87..000000000 --- a/.claude/skills/mechanical-refactor-verify/mechanical_refactor_verify_utils.py +++ /dev/null @@ -1,80 +0,0 @@ -"""Utilities for mechanical refactor verification scripts. - -See SKILL.md for usage and transform script template. -""" - -import shlex -import subprocess -import sys -import tempfile -from collections.abc import Callable -from pathlib import Path - - -def exec_command(cmd: str, cwd: str | None = None, check: bool = True) -> str: - print(f" $ {cmd}", flush=True) - result = subprocess.run( - cmd, - shell=True, - cwd=cwd, - capture_output=True, - text=True, - ) - if check and result.returncode != 0: - print(f"FAILED: {result.stderr}", file=sys.stderr) - sys.exit(1) - return result.stdout.strip() - - -def git_add_and_commit(message: str, cwd: str) -> None: - exec_command(f"git add -A && git commit -m {shlex.quote(message)}", cwd=cwd) - - -def dedent(text: str, n: int) -> str: - """Remove exactly n leading spaces from each line.""" - lines = text.splitlines(keepends=True) - return "".join(line[n:] if line[:n] == " " * n else line for line in lines) - - -def verify_mechanical_refactor( - base_commit: str, - target_commit: str, - transform: "Callable[[Path], None]", -) -> None: - repo_root = exec_command("git rev-parse --show-toplevel") - worktree_dir = tempfile.mkdtemp(prefix="verify-mechanical-") - branch_name = f"verify-mechanical-{base_commit[:8]}" - - try: - print(f"[1/4] Creating worktree at {base_commit[:8]}...") - exec_command( - f"git worktree add -b {branch_name} {worktree_dir} {base_commit}", - cwd=repo_root, - ) - - print("[2/4] Running transformation...") - transform(Path(worktree_dir)) - - print("[3/4] Running pre-commit...") - exec_command("pre-commit run --all-files", cwd=worktree_dir, check=False) - if exec_command("git status --porcelain", cwd=worktree_dir): - git_add_and_commit("pre-commit fixes", cwd=worktree_dir) - - print(f"[4/4] Diffing against {target_commit[:8]}...") - diff = exec_command( - f"git diff {target_commit} -- .", - cwd=worktree_dir, - check=False, - ) - - if diff: - print(f"\nFAIL: diff is non-empty:\n{diff}") - sys.exit(1) - else: - print("\nPASS: transform reproduces the commit exactly.") - - finally: - print(f"\nWorktree left at: {worktree_dir}") - print(f"Branch: {branch_name}") - print("To clean up manually:") - print(f" git worktree remove {worktree_dir} && git branch -D {branch_name}") diff --git a/.claude/skills/mechanical-refactor-verify/scripts/mechanical_refactor_proof_generator.py b/.claude/skills/mechanical-refactor-verify/scripts/mechanical_refactor_proof_generator.py new file mode 100644 index 000000000..ad36d561e --- /dev/null +++ b/.claude/skills/mechanical-refactor-verify/scripts/mechanical_refactor_proof_generator.py @@ -0,0 +1,1034 @@ +"""Infer a faithful reproduce recipe for a move commit, then emit and run a self-contained +reproduce script. Lets the verifier turn a commit a formatter re-wrapped into an auditable, +runnable reproduce script -- no one hand-writes it. + +A recipe is inferred from the commit's diff and its before-state AST: which symbols moved +(src -> dst, into which class, or into a new module), which call sites were adapted, which +imports were repathed, and the symmetric module-level import diff each file gained or lost +(realised directly with add_import / remove_imported_name, since an import diff is always +whitelisted). +``recipe_to_script`` emits a standalone ``repro_scripts/.py`` (importing only the +reproduce util); running it reproduces the commit and diffs it byte-for-byte. +``generate_range`` writes a whole folder (scripts + output.log + output.html) for a range. + +Handles a method moved onto an existing class (call sites lowered), a method moved to a +module-level free function (call sites requalified), a free-function-source move to an +existing module (callers repath their import), and a new-file extract -- where the prep +commit staged the whole module body (scaffolding plus def) as a trailing block in the +source, so the move cuts that tail into the new file (extract_to_new_module). A rename or a +statement-level reorder relocates no def and is reported unsupported. Runnable directly: + + python3 mechanical_refactor_proof_generator.py + python3 mechanical_refactor_proof_generator.py .. --match -move: --out DIR +""" + +import ast +import html +import json +import re +import subprocess +import sys +from dataclasses import asdict, dataclass, field +from pathlib import Path + +sys.path.insert(0, str(Path(__file__).resolve().parent)) + +import mechanical_refactor_reproduction_utils as rr + + +def _git_output(args: list[str], cwd: str) -> str: + """Raw stdout of a git command ("" if it fails). Not stripped, so ``ast`` line numbers + stay aligned with a file's real lines.""" + result = subprocess.run(["git", *args], cwd=cwd, capture_output=True, text=True) + return result.stdout if result.returncode == 0 else "" + + +def _repo_root() -> str: + return subprocess.run( + ["git", "rev-parse", "--show-toplevel"], + capture_output=True, + text=True, + check=True, + ).stdout.strip() + + +def _removed_symbol_names(lines: list[str]) -> set[str]: + """Names of top-level defs and classes among removed diff lines (the extract's source + relinquishes these), so a moved class is found, not just a moved def.""" + return { + m.group(2) + for ln in lines + if (m := re.match(r"\s*(?:async\s+)?(def|class)\s+(\w+)", ln)) + } + + +def _def_indent(lines: list[str], name: str) -> int | None: + for line in lines: + match = re.match(r"(\s*)(?:async\s+)?def\s+" + re.escape(name) + r"\b", line) + if match: + return len(match.group(1)) + return None + + +def _per_file_diff(commit: str, root: str) -> dict[str, dict]: + """Per-file removed/added content lines (whitespace intact) and a new-file flag.""" + out = _git_output( + ["show", commit, "--format=", "--no-color", "--no-ext-diff"], root + ) + files: dict[str, dict] = {} + path: str | None = None + in_hunk = False + for line in out.splitlines(): + header = re.match(r"diff --git a/(.*) b/(.+)$", line) + if header: + path = header.group(2) + files[path] = {"removed": [], "added": [], "new": False, "deleted": False} + in_hunk = False + elif line.startswith("new file"): + files[path]["new"] = True + elif line.startswith("deleted file"): + files[path]["deleted"] = True + elif line.startswith("@@"): + in_hunk = True + elif in_hunk and line.startswith("+"): + files[path]["added"].append(line[1:]) + elif in_hunk and line.startswith("-"): + files[path]["removed"].append(line[1:]) + return files + + +def _enclosing_function(tree: ast.AST, lineno: int) -> str | None: + best: tuple[int, str] | None = None + for node in ast.walk(tree): + if isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)): + if node.lineno <= lineno <= node.end_lineno: + if best is None or node.lineno > best[0]: + best = (node.lineno, node.name) + return best[1] if best else None + + +def _enclosing_class_of_def(tree: ast.AST, name: str) -> str | None: + for node in ast.walk(tree): + if isinstance(node, ast.ClassDef): + for child in node.body: + if ( + isinstance(child, (ast.FunctionDef, ast.AsyncFunctionDef)) + and child.name == name + ): + return node.name + return None + + +def _nested_in_function(tree: ast.AST, name: str) -> bool: + target = rr._find_def(tree, name) + if target is None: + return False + for node in ast.walk(tree): + if ( + isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)) + and node is not target + and node.lineno <= target.lineno <= node.end_lineno + ): + return True + return False + + +def _module_of_path(path: str) -> str: + return path.removeprefix("python/").removesuffix(".py").replace("/", ".") + + +def _import_pairs(text: str) -> dict: + """Module-level imports keyed one entry per imported name, so removing a single name from + a multi-name ``from x import a, b`` is not mistaken for the whole statement changing. The + value is the one-name ``add_import`` text for a gained name (the import sorter merges it). + """ + pairs: dict = {} + for node in ast.parse(text).body: + if isinstance(node, ast.Import): + for alias in node.names: + stmt = "import " + alias.name + if alias.asname: + stmt += f" as {alias.asname}" + pairs[stmt] = stmt + elif isinstance(node, ast.ImportFrom): + module = "." * node.level + (node.module or "") + for alias in node.names: + name = alias.name + (f" as {alias.asname}" if alias.asname else "") + pairs[(module, alias.name, alias.asname)] = ( + f"from {module} import {name}" + ) + return pairs + + +def _typechecking_pairs(text: str) -> dict: + """Imports inside the module's ``if TYPE_CHECKING:`` block, keyed per name (same shape as + ``_import_pairs``), so a type-only import the destination gains for a moved annotation is + inferable separately from the runtime imports.""" + pairs: dict = {} + for node in ast.parse(text).body: + if not ( + isinstance(node, ast.If) + and ast.unparse(node.test) in ("TYPE_CHECKING", "typing.TYPE_CHECKING") + ): + continue + for stmt in node.body: + if isinstance(stmt, ast.ImportFrom): + module = "." * stmt.level + (stmt.module or "") + for alias in stmt.names: + name = alias.name + (f" as {alias.asname}" if alias.asname else "") + pairs[(module, alias.name, alias.asname)] = ( + f"from {module} import {name}" + ) + elif isinstance(stmt, ast.Import): + for alias in stmt.names: + stmt_text = "import " + alias.name + if alias.asname: + stmt_text += f" as {alias.asname}" + pairs[stmt_text] = stmt_text + return pairs + + +def _local_import_of( + tree: ast.AST, fn_name: str, module: str, symbol: str +) -> str | None: + fn = rr._find_def(tree, fn_name) + if fn is None: + return None + for node in ast.walk(fn): + if ( + isinstance(node, ast.ImportFrom) + and node.module == module + and any(alias.name == symbol for alias in node.names) + ): + return f"from {module} import {symbol}" + return None + + +def _removal_from_key(path: str, key) -> dict: + """Turn an ``_import_pairs`` key the target dropped into a ``remove_imported_name`` call. A + ``(module, name, asname)`` key drops one name from a ``from`` import; a bare ``import x`` + statement string drops a plain import.""" + if isinstance(key, tuple): + module, name, asname = key + return {"path": path, "module": module, "name": name, "asname": asname} + rest = key.removeprefix("import ") + if " as " in rest: + name, asname = rest.split(" as ", 1) + else: + name, asname = rest, None + return {"path": path, "module": None, "name": name, "asname": asname} + + +def _module_assign_names(text: str) -> set: + """Names bound by module-level assignments (``logger = ...``, ``_is_hip = is_hip()``), so a + constant relocated into an extracted module can be told apart from one the source keeps. + """ + names: set = set() + for node in ast.parse(text).body: + targets = ( + node.targets + if isinstance(node, ast.Assign) + else [node.target] if isinstance(node, ast.AnnAssign) else [] + ) + names |= {t.id for t in targets if isinstance(t, ast.Name)} + return names + + +def _import_additions( + path: str, after: str, before_pairs: dict, after_pairs: dict +) -> list: + """The module-level imports a file gained, as ``add_import`` texts. A name gained from a + module the file already imported is added per-name (the sorter merges it); a name from a + wholly new module is added as the target's *verbatim* statement, so a multi-line or + magic-trailing-comma wrapping the target chose is reproduced (a freshly merged single line + would otherwise collapse and not match).""" + before_modules = {key[0] for key in before_pairs if isinstance(key, tuple)} + additions: list = [] + verbatim_modules: set = set() + for key in after_pairs: + if key in before_pairs: + continue + if isinstance(key, tuple) and key[0] not in before_modules: + verbatim_modules.add(key[0]) + else: + additions.append({"path": path, "text": after_pairs[key]}) + if verbatim_modules: + after_lines = after.splitlines(keepends=True) + for node in ast.parse(after).body: + if ( + isinstance(node, ast.ImportFrom) + and "." * node.level + (node.module or "") in verbatim_modules + ): + text = "".join(after_lines[node.lineno - 1 : node.end_lineno]) + additions.append({"path": path, "text": text.rstrip("\n")}) + return additions + + +@dataclass +class Recipe: + base: str + target: str + supported: bool = True + moves: list = field(default_factory=list) + extracts: list = field(default_factory=list) + scatter_extracts: list = field(default_factory=list) + lowerings: list = field(default_factory=list) + repaths: list = field(default_factory=list) + import_removals: list = field(default_factory=list) + module_import_removals: list = field(default_factory=list) + import_additions: list = field(default_factory=list) + typechecking_additions: list = field(default_factory=list) + deletes: list = field(default_factory=list) + notes: list = field(default_factory=list) + + +def _infer_call_adaptations( + recipe: Recipe, + files: dict[str, dict], + *, + name: str, + src: str, + src_class: str, + into_class: str | None, + commit: str, + root: str, +) -> None: + """A method-source move adapts its call sites: a move onto a class lowers the receiver + out of the args; a move to a module-level free function drops the qualifier. A caller is + a before-state call ``.name(...)`` -- matched on ``src_class`` (not a loose + text search) so the moved body's own same-named calls on a different receiver are + excluded -- and its orphaned local import of ``src_class`` is removed.""" + kind = "lower" if into_class is not None else "requalify" + src_module = _module_of_path(src) + for path, f in files.items(): + before = _git_output(["show", f"{commit}^:{path}"], root) + try: + tree = ast.parse(before) + except SyntaxError: + continue + caller_fns: set[str] = set() + for node in ast.walk(tree): + if ( + isinstance(node, ast.Call) + and isinstance(node.func, ast.Attribute) + and node.func.attr == name + and (node.args or kind == "requalify") + and ast.unparse(node.func.value) == src_class + ): + fn = _enclosing_function(tree, node.lineno) + if fn is not None: + caller_fns.add(fn) + if not caller_fns: + continue + recipe.lowerings.append( + {"name": name, "owner": src_class, "path": path, "kind": kind} + ) + for fn in sorted(caller_fns): + imp = _local_import_of(tree, fn, src_module, src_class) + if imp is not None and any(imp in r for r in f["removed"]): + recipe.import_removals.append( + {"path": path, "text": imp, "in_function": fn} + ) + + +def _infer_function_scoped_repaths( + recipe: Recipe, + files: dict[str, dict], + *, + name: str, + src: str, + dst: str, + commit: str, + root: str, +) -> None: + """A moved free function keeps the same bare call, so a caller only repaths its import. + Module-level repaths fall out of the symmetric import diff; only function-scoped imports + (which ``add_import`` cannot place) need an explicit in-place repath.""" + src_module = _module_of_path(src) + dst_module = _module_of_path(dst) + for path in sorted(files): + if path == src: + continue + before = _git_output(["show", f"{commit}^:{path}"], root) + try: + tree = ast.parse(before) + except SyntaxError: + continue + top_level = {id(node) for node in tree.body} + nested = any( + isinstance(node, ast.ImportFrom) + and id(node) not in top_level + and node.module == src_module + and any(alias.name == name for alias in node.names) + for node in ast.walk(tree) + ) + if nested: + recipe.repaths.append( + { + "path": path, + "old_module": src_module, + "new_module": dst_module, + "name": name, + } + ) + + +def _self_annotation_dropped(src_def: ast.AST | None, dst_def: ast.AST | None) -> bool: + """Whether the move drops a ``self: Target`` annotation -- the source had it and the + destination does not. Some class moves keep it (a retyped self that stays annotated), so + this is inferred from both sides rather than assumed.""" + + def has_self_annotation(node: ast.AST | None) -> bool: + return bool( + node is not None + and node.args.args + and node.args.args[0].arg == "self" + and node.args.args[0].annotation is not None + ) + + return has_self_annotation(src_def) and not has_self_annotation(dst_def) + + +def _wants_future_import(files: dict[str, dict], src: str, dst: str) -> bool: + future = "from __future__ import annotations" + gained = any(future in line for line in files[dst]["added"]) + travelled = any(future in line for line in files[src]["removed"]) + return gained and not travelled + + +def _next_sibling_def_name( + dst_tree: ast.AST, name: str, into_class: str | None +) -> str | None: + """The name of the def that immediately follows ``name`` at its scope in the destination + (module level, or inside ``into_class``), or None when ``name`` is the last def there. Lets + a move reinsert the relocated def in the chain's order instead of appending at the end. + """ + container: list = [] + if into_class is not None: + cls = rr._find_class(dst_tree, into_class) + container = cls.body if cls is not None else [] + else: + container = getattr(dst_tree, "body", []) + defs = [ + n + for n in container + if isinstance(n, (ast.FunctionDef, ast.AsyncFunctionDef, ast.ClassDef)) + ] + for i, node in enumerate(defs): + if node.name == name: + return defs[i + 1].name if i + 1 < len(defs) else None + return None + + +def _symbols_form_tail(src_text: str, symbols: list[str]) -> bool: + """Whether ``symbols`` sit at the end of the source as a contiguous block of defs/classes + and the scaffolding leading into them -- the trailing block a prep commit stages for a + new-module extract. A method still inside a class, or a symbol separated from the tail by + other code, fails this and is not an extractable tail.""" + body = ast.parse(src_text).body + wanted = set(symbols) + + def is_scaffolding(node: ast.stmt) -> bool: + if isinstance(node, (ast.Import, ast.ImportFrom)): + return True + if isinstance(node, ast.If): + return ast.unparse(node.test) in ("TYPE_CHECKING", "typing.TYPE_CHECKING") + if isinstance(node, ast.Assign): + return all(isinstance(x, ast.Name) for x in node.targets) + if isinstance(node, ast.AnnAssign): + return isinstance(node.target, ast.Name) + return False + + cut = len(body) + while cut > 0: + node = body[cut - 1] + is_symbol = ( + isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef, ast.ClassDef)) + and node.name in wanted + ) + if is_symbol or is_scaffolding(node): + cut -= 1 + else: + break + present = { + node.name + for node in body[cut:] + if isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef, ast.ClassDef)) + } + return wanted <= present + + +def _scatter_extract_layout(dst_after: str, symbols: list[str]) -> dict | None: + """For a new module whose relocated ``symbols`` form a contiguous block at the end (after + an authored header of imports, constants, a logger, a ``TYPE_CHECKING`` block), return the + ``header`` text and the ``symbols`` in target order. Returns None when a non-symbol + statement is interleaved among the symbols, so there is no clean header/body split. + """ + lines = dst_after.splitlines(keepends=True) + body = ast.parse(dst_after).body + wanted = set(symbols) + + def is_wanted(node: ast.AST) -> bool: + return ( + isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef, ast.ClassDef)) + and node.name in wanted + ) + + sym_nodes = [node for node in body if is_wanted(node)] + if len(sym_nodes) != len(wanted): + return None + first_index = body.index(sym_nodes[0]) + if any(not is_wanted(node) for node in body[first_index:]): + return None + header = "".join(lines[: rr._def_span(sym_nodes[0])[0] - 1]) + return {"header": header, "order": [node.name for node in sym_nodes]} + + +def infer_recipe(commit: str, root: str) -> Recipe: + """Infer a faithful relocation recipe for a move commit from its diff + before-state. + + A move onto an existing module/class becomes a ``move_symbol``; a move whose destination + file is new becomes an ``extract_to_new_module`` (the prep commit staged the whole module + body -- scaffolding plus def -- as a trailing block in the source, so the move cuts that + tail into the new file). Method-source moves adapt their call sites; free-function-source + moves keep the bare call and only repath imports. A rename or a statement-level reorder + relocates no def, so nothing is inferred and the commit is reported unsupported.""" + all_files = _per_file_diff(commit, root) + files = {path: f for path, f in all_files.items() if path.endswith(".py")} + recipe = Recipe(base=f"{commit}~1", target=commit) + for path in sorted(set(all_files) - set(files)): + recipe.notes.append( + f"non-Python file changed: {path} (left to the residual diff)" + ) + + def def_names(lines: list[str]) -> set[str]: + return { + m.group(1) + for ln in lines + if (m := re.match(r"\s*(?:async\s+)?def\s+(\w+)", ln)) + } + + new_files = {p for p, f in files.items() if f["new"]} + + # A new file is a staged module body cut from one source: its top-level defs and classes + # are exactly the relocated symbols (the prep commit inlined them, scaffolding included, as + # a trailing block in the source). Take the symbol list from the new file itself so a + # moved class -- not just a moved def -- is in the cut tail. + for dst in sorted(new_files): + dst_after = _git_output(["show", f"{commit}:{dst}"], root) + try: + dst_body = ast.parse(dst_after).body + except SyntaxError: + continue + symbols = [ + node.name + for node in dst_body + if isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef, ast.ClassDef)) + ] + if not symbols: + continue + srcs = { + p + for name in symbols + for p, f in files.items() + if p not in new_files and name in _removed_symbol_names(f["removed"]) + } + if len(srcs) != 1: + recipe.supported = False + recipe.notes.append( + f"{dst}: extract source not a single file ({sorted(srcs)})" + ) + continue + src = next(iter(srcs)) + src_before = _git_output(["show", f"{commit}^:{src}"], root) + if _symbols_form_tail(src_before, symbols): + recipe.extracts.append( + { + "src": src, + "dst": dst, + "symbols": symbols, + "future_import": _wants_future_import(files, src, dst), + } + ) + continue + src_top_level = { + node.name + for node in ast.parse(src_before).body + if isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef, ast.ClassDef)) + } + if not set(symbols) <= src_top_level: + recipe.supported = False + recipe.notes.append( + f"{dst}: relocated symbols are not all top-level in {src} " + "(a method still inside a class needs prep to lift it out first)" + ) + continue + # The symbols are scattered in the source (not a staged trailing block): cut each one + # verbatim and assemble the new module under its authored header (imports/constants/ + # logger/TYPE_CHECKING reproduced from the target). The defs are the proven relocation; + # only the small header is authored, so the prep no longer has to gather them at the + # source tail first. + layout = _scatter_extract_layout(dst_after, symbols) + if layout is None: + recipe.supported = False + recipe.notes.append( + f"{dst}: relocated symbols are not a trailing block in the new module " + "(a non-symbol statement is interleaved with them)" + ) + continue + # A module-level constant the source no longer assigns but the new module does (e.g. a + # ``_is_hip = is_hip()`` flag) relocated into the header, so it is dropped from the + # source too -- its copy is reproduced in the authored header. + src_after = _git_output(["show", f"{commit}:{src}"], root) + drop_assigns = sorted( + (_module_assign_names(src_before) - _module_assign_names(src_after)) + & _module_assign_names(dst_after) + ) + recipe.scatter_extracts.append( + { + "src": src, + "dst": dst, + "symbols": symbols, + "header": layout["header"], + "order": layout["order"], + "drop_assigns": drop_assigns, + } + ) + + # A move whose destination already exists becomes a move_symbol (the def relocated in + # order); a moved class to an existing file is left unsupported (move_symbol moves defs). + all_removed = [ln for f in files.values() for ln in f["removed"]] + all_added = [ln for f in files.values() for ln in f["added"]] + for name in sorted(def_names(all_removed) & def_names(all_added)): + src = next( + (p for p, f in files.items() if name in def_names(f["removed"])), None + ) + dst = next((p for p, f in files.items() if name in def_names(f["added"])), None) + if src is None or dst is None or src == dst or dst in new_files: + continue + src_before = _git_output(["show", f"{commit}^:{src}"], root) + if _nested_in_function(ast.parse(src_before), name): + recipe.notes.append(f"skip {name}: nested function (moves with parent)") + continue + src_tree = ast.parse(src_before) + src_class = _enclosing_class_of_def(src_tree, name) + dst_tree = ast.parse(_git_output(["show", f"{commit}:{dst}"], root)) + into_class = _enclosing_class_of_def(dst_tree, name) + dst_def = rr._find_def(dst_tree, name) + src_indent = _def_indent(files[src]["removed"], name) or 0 + dst_indent = _def_indent(files[dst]["added"], name) or 0 + recipe.moves.append( + { + "name": name, + "src": src, + "dst": dst, + "into_class": into_class, + "from_class": src_class, + "dedent": src_indent - dst_indent, + "dst_order": dst_def.lineno if dst_def else 0, + "before": _next_sibling_def_name(dst_tree, name, into_class), + "drop_self_annotation": _self_annotation_dropped( + rr._find_def(src_tree, name), dst_def + ), + } + ) + if src_class is not None: + _infer_call_adaptations( + recipe, + files, + name=name, + src=src, + src_class=src_class, + into_class=into_class, + commit=commit, + root=root, + ) + else: + _infer_function_scoped_repaths( + recipe, files, name=name, src=src, dst=dst, commit=commit, root=root + ) + + # Module-level imports a file gained or lost are realised directly from the symmetric + # base<->target diff: a gained name is added (the destination needs the moved code's + # imports, or a caller of a moved free function gains one), a lost name is removed. An + # import diff is always whitelisted, so this is deterministic and does not depend on the + # formatter pruning (this repo's ruff has no F811, so a still-used symbol repointed to a new + # module would otherwise leave a duplicate). A file written whole by extract_to_new_module + # (the new file, or the extract source whose tail the cut removed) is skipped. + extract_dsts = {ex["dst"] for ex in recipe.extracts} + extract_srcs = {ex["src"] for ex in recipe.extracts} + for path in sorted(files): + if path in new_files or path in extract_dsts: + continue + before = _git_output(["show", f"{commit}^:{path}"], root) + after = _git_output(["show", f"{commit}:{path}"], root) + before_pairs = _import_pairs(before) if before.strip() else {} + after_pairs = _import_pairs(after) if after.strip() else {} + recipe.import_additions.extend( + _import_additions(path, after, before_pairs, after_pairs) + ) + if path not in extract_srcs: + for key in before_pairs: + if key not in after_pairs: + recipe.module_import_removals.append(_removal_from_key(path, key)) + before_tc = _typechecking_pairs(before) if before.strip() else {} + after_tc = _typechecking_pairs(after) if after.strip() else {} + for key, stmt in after_tc.items(): + if key not in before_tc: + recipe.typechecking_additions.append({"path": path, "text": stmt}) + + # A move source the commit deletes (its defs all relocated, leaving only scaffolding) is + # removed after the moves; move_symbol only cuts defs, it does not delete the emptied file. + move_srcs = {mv["src"] for mv in recipe.moves} + for path, f in files.items(): + if f.get("deleted") and path in move_srcs: + recipe.deletes.append(path) + + if not recipe.moves and not recipe.extracts and not recipe.scatter_extracts: + recipe.supported = False + if not recipe.notes: + recipe.notes.append( + "no def relocated (rename or statement-level change): review as prep" + ) + return recipe + + +def _recipe_ops(recipe: Recipe) -> list: + """The ordered relocation operations a recipe replays, as ``(method, args, kwargs)`` -- + shared by ``build_repro`` (which runs them on a Repro) and ``recipe_to_script`` (which + renders them as ``r.method(...)`` lines), so the emitted script and the in-process run can + never drift. + + Call sites and import repaths/removals run BEFORE the moves, so a call to a moved method + from inside another moved method is adapted while still in the source and travels with the + body. The moves (in destination order) and the new-module extracts relocate next. + Module-level import additions/removals run LAST, so a consumer import lands after an extract + has cut the source tail (otherwise it would be swept into the new module). Same-destination + moves are emitted in reverse destination order so each move's ``before`` anchor (a sibling + further down) is already present when the move is inserted.""" + ops: list = [] + for lo in recipe.lowerings: + method = ( + "requalify_call_sites" if lo["kind"] == "requalify" else "lower_call_sites" + ) + ops.append((method, (lo["name"], lo["owner"]), {"paths": [lo["path"]]})) + for rp in recipe.repaths: + ops.append( + ( + "repath_import", + (rp["path"],), + { + "old_module": rp["old_module"], + "new_module": rp["new_module"], + "name": rp["name"], + }, + ) + ) + for im in recipe.import_removals: + ops.append( + ( + "remove_import", + (im["path"], im["text"]), + {"in_function": im["in_function"]}, + ) + ) + for mv in sorted(recipe.moves, key=lambda m: (m["dst"], -m["dst_order"])): + ops.append( + ( + "move_symbol", + (mv["name"],), + { + "src": mv["src"], + "dst": mv["dst"], + "into_class": mv["into_class"], + "from_class": mv.get("from_class"), + "dedent": mv["dedent"], + "drop_self_annotation": mv["drop_self_annotation"], + "before": mv.get("before"), + }, + ) + ) + for ex in recipe.extracts: + ops.append( + ( + "extract_to_new_module", + (ex["src"], ex["dst"]), + {"symbols": ex["symbols"], "future_import": ex["future_import"]}, + ) + ) + for ex in recipe.scatter_extracts: + ops.append( + ( + "extract_symbols_to_new_module", + (ex["src"], ex["dst"]), + { + "symbols": ex["symbols"], + "header": ex["header"], + "order": ex["order"], + "drop_assigns": ex["drop_assigns"], + }, + ) + ) + for path in recipe.deletes: + ops.append(("delete_file", (path,), {})) + for im in recipe.module_import_removals: + ops.append( + ( + "remove_imported_name", + (im["path"],), + {"module": im["module"], "name": im["name"], "asname": im["asname"]}, + ) + ) + for im in recipe.import_additions: + ops.append(("add_import", (im["path"], im["text"]), {})) + for im in recipe.typechecking_additions: + ops.append(("add_typechecking_import", (im["path"], im["text"]), {})) + return ops + + +def build_repro(recipe: Recipe, repo_root: str | None = None) -> rr.Repro: + """Compose a Repro from the recipe's canonical ordered operations (``_recipe_ops``).""" + repro = rr.Repro(base=recipe.base, target=recipe.target, repo_root=repo_root) + for method, args, kwargs in _recipe_ops(recipe): + getattr(repro, method)(*args, **kwargs) + return repro + + +def recipe_to_script(recipe: Recipe, subject: str) -> str: + """A standalone, auditable reproduce script (imports only the reproduce util).""" + lines = [ + '"""Auto-generated reproduce script. Audit each call, then run.', + "", + f"commit: {recipe.target}", + f"subject: {subject}", + "", + "Each call is a faithful relocation primitive. Running this reproduces the commit", + "in a throwaway worktree and diffs it byte-for-byte; PASS means the commit is", + "exactly these relocations.", + '"""', + "import sys", + "from pathlib import Path", + "", + "sys.path.insert(0, str(Path(__file__).resolve().parent.parent))", + "from mechanical_refactor_reproduction_utils import Repro", + "", + f"r = Repro(base={recipe.base!r}, target={recipe.target!r})", + ] + for method, args, kwargs in _recipe_ops(recipe): + rendered = [repr(a) for a in args] + [f"{k}={v!r}" for k, v in kwargs.items()] + lines.append(f"r.{method}(" + ", ".join(rendered) + ")") + lines += ["r.run()", ""] + return "\n".join(lines) + + +@dataclass +class GenResult: + commit: str + subject: str + supported: bool + passed: bool + residual: str + script: str + notes: list + + +def generate_range( + rev_range: str, + *, + match: str | None = None, + out_dir: str, + repo_root: str | None = None, +) -> list[GenResult]: + """For each matched commit: infer a recipe, emit repro_scripts/.py, run it, and + record PASS / residual. Writes output.log + output.html and copies the reproduce util + so the folder is self-contained.""" + root = repo_root or _repo_root() + commits = _git_output(["rev-list", "--reverse", rev_range], root).split() + pattern = re.compile(match) if match else None + + out = Path(out_dir) + scripts_dir = out / "repro_scripts" + scripts_dir.mkdir(parents=True, exist_ok=True) + (out / "mechanical_refactor_reproduction_utils.py").write_text( + Path(rr.__file__).read_text() + ) + + results: list[GenResult] = [] + for commit in commits: + subject = _git_output(["log", "-1", "--format=%s", commit], root).strip() + if pattern is not None and not pattern.search(subject): + continue + passed, residual, script, supported, notes = False, "", "", False, [] + try: + recipe = infer_recipe(commit, root) + script = recipe_to_script(recipe, subject) + (scripts_dir / f"{commit[:9]}.py").write_text(script) + relocates = bool(recipe.moves or recipe.extracts or recipe.scatter_extracts) + supported = recipe.supported and relocates + notes = recipe.notes + if supported: + residual = build_repro(recipe, repo_root=root).run() + passed = residual == "" + except Exception as exc: + supported = False + residual = f"reproduce raised {type(exc).__name__}: {exc}" + notes = notes + [residual] + results.append( + GenResult( + commit=commit, + subject=subject, + supported=supported, + passed=passed, + residual=residual, + script=script, + notes=notes, + ) + ) + + _write_log(out / "output.log", rev_range, results) + _write_html(out / "output.html", rev_range, results) + return results + + +def _write_log(path: Path, rev_range: str, results: list[GenResult]) -> None: + n_pass = sum(1 for r in results if r.passed) + lines = [ + f"reproduce-gen: {rev_range}", + f"{len(results)} commit(s): {n_pass} reproduced, {len(results) - n_pass} not", + "", + ] + for r in results: + if r.passed: + verdict = "PASS" + elif not r.supported: + verdict = "UNSUPPORTED (" + "; ".join(r.notes) + ")" + else: + verdict = f"RESIDUAL ({len(r.residual.splitlines())} lines)" + lines.append(f"{r.commit[:9]} {verdict} {r.subject}") + path.write_text("\n".join(lines) + "\n") + + +def _write_html(path: Path, rev_range: str, results: list[GenResult]) -> None: + payload = { + "title": rev_range, + "passed": sum(1 for r in results if r.passed), + "total": len(results), + "results": [asdict(r) for r in results], + } + data = json.dumps(payload, ensure_ascii=False).replace(" + + +reproduce-gen __TITLE__ + + +

reproduce-gen: __TITLE__

+
+
+ +""" + + +def _main(argv: list[str]) -> int: + out_dir = None + if "--out" in argv: + i = argv.index("--out") + out_dir = argv[i + 1] + argv = argv[:i] + argv[i + 2 :] + match = None + if "--match" in argv: + i = argv.index("--match") + match = argv[i + 1] + argv = argv[:i] + argv[i + 2 :] + if len(argv) != 1: + print( + "usage: python3 mechanical_refactor_proof_generator.py \n" + " python3 mechanical_refactor_proof_generator.py .. " + "[--match REGEX] --out DIR", + file=sys.stderr, + ) + return 2 + target = argv[0] + if ".." in target: + assert out_dir, "--out DIR is required for a range" + results = generate_range(target, match=match, out_dir=out_dir) + n = sum(1 for r in results if r.passed) + print(f"{n}/{len(results)} reproduced; folder: {out_dir}") + return 0 + root = _repo_root() + recipe = infer_recipe(target, root) + print( + recipe_to_script( + recipe, _git_output(["log", "-1", "--format=%s", target], root) + ) + ) + relocates = bool(recipe.moves or recipe.extracts or recipe.scatter_extracts) + if not (recipe.supported and relocates): + print("UNSUPPORTED: " + "; ".join(recipe.notes), file=sys.stderr) + return 1 + residual = build_repro(recipe, repo_root=root).run() + return 0 if residual == "" else 1 + + +if __name__ == "__main__": + sys.exit(_main(sys.argv[1:])) diff --git a/.claude/skills/mechanical-refactor-verify/scripts/mechanical_refactor_reproduction_utils.py b/.claude/skills/mechanical-refactor-verify/scripts/mechanical_refactor_reproduction_utils.py new file mode 100644 index 000000000..972d1c968 --- /dev/null +++ b/.claude/skills/mechanical-refactor-verify/scripts/mechanical_refactor_reproduction_utils.py @@ -0,0 +1,1202 @@ +"""Reproduce a whole mechanical refactor and diff it byte-for-byte against a commit. + +You write a ``transform()`` that recreates the change; ``verify_mechanical_refactor`` +checks out the base commit in a throwaway worktree, runs the transform, runs pre-commit on +the changed files, and diffs the result against the target commit. An empty diff is a PASS. +Use this for a single mechanical PR -- a relocation, a whole-file split, or a rename where a +formatter re-wraps lines, which reproduce-and-byte-diff certifies exactly. + +The ``Repro`` builder composes faithful relocation primitives (``move_symbol``, +``extract_to_new_module``, ``extract_symbols_to_new_module``, ``extract_function``, +``lower_call_sites``, ``requalify_call_sites``, ``remove_import``, ``remove_imported_name``, +``add_import``, ``repath_import``, ``add_typechecking_import``) into a transform, so a move +that a formatter re-wrapped can be reproduced and certified. Each primitive does only a relocation-faithful +edit (it never changes logic), so a byte match after the formatter certifies the commit is +exactly that relocation. The primitives are deliberately small -- AST-located, spliced as +original source text; see spec-reproduction-utils.md. + +This module is self-contained and needs only git and the standard library. +""" + +import ast +import io +import re +import shlex +import subprocess +import sys +import tempfile +import tokenize +from collections.abc import Callable +from pathlib import Path + + +def exec_command(cmd: str, cwd: str | None = None, check: bool = True) -> str: + print(f" $ {cmd}", flush=True) + result = subprocess.run( + cmd, + shell=True, + cwd=cwd, + capture_output=True, + text=True, + ) + if check and result.returncode != 0: + raise RuntimeError(f"command failed: {cmd}\n{result.stderr.strip()}") + return result.stdout.strip() + + +def git_add_and_commit(message: str, cwd: str) -> None: + exec_command(f"git add -A && git commit -m {shlex.quote(message)}", cwd=cwd) + + +def dedent(text: str, n: int) -> str: + """Remove exactly n leading spaces from each line.""" + lines = _split_keepends(text) + return "".join(line[n:] if line[:n] == " " * n else line for line in lines) + + +def _split_keepends(text: str) -> list[str]: + """Split into lines ending in "\\n" only -- unlike ``str.splitlines``, a form feed or + other exotic line break stays inside its line, matching ast's line numbering.""" + parts = text.split("\n") + lines = [part + "\n" for part in parts[:-1]] + if parts[-1]: + lines.append(parts[-1]) + return lines + + +def _read_source(path: Path) -> str: + """Read preserving the file's line endings (no universal-newline translation), so a + CRLF file round-trips byte-for-byte through the primitives.""" + with path.open("r", newline="") as f: + return f.read() + + +def _write_source(path: Path, text: str) -> None: + with path.open("w", newline="") as f: + f.write(text) + + +def _newline_style(text: str) -> str: + return "\r\n" if "\r\n" in text else "\n" + + +def verify_mechanical_refactor( + base_commit: str, + target_commit: str, + transform: "Callable[[Path], None]", +) -> None: + repo_root = exec_command("git rev-parse --show-toplevel") + worktree_dir = tempfile.mkdtemp(prefix="verify-mechanical-") + branch_name = f"verify-mechanical-{base_commit[:8]}" + + try: + print(f"[1/4] Creating worktree at {base_commit[:8]}...") + exec_command( + f"git worktree add -b {branch_name} {worktree_dir} {base_commit}", + cwd=repo_root, + ) + + print("[2/4] Running transformation...") + transform(Path(worktree_dir)) + + print("[3/4] Running pre-commit...") + exec_command("git add -A", cwd=worktree_dir) + changed = exec_command( + f"git diff --cached --name-only --diff-filter=ACMR {base_commit}", + cwd=worktree_dir, + ).split() + if changed: + files = " ".join(shlex.quote(path) for path in changed) + exec_command( + f"pre-commit run --files {files}", cwd=worktree_dir, check=False + ) + if exec_command("git status --porcelain", cwd=worktree_dir): + git_add_and_commit("pre-commit fixes", cwd=worktree_dir) + + print(f"[4/4] Diffing against {target_commit[:8]}...") + diff = exec_command( + f"git diff {target_commit} -- .", + cwd=worktree_dir, + check=False, + ) + + if diff: + print(f"\nFAIL: diff is non-empty:\n{diff}") + sys.exit(1) + else: + print("\nPASS: transform reproduces the commit exactly.") + + finally: + print(f"\nWorktree left at: {worktree_dir}") + print(f"Branch: {branch_name}") + print("To clean up manually:") + print(f" git worktree remove {worktree_dir} && git branch -D {branch_name}") + + +# Decorators a method sheds when it becomes a free function; carried on one side of a move. +_MOVE_DECORATORS = {"@staticmethod", "@classmethod"} + + +def _find_def(tree: ast.AST, name: str) -> ast.AST | None: + for node in ast.walk(tree): + if ( + isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)) + and node.name == name + ): + return node + return None + + +def _find_unique_def( + tree: ast.AST, name: str, *, from_class: str | None = None, where: str +) -> ast.AST: + """Resolve ``def name`` and refuse ambiguity: with same-named defs in scope the + first-match lookup could silently cut the wrong body, so the caller must scope the + search with ``from_class``.""" + root: ast.AST = tree + if from_class is not None: + cls = _find_class(tree, from_class) + assert cls is not None, f"class {from_class} not found in {where}" + root = cls + elif isinstance(tree, ast.Module): + top_level = [ + node + for node in tree.body + if isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)) + and node.name == name + ] + if len(top_level) == 1: + return top_level[0] + matches = [ + node + for node in ast.walk(root) + if isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)) + and node.name == name + ] + assert matches, f"{name} not found in {where}" + assert ( + len(matches) == 1 + ), f"{len(matches)} defs named {name} in {where}; pass from_class to disambiguate" + return matches[0] + + +def _def_header_end(def_text: str) -> int: + """1-based line (within ``def_text``, which starts at the def line) of the colon that + opens the body. Tokenize-based, so parentheses inside string defaults do not confuse + the bracket depth.""" + depth = 0 + for token in tokenize.generate_tokens(io.StringIO(def_text).readline): + if token.type == tokenize.OP: + if token.string in "([{": + depth += 1 + elif token.string in ")]}": + depth -= 1 + elif token.string == ":" and depth == 0: + return token.start[0] + raise AssertionError("no header-ending colon found") + + +def _find_class(tree: ast.AST, name: str) -> ast.ClassDef | None: + for node in ast.walk(tree): + if isinstance(node, ast.ClassDef) and node.name == name: + return node + return None + + +def _def_span(node: ast.AST) -> tuple[int, int]: + """(first, last) 1-based line numbers of a def, including its decorators.""" + start = min([node.lineno] + [d.lineno for d in node.decorator_list]) + return start, node.end_lineno + + +def _byte_slice(line: str, start: int | None, end: int | None) -> str: + """Slice a line by UTF-8 byte offsets -- ast col_offsets count bytes, not characters.""" + return line.encode("utf-8")[start:end].decode("utf-8") + + +def _replace_span(text: str, sl: int, sc: int, el: int, ec: int, repl: str) -> str: + """Replace the text from (sl, sc) to (el, ec) -- 1-based lines, 0-based byte columns, + end exclusive (ast node span semantics) -- with ``repl``.""" + lines = _split_keepends(text) + before = "".join(lines[: sl - 1]) + _byte_slice(lines[sl - 1], None, sc) + after = _byte_slice(lines[el - 1], ec, None) + "".join(lines[el:]) + return before + repl + after + + +def _slice_span(text: str, sl: int, sc: int, el: int, ec: int) -> str: + lines = _split_keepends(text) + if sl == el: + return _byte_slice(lines[sl - 1], sc, ec) + return ( + _byte_slice(lines[sl - 1], sc, None) + + "".join(lines[sl : el - 1]) + + _byte_slice(lines[el - 1], None, ec) + ) + + +def _node_slice(text: str, node: ast.AST) -> str: + return _slice_span( + text, node.lineno, node.col_offset, node.end_lineno, node.end_col_offset + ) + + +def _rewrite_matching_calls( + text: str, predicate: "Callable", rewrite: "Callable" +) -> str: + """Rewrite every call ``predicate`` accepts by splicing the original source text + (never ``ast.unparse``, which would re-spell literals and drop comments). One call is + rewritten per pass and the text re-parsed, so a matching call nested inside another + match is rewritten on a later pass instead of being overwritten.""" + while True: + node = next( + ( + n + for n in ast.walk(ast.parse(text)) + if isinstance(n, ast.Call) and predicate(n) + ), + None, + ) + if node is None: + return text + text = _replace_span( + text, + node.lineno, + node.col_offset, + node.end_lineno, + node.end_col_offset, + rewrite(text, node), + ) + + +def _lowered_call_text(text: str, node: ast.Call) -> str: + """Original call text with the leading receiver argument spliced out and made the + call's new receiver: ``Owner.foo(recv, rest...)`` -> ``recv.foo(rest...)``. All other + argument bytes (literal spelling, comments, the magic trailing comma) are untouched. + """ + receiver = node.args[0] + receiver_src = _node_slice(text, receiver) + assert ( + "\n" not in receiver_src and "#" not in receiver_src + ), f"receiver {receiver_src!r} must be single-line and comment-free" + opener = _slice_span( + text, + node.func.end_lineno, + node.func.end_col_offset, + receiver.lineno, + receiver.col_offset, + ) + assert "#" not in opener, f"comment before the receiver in {opener!r}" + seg = _slice_span( + text, + receiver.end_lineno, + receiver.end_col_offset, + node.end_lineno, + node.end_col_offset, + ) + head, comma, rest = seg.partition(",") + assert "#" not in head, f"comment after the receiver in {head!r}" + if comma: + assert head.strip() == "", f"unexpected text {head!r} after the receiver" + rest = rest.lstrip(" \t") + else: + assert head.strip() == ")", f"unexpected text {head!r} after the receiver" + rest = head.lstrip(" \t") + return f"{receiver_src}.{node.func.attr}({rest}" + + +def _drop_self_annotation(method_text: str, name: str) -> str: + """Drop the type annotation from a moved method's ``self`` parameter -- relocating + ``def foo(self: Target)`` into ``Target`` makes the annotation redundant. The body is + otherwise untouched, so the relocation stays byte-faithful. ``method_text`` may still be + class-indented, so it is dedented to parse and the columns are mapped back.""" + first_line = method_text.split("\n", 1)[0] + base_indent = len(first_line) - len(first_line.lstrip(" ")) + fn = _find_def(ast.parse(dedent(method_text, base_indent)), name) + if fn is None or not fn.args.args: + return method_text + first = fn.args.args[0] + if first.arg != "self" or first.annotation is None: + return method_text + annotation = first.annotation + return _replace_span( + method_text, + first.lineno, + first.col_offset + base_indent + len("self"), + annotation.end_lineno, + annotation.end_col_offset + base_indent, + "", + ) + + +def _multiline_string_interior_lines(top_level_text: str) -> set[int]: + """1-based lines of ``top_level_text`` that lie inside a multi-line string token + (every line after the token's opening line, including the closing-delimiter line). + Re-indenting those lines would change the literal's value, not its layout.""" + interior: set[int] = set() + for token in tokenize.generate_tokens(io.StringIO(top_level_text).readline): + if token.type == tokenize.STRING and token.end[0] > token.start[0]: + interior.update(range(token.start[0] + 1, token.end[0] + 1)) + return interior + + +def _audit_extract_header( + header: str, removed_assigns: dict[str, str | None], where: str +) -> None: + """Refuse header content the extraction cannot vouch for. The header of a scattered + extraction is authored text reproduced from the target commit, so anything beyond + imports, a TYPE_CHECKING import block, a logger, or a byte-equivalent copy of an + assignment deleted from the source would let arbitrary new code ride into the new + module under a PASS verdict.""" + header_assigned: set[str] = set() + for stmt in ast.parse(header).body: + if isinstance(stmt, (ast.Import, ast.ImportFrom)): + continue + if ( + isinstance(stmt, ast.Expr) + and isinstance(stmt.value, ast.Constant) + and isinstance(stmt.value.value, str) + ): + continue + if ( + isinstance(stmt, ast.If) + and ast.unparse(stmt.test) in ("TYPE_CHECKING", "typing.TYPE_CHECKING") + and all(isinstance(sub, (ast.Import, ast.ImportFrom)) for sub in stmt.body) + ): + continue + if isinstance(stmt, (ast.Assign, ast.AnnAssign)): + targets = stmt.targets if isinstance(stmt, ast.Assign) else [stmt.target] + names = [x.id for x in targets if isinstance(x, ast.Name)] + value_src = ast.unparse(stmt.value) if stmt.value is not None else None + if value_src == "logging.getLogger(__name__)": + continue + if names and all( + n in removed_assigns and removed_assigns[n] == value_src for n in names + ): + header_assigned.update(names) + continue + raise AssertionError( + f"unverifiable header statement in {where}: {ast.unparse(stmt)!r} is " + f"neither scaffolding nor a relocated source assignment" + ) + missing = set(removed_assigns) - header_assigned + assert not missing, ( + f"drop_assigns {sorted(missing)} deleted from the source but not reproduced " + f"in the header of {where}" + ) + + +class Repro: + """Builds a faithful relocation transform from primitives, then reproduces a commit. + + Operations are recorded and applied in order to a throwaway worktree at ``base``; the + formatter (pre-commit) runs, and the result is diffed against ``target``. ``run`` prints + PASS on an empty diff, otherwise the residual -- exactly what the relocation does not + account for (a bundled change, or a re-derived scaffold a human must confirm).""" + + def __init__(self, base: str, target: str, repo_root: str | None = None) -> None: + self.base = base + self.target = target + self.repo_root = repo_root + self.ops: list[Callable[[Path], None]] = [] + + def lower_call_sites(self, name: str, owner: str, *, paths: list[str]) -> "Repro": + """Rewrite ``owner.name(receiver, rest)`` to ``receiver.name(rest)`` -- a static + call becoming an instance-method call when ``name`` moves onto a class.""" + + def op(root: Path) -> None: + for rel in paths: + path = root / rel + + def predicate(node: ast.Call) -> bool: + return ( + isinstance(node.func, ast.Attribute) + and node.func.attr == name + and bool(node.args) + and ast.unparse(node.func.value) == owner + ) + + _write_source( + path, + _rewrite_matching_calls( + _read_source(path), predicate, _lowered_call_text + ), + ) + + self.ops.append(op) + return self + + def requalify_call_sites( + self, name: str, owner: str, *, paths: list[str] + ) -> "Repro": + """Rewrite ``owner.name(args)`` to ``name(args)`` -- dropping the qualifier when + ``name`` moves to a module-level free function.""" + + def op(root: Path) -> None: + for rel in paths: + path = root / rel + + def predicate(node: ast.Call) -> bool: + return ( + isinstance(node.func, ast.Attribute) + and node.func.attr == name + and ast.unparse(node.func.value) == owner + ) + + def rewrite(text: str, node: ast.Call) -> str: + call_src = _node_slice(text, node) + func_src = _node_slice(text, node.func) + return name + call_src[len(func_src) :] + + _write_source( + path, + _rewrite_matching_calls(_read_source(path), predicate, rewrite), + ) + + self.ops.append(op) + return self + + def remove_import( + self, rel: str, import_text: str, *, in_function: str | None = None + ) -> "Repro": + """Remove every import statement whose text contains ``import_text`` (and a trailing + blank), optionally scoped to one function so a same-text module-level import (e.g. a + ``TYPE_CHECKING`` guard) is left untouched.""" + + def op(root: Path) -> None: + path = root / rel + lines = _split_keepends(_read_source(path)) + tree = ast.parse("".join(lines)) + scope: tuple[int, int] | None = None + if in_function is not None: + fn = _find_unique_def(tree, in_function, where=rel) + scope = (fn.lineno, fn.end_lineno) + compound = ( + ast.FunctionDef, + ast.AsyncFunctionDef, + ast.ClassDef, + ast.If, + ast.For, + ast.AsyncFor, + ast.While, + ast.With, + ast.AsyncWith, + ast.Try, + ast.Match, + ) + simple_stmt_lines: dict[int, int] = {} + for stmt in ast.walk(tree): + if isinstance(stmt, ast.stmt) and not isinstance(stmt, compound): + for lineno in range(stmt.lineno, stmt.end_lineno + 1): + simple_stmt_lines[lineno] = simple_stmt_lines.get(lineno, 0) + 1 + pattern = re.compile(rf"(? "Repro": + """Drop a single imported ``name`` from a module-level import: from a ``from module + import a, b`` keep the rest and drop only ``name``; when it was the sole name -- or for + a plain ``import name`` (``module=None``) -- remove the whole statement. The symbol's + home changed, so an importer that no longer references it loses exactly that name; the + import sorter rewrites the surviving line. An import diff is always whitelisted, so this + realises a lost name directly instead of relying on the formatter to prune it. + """ + + def alias_text(alias: ast.alias) -> str: + return alias.name + (f" as {alias.asname}" if alias.asname else "") + + def op(root: Path) -> None: + path = root / rel + lines = _split_keepends(_read_source(path)) + nl = _newline_style("".join(lines)) + edits: list[tuple[int, int, str | None]] = [] + for node in ast.parse("".join(lines)).body: + if module is None: + if not isinstance(node, ast.Import): + continue + else: + if not isinstance(node, ast.ImportFrom): + continue + if "." * node.level + (node.module or "") != module: + continue + dropped_alias = next( + (a for a in node.names if a.name == name and a.asname == asname), + None, + ) + if dropped_alias is None: + continue + kept = [a for a in node.names if a is not dropped_alias] + if not kept: + edits.append((node.lineno, node.end_lineno, None)) + continue + stmt_lines = lines[node.lineno - 1 : node.end_lineno] + if any("#" in ln for ln in stmt_lines): + own = dropped_alias.lineno + own_line = lines[own - 1] + assert own_line.strip().rstrip(",").strip() == alias_text( + dropped_alias + ), ( + f"cannot drop {name!r}: it shares a line with other text and " + f"the import holds comments that a rebuild would delete" + ) + edits.append((own, own, None)) + else: + keyword = "import " if module is None else f"from {module} import " + rebuilt = keyword + ", ".join(alias_text(a) for a in kept) + nl + edits.append((node.lineno, node.end_lineno, rebuilt)) + assert edits, f"import of {name!r} from {module!r} not found in {rel}" + for lo, hi, repl in sorted(edits, reverse=True): + if repl is None: + del lines[lo - 1 : hi] + else: + lines[lo - 1 : hi] = [repl] + _write_source(path, "".join(lines)) + + self.ops.append(op) + return self + + def add_import(self, rel: str, import_stmt: str) -> "Repro": + """Append an import after the last top-level import; the formatter's import sorter + places it (so the exact insertion point does not matter).""" + + def op(root: Path) -> None: + path = root / rel + lines = _split_keepends(_read_source(path)) + nl = _newline_style("".join(lines)) + body = ast.parse("".join(lines)).body + last = 0 + if ( + body + and isinstance(body[0], ast.Expr) + and isinstance(body[0].value, ast.Constant) + and isinstance(body[0].value.value, str) + ): + last = body[0].end_lineno + for node in body: + if isinstance(node, (ast.Import, ast.ImportFrom)): + last = max(last, node.end_lineno) + _write_source( + path, "".join(lines[:last] + [import_stmt + nl] + lines[last:]) + ) + + self.ops.append(op) + return self + + def add_typechecking_import(self, rel: str, import_stmt: str) -> "Repro": + """Append ``import_stmt`` inside the file's ``if TYPE_CHECKING:`` block -- a moved + definition whose annotations reference a type needs that type imported there. The + import sorter orders the block, so the exact insertion point does not matter.""" + + def op(root: Path) -> None: + path = root / rel + lines = _split_keepends(_read_source(path)) + for node in ast.parse("".join(lines)).body: + if isinstance(node, ast.If) and ast.unparse(node.test) in ( + "TYPE_CHECKING", + "typing.TYPE_CHECKING", + ): + indent = " " * node.body[0].col_offset + at = node.body[-1].end_lineno + lines.insert( + at, indent + import_stmt + _newline_style("".join(lines)) + ) + _write_source(path, "".join(lines)) + return + raise AssertionError(f"no `if TYPE_CHECKING:` block in {rel}") + + self.ops.append(op) + return self + + def repath_import( + self, rel: str, *, old_module: str, new_module: str, name: str + ) -> "Repro": + """Repath every function-scoped ``from old_module import ... name ...`` to + ``from new_module import ...`` in place -- the moved symbol's home changed, so its + importer adjusts. Only imports nested below module level are touched; a module-level + import is left to the import sorter via add_import / remove_import.""" + + def op(root: Path) -> None: + path = root / rel + lines = _split_keepends(_read_source(path)) + tree = ast.parse("".join(lines)) + top_level = {id(node) for node in tree.body} + changed = False + for node in ast.walk(tree): + if ( + isinstance(node, ast.ImportFrom) + and id(node) not in top_level + and node.module == old_module + and any(alias.name == name for alias in node.names) + ): + spelled = "." * node.level + (node.module or "") + replaced = lines[node.lineno - 1].replace( + f"from {spelled} import", f"from {new_module} import", 1 + ) + assert ( + replaced != lines[node.lineno - 1] + ), f"import spelling {spelled!r} not found on its line in {rel}" + lines[node.lineno - 1] = replaced + changed = True + assert changed, f"nested import of {name} from {old_module} not in {rel}" + _write_source(path, "".join(lines)) + + self.ops.append(op) + return self + + def move_symbol( + self, + name: str, + *, + src: str, + dst: str, + into_class: str | None, + from_class: str | None = None, + dedent: int = 0, + drop_self_annotation: bool = False, + before: str | None = None, + leave_delegate: str | None = None, + delegate_name: str | None = None, + ) -> "Repro": + """Cut ``def name`` (with decorators) from ``src`` and paste it into ``dst`` -- + immediately above the sibling def ``before`` when given (so the relocated def lands in + the chain's order), else at the end of ``into_class`` (or module level when None) -- + dropping a move decorator and dedenting by ``dedent``. When ``drop_self_annotation``, + the moved method's ``self: Target`` annotation is dropped (redundant inside the class). + The body is moved verbatim; the formatter normalises the surrounding blank lines. + """ + + def op(root: Path) -> None: + src_path = root / src + dst_path = root / dst + src_lines = _split_keepends(_read_source(src_path)) + src_nl = _newline_style("".join(src_lines)) + node = _find_unique_def( + ast.parse("".join(src_lines)), name, from_class=from_class, where=src + ) + start, end = _def_span(node) + block = src_lines[start - 1 : end] + decorator_lines = node.lineno - start + if leave_delegate is not None: + assert not any( + ln.strip() in _MOVE_DECORATORS for ln in block[:decorator_lines] + ), f"leave_delegate on a {_MOVE_DECORATORS} method has no self to forward" + args = node.args + parts = [p.arg for p in args.posonlyargs + args.args if p.arg != "self"] + if args.vararg is not None: + parts.append(f"*{args.vararg.arg}") + parts += [f"{k.arg}={k.arg}" for k in args.kwonlyargs] + if args.kwarg is not None: + parts.append(f"**{args.kwarg.arg}") + # The signature spans the def header only (def line through the line whose + # colon opens the body). node.body[0].lineno would skip over any leading + # comment/blank lines (not AST nodes), wrongly absorbing them into the + # delegate, so the header end is found by tokenizing the def. + header_end = ( + node.lineno + - 1 + + _def_header_end("".join(src_lines[node.lineno - 1 : end])) + ) + signature = src_lines[start - 1 : header_end] + body_indent = " " * node.body[0].col_offset + returning = ( + "return await" + if isinstance(node, ast.AsyncFunctionDef) + else "return" + ) + forward = ( + f"{body_indent}{returning} self.{leave_delegate}." + f"{delegate_name or name}({', '.join(parts)})" + src_nl + ) + delegate = "".join(signature) + forward + _write_source( + src_path, + "".join(src_lines[: start - 1] + [delegate] + src_lines[end:]), + ) + else: + _write_source( + src_path, "".join(src_lines[: start - 1] + src_lines[end:]) + ) + + kept = [ + ln + for index, ln in enumerate(block) + if not (index < decorator_lines and ln.strip() in _MOVE_DECORATORS) + ] + if dedent > 0: + kept = [ + ln[dedent:] if ln[:dedent] == " " * dedent else ln for ln in kept + ] + elif dedent < 0: + pad = " " * -dedent + kept = [pad + ln if ln.strip() else ln for ln in kept] + method_text = "".join(kept) + if drop_self_annotation: + method_text = _drop_self_annotation(method_text, name) + + dst_lines = _split_keepends(_read_source(dst_path)) + dst_nl = _newline_style("".join(dst_lines)) + dst_tree = ast.parse("".join(dst_lines)) + container = dst_tree.body + if into_class is not None: + cls = _find_class(dst_tree, into_class) + assert cls is not None, f"class {into_class} not found in {dst}" + container = cls.body + target = None + if before is not None: + target = next( + ( + n + for n in container + if isinstance( + n, + (ast.FunctionDef, ast.AsyncFunctionDef, ast.ClassDef), + ) + and n.name == before + ), + None, + ) + assert target is not None, f"before={before!r} not found in {dst}" + if target is not None: + at = _def_span(target)[0] - 1 + _write_source( + dst_path, + "".join(dst_lines[:at] + [method_text, dst_nl] + dst_lines[at:]), + ) + else: + at = ( + container[-1].end_lineno + if into_class is not None + else len(dst_lines) + ) + _write_source( + dst_path, + "".join(dst_lines[:at] + [dst_nl, method_text] + dst_lines[at:]), + ) + + self.ops.append(op) + return self + + def extract_to_new_module( + self, + src: str, + dst: str, + *, + symbols: list[str], + future_import: bool = True, + ) -> "Repro": + """Cut the contiguous tail of ``src`` -- the moved ``symbols`` and the module + scaffolding that leads into them (imports, a ``TYPE_CHECKING`` guard, a logger, + module constants) -- and write it as the new module ``dst``, prepending + ``from __future__ import annotations`` when ``future_import``. The body is moved + verbatim; the formatter sorts the imports and normalises the blank lines.""" + + def op(root: Path) -> None: + src_path = root / src + dst_path = root / dst + src_lines = _split_keepends(_read_source(src_path)) + body = ast.parse("".join(src_lines)).body + wanted = set(symbols) + + def is_scaffolding(node: ast.stmt) -> bool: + if isinstance(node, (ast.Import, ast.ImportFrom)): + return True + if isinstance(node, ast.If): + return ast.unparse(node.test) in ( + "TYPE_CHECKING", + "typing.TYPE_CHECKING", + ) + if isinstance(node, ast.Assign): + return all(isinstance(x, ast.Name) for x in node.targets) + if isinstance(node, ast.AnnAssign): + return isinstance(node.target, ast.Name) + return False + + cut = len(body) + while cut > 0: + node = body[cut - 1] + is_symbol = ( + isinstance( + node, (ast.FunctionDef, ast.AsyncFunctionDef, ast.ClassDef) + ) + and node.name in wanted + ) + if is_symbol or is_scaffolding(node): + cut -= 1 + else: + break + tail = body[cut:] + present = { + node.name + for node in tail + if isinstance( + node, (ast.FunctionDef, ast.AsyncFunctionDef, ast.ClassDef) + ) + } + assert wanted <= present, f"{wanted - present} not in the cut tail of {src}" + + decorators = getattr(tail[0], "decorator_list", []) + start = min([tail[0].lineno] + [d.lineno for d in decorators]) + block = "".join(src_lines[start - 1 :]) + _write_source(src_path, "".join(src_lines[: start - 1])) + + nl = _newline_style(block) + prefix = "from __future__ import annotations" + nl if future_import else "" + dst_path.parent.mkdir(parents=True, exist_ok=True) + _write_source(dst_path, prefix + block) + + self.ops.append(op) + return self + + def extract_symbols_to_new_module( + self, + src: str, + dst: str, + *, + symbols: list[str], + header: str, + order: list[str], + drop_assigns: list[str] | None = None, + ) -> "Repro": + """Relocate the named top-level defs/classes from *scattered* positions in ``src`` into + a new module ``dst`` whose authored ``header`` (the imports, module constants, a logger, + a ``TYPE_CHECKING`` block -- harmless or re-derived boilerplate reproduced from the + target) precedes them. Unlike ``extract_to_new_module``, the symbols need not be a + contiguous tail: each is cut from ``src`` verbatim, so its body stays a proven + relocation, and the cut blocks are appended in ``order`` (their order in the target). + ``drop_assigns`` names module-level assignments (e.g. a ``_is_hip = is_hip()`` constant) + that moved into the new module's header, so they are deleted from ``src`` too -- their + relocated copy is reproduced in the authored ``header``. The formatter normalises the + spacing; the byte diff then certifies the bodies are exactly the source's, while only + the small header is authored.""" + + def op(root: Path) -> None: + src_path = root / src + dst_path = root / dst + src_lines = _split_keepends(_read_source(src_path)) + src_nl = _newline_style("".join(src_lines)) + wanted = set(symbols) + dropped = set(drop_assigns or []) + assert set(order) == wanted, f"order {order} must permute symbols {symbols}" + tree = ast.parse("".join(src_lines)) + nodes = { + node.name: node + for node in tree.body + if isinstance( + node, (ast.FunctionDef, ast.AsyncFunctionDef, ast.ClassDef) + ) + and node.name in wanted + } + missing = wanted - set(nodes) + assert not missing, f"{missing} not top-level defs/classes in {src}" + spans = {name: _def_span(node) for name, node in nodes.items()} + blocks = { + name: "".join(src_lines[start - 1 : end]) + for name, (start, end) in spans.items() + } + assign_spans: list[tuple[int, int]] = [] + assign_rewrites: list[tuple[int, int, str]] = [] + removed_assigns: dict[str, str] = {} + found_assigns: set[str] = set() + for node in tree.body: + targets = ( + node.targets + if isinstance(node, ast.Assign) + else [node.target] if isinstance(node, ast.AnnAssign) else [] + ) + names = {t.id for t in targets if isinstance(t, ast.Name)} + hit = names & dropped + if not hit: + continue + assert len(names) == len( + targets + ), f"drop_assigns {sorted(hit)}: non-name targets in {src}" + value_src = ast.unparse(node.value) if node.value is not None else None + for dropped_name in hit: + removed_assigns[dropped_name] = value_src + surviving = [ + x.id + for x in targets + if isinstance(x, ast.Name) and x.id not in dropped + ] + if surviving: + kept_stmt = ( + " = ".join(surviving) + + " = " + + _slice_span( + "".join(src_lines), + node.value.lineno, + node.value.col_offset, + node.value.end_lineno, + node.value.end_col_offset, + ) + + src_nl + ) + assign_rewrites.append((node.lineno, node.end_lineno, kept_stmt)) + else: + assign_spans.append((node.lineno, node.end_lineno)) + found_assigns |= hit + assert ( + found_assigns == dropped + ), f"{dropped - found_assigns} not assigned in {src}" + if header.strip() or removed_assigns: + _audit_extract_header(header, removed_assigns, where=dst) + cuts = [(start, end, None) for start, end in spans.values()] + cuts += [(start, end, None) for start, end in assign_spans] + cuts += assign_rewrites + for start, end, repl in sorted( + cuts, key=lambda c: (c[0], c[1]), reverse=True + ): + if repl is None: + del src_lines[start - 1 : end] + else: + src_lines[start - 1 : end] = [repl] + _write_source(src_path, "".join(src_lines)) + + gap = src_nl * 3 + relocated = gap.join(blocks[name].rstrip("\r\n") for name in order) + prefix = header.rstrip("\r\n") + gap if header.strip() else "" + dst_path.parent.mkdir(parents=True, exist_ok=True) + _write_source(dst_path, prefix + relocated + src_nl) + + self.ops.append(op) + return self + + def extract_function( + self, + src: str, + dst: str, + *, + name: str, + signature: str, + body: str, + body_indent: int, + call: str, + return_text: str | None = None, + before: str | None = None, + into_class: str | None = None, + ) -> "Repro": + """Extract an inline block into a new ``name`` function. The block ``body`` is cut from + ``src`` *verbatim* (so the byte diff certifies the function body is exactly the source's), + re-indented from ``body_indent`` to a function-body indent, and wrapped under the + authored ``signature`` (with ``return_text`` appended when given); the def is inserted + into ``dst`` (above the sibling ``before`` or at the end of ``into_class`` / module), and + the block in ``src`` is replaced by the authored ``call``. + + This is the certifiable core of an extract-function: the bulk (the relocated body) is + machine-checked, and only the small signature/return/call interface is authored. It is + faithful **only** when the body is moved unchanged -- a de-self (``self.x`` -> a + parameter), a control-flow restructure, or a bookkeeping consolidation must be done as a + separate semantic commit first, since those are not relocations (see + guide-split.md).""" + + def reindent(text: str, shift: int) -> str: + if shift == 0: + return text + interior = _multiline_string_interior_lines(dedent(text, body_indent)) + lines = _split_keepends(text) + if shift < 0: + return "".join( + ( + line[-shift:] + if index + 1 not in interior and line[:-shift] == " " * -shift + else line + ) + for index, line in enumerate(lines) + ) + pad = " " * shift + return "".join( + pad + line if line.strip() and index + 1 not in interior else line + for index, line in enumerate(lines) + ) + + def op(root: Path) -> None: + src_path = root / src + src_text = _read_source(src_path) + assert src_text.count(body) == 1, f"block not found uniquely in {src}" + at = src_text.find(body) + assert ( + at == 0 or src_text[at - 1] == "\n" + ), f"block matches mid-line in {src}; it must start at a line boundary" + _write_source(src_path, src_text.replace(body, call, 1)) + + dst_path = root / dst + dst_lines = _split_keepends(_read_source(dst_path)) + dst_nl = _newline_style("".join(dst_lines)) + sig_first = _split_keepends(signature)[0] + sig_indent = len(sig_first) - len(sig_first.lstrip(" ")) + function = ( + signature.rstrip("\r\n") + + dst_nl + + reindent(body, sig_indent + 4 - body_indent) + ) + if return_text is not None: + function = function.rstrip("\r\n") + dst_nl + return_text + function = function.rstrip("\r\n") + dst_nl + + dst_tree = ast.parse("".join(dst_lines)) + container = dst_tree.body + if into_class is not None: + cls = _find_class(dst_tree, into_class) + assert cls is not None, f"class {into_class} not found in {dst}" + container = cls.body + anchor = None + if before is not None: + anchor = next( + ( + node + for node in container + if isinstance( + node, + (ast.FunctionDef, ast.AsyncFunctionDef, ast.ClassDef), + ) + and node.name == before + ), + None, + ) + if anchor is not None: + at = _def_span(anchor)[0] - 1 + _write_source( + dst_path, + "".join(dst_lines[:at] + [function, dst_nl] + dst_lines[at:]), + ) + else: + at = ( + container[-1].end_lineno + if into_class is not None + else len(dst_lines) + ) + _write_source( + dst_path, + "".join(dst_lines[:at] + [dst_nl, function] + dst_lines[at:]), + ) + + self.ops.append(op) + return self + + def delete_file(self, path: str) -> "Repro": + """Delete a source module that its symbols' relocation left empty (the chain deletes + the leftover scaffolding-only file). Run after the moves that empty it. Refuses a + file that still holds anything beyond a docstring, imports, or a TYPE_CHECKING + block -- deleting live code is not a relocation.""" + + def op(root: Path) -> None: + target = root / path + if not target.exists(): + return + leftover = [ + ast.unparse(stmt) + for stmt in ast.parse(_read_source(target)).body + if not ( + isinstance(stmt, (ast.Import, ast.ImportFrom)) + or ( + isinstance(stmt, ast.Expr) + and isinstance(stmt.value, ast.Constant) + and isinstance(stmt.value.value, str) + ) + or ( + isinstance(stmt, ast.If) + and ast.unparse(stmt.test) + in ("TYPE_CHECKING", "typing.TYPE_CHECKING") + ) + ) + ] + assert not leftover, ( + f"{path} still holds non-scaffolding code, refusing to delete: " + f"{leftover[:3]}" + ) + target.unlink() + + self.ops.append(op) + return self + + def run(self) -> str: + """Apply the operations to a worktree at base, run pre-commit, diff against target. + Returns the residual diff ("" on a clean reproduction).""" + repo_root = self.repo_root or exec_command("git rev-parse --show-toplevel") + worktree = tempfile.mkdtemp(prefix="repro-") + branch = Path(worktree).name + try: + exec_command( + f"git worktree add -b {branch} {worktree} {self.base}", cwd=repo_root + ) + for op in self.ops: + op(Path(worktree)) + exec_command("git add -A", cwd=worktree) + changed = exec_command( + f"git diff --cached --name-only --diff-filter=ACMR {self.base}", + cwd=worktree, + ).split() + if changed: + files = " ".join(shlex.quote(path) for path in changed) + exec_command( + f"pre-commit run --files {files}", cwd=worktree, check=False + ) + if exec_command("git status --porcelain", cwd=worktree): + git_add_and_commit("repro", cwd=worktree) + diff = exec_command( + f"git diff {self.target} -- .", cwd=worktree, check=False + ) + if diff: + print(f"\nRESIDUAL ({len(diff.splitlines())} lines):\n{diff}") + else: + print("\nPASS: reproduces the commit byte-for-byte.") + return diff + finally: + exec_command( + f"git worktree remove --force {worktree}", cwd=repo_root, check=False + ) + exec_command(f"git branch -D {branch}", cwd=repo_root, check=False) diff --git a/.claude/skills/mechanical-refactor-verify/scripts/tests/proof_generator/conftest.py b/.claude/skills/mechanical-refactor-verify/scripts/tests/proof_generator/conftest.py new file mode 100644 index 000000000..11ad0980a --- /dev/null +++ b/.claude/skills/mechanical-refactor-verify/scripts/tests/proof_generator/conftest.py @@ -0,0 +1,24 @@ +import subprocess +import sys +from pathlib import Path + +import pytest + +sys.path.insert(0, str(Path(__file__).resolve().parents[2])) + +from generator_testlib import _git +from mechanical_refactor_proof_generator import ( + infer_recipe, + recipe_to_script, +) + + +@pytest.fixture +def repo(tmp_path: Path) -> Path: + root = tmp_path / "repo" + root.mkdir() + _git(root, "init", "-q") + _git(root, "config", "user.email", "test@example.com") + _git(root, "config", "user.name", "test") + _git(root, "config", "commit.gpgsign", "false") + return root diff --git a/.claude/skills/mechanical-refactor-verify/scripts/tests/proof_generator/generator_testlib.py b/.claude/skills/mechanical-refactor-verify/scripts/tests/proof_generator/generator_testlib.py new file mode 100644 index 000000000..1a98808fe --- /dev/null +++ b/.claude/skills/mechanical-refactor-verify/scripts/tests/proof_generator/generator_testlib.py @@ -0,0 +1,106 @@ +import subprocess +import sys +from pathlib import Path + +import pytest + +sys.path.insert(0, str(Path(__file__).resolve().parents[2])) + +from mechanical_refactor_proof_generator import ( + infer_recipe, + recipe_to_script, +) + + +def _git(repo: Path, *args: str) -> str: + return subprocess.run( + ["git", *args], cwd=repo, check=True, capture_output=True, text=True + ).stdout.strip() + + +def _write(repo: Path, **files: str | None) -> None: + for name, content in files.items(): + path = repo / name.replace("__", "/") + if content is None: + path.unlink() + else: + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text(content) + + +def _commit(repo: Path, message: str) -> str: + _git(repo, "add", "-A") + _git(repo, "commit", "-q", "-m", message) + return _git(repo, "rev-parse", "HEAD") + + +def _method_onto_class(repo: Path) -> None: + """Stage a base + a 'move foo from M onto C, lower the caller' commit.""" + _write( + repo, + **{ + "model.py": ( + "class M:\n" + " @staticmethod\n" + " def foo(self, x):\n" + " return x + 1\n" + "\n" + " def other(self):\n" + " return 0\n" + ), + "comp.py": "class C:\n def keep(self):\n return 1\n", + "caller.py": ( + "class K:\n" + " def run(self):\n" + " from model import M\n" + "\n" + " return M.foo(self.c, 9)\n" + ), + }, + ) + _commit(repo, "base") + _write( + repo, + **{ + "model.py": "class M:\n def other(self):\n return 0\n", + "comp.py": ( + "class C:\n" + " def keep(self):\n" + " return 1\n" + "\n" + " def foo(self, x):\n" + " return x + 1\n" + ), + "caller.py": ( + "class K:\n def run(self):\n return self.c.foo(9)\n" + ), + }, + ) + _commit(repo, "move foo onto C") + + +def _free_function_move_with_module_level_caller(repo: Path) -> None: + """Stage a free function moved model.py -> util.py whose caller imports it at module + level (so the repoint shows up in the symmetric module-level import diff).""" + _write( + repo, + **{ + "model.py": "def keep():\n return 0\n\n\ndef resolve(m):\n return m\n", + "util.py": "import os\n", + "caller.py": ( + "from model import resolve\n\n\ndef run(m):\n return resolve(m)\n" + ), + }, + ) + _commit(repo, "base") + _write( + repo, + **{ + "model.py": "def keep():\n return 0\n", + "util.py": "import os\n\n\ndef resolve(m):\n return m\n", + "caller.py": ( + "from util import resolve\n\n\ndef run(m):\n return resolve(m)\n" + ), + }, + ) + _commit(repo, "move resolve to util") diff --git a/.claude/skills/mechanical-refactor-verify/scripts/tests/proof_generator/test_infer_extracts.py b/.claude/skills/mechanical-refactor-verify/scripts/tests/proof_generator/test_infer_extracts.py new file mode 100644 index 000000000..f9e3e5b5e --- /dev/null +++ b/.claude/skills/mechanical-refactor-verify/scripts/tests/proof_generator/test_infer_extracts.py @@ -0,0 +1,230 @@ +import subprocess +import sys +from pathlib import Path + +import pytest + +sys.path.insert(0, str(Path(__file__).resolve().parents[2])) + +from generator_testlib import ( # noqa: F401 + _commit, + _free_function_move_with_module_level_caller, + _git, + _method_onto_class, + _write, +) +from mechanical_refactor_proof_generator import ( + infer_recipe, + recipe_to_script, +) + + +def test_infer_recipe_new_file_extract_from_class_method_unsupported( + repo: Path, +) -> None: + """A method still inside the class cut straight into a new module cannot be cut as a + top-level symbol, so the extract is reported unsupported (prep must lift it out first). + """ + _write( + repo, + **{ + "model.py": ( + "class M:\n" + " @staticmethod\n" + " def foo(self):\n" + " return 1\n" + "\n" + " def other(self):\n" + " return 0\n" + ) + }, + ) + _commit(repo, "base") + _write( + repo, + **{ + "model.py": "class M:\n def other(self):\n return 0\n", + "newmod.py": "def foo():\n return 1\n", + }, + ) + _commit(repo, "extract foo to a new module") + recipe = infer_recipe("HEAD", str(repo)) + assert recipe.supported is False + assert any("not all top-level" in note for note in recipe.notes) + + +def test_infer_recipe_new_file_extract_from_staged_tail(repo: Path) -> None: + """A staged trailing block (scaffolding + def at the source tail) cut into a new file + infers an extract_to_new_module, prepending the future import.""" + _write( + repo, + **{ + "model.py": ( + "class M:\n" + " def keep(self):\n" + " return 1\n" + "\n" + "\n" + "import logging\n" + "\n" + "logger = logging.getLogger(__name__)\n" + "\n" + "\n" + "def foo(x):\n" + " return x + 1\n" + ) + }, + ) + _commit(repo, "base") + _write( + repo, + **{ + "model.py": "class M:\n def keep(self):\n return 1\n", + "newmod.py": ( + "from __future__ import annotations\n" + "\n" + "import logging\n" + "\n" + "logger = logging.getLogger(__name__)\n" + "\n" + "\n" + "def foo(x):\n" + " return x + 1\n" + ), + }, + ) + _commit(repo, "extract foo to a new module") + recipe = infer_recipe("HEAD", str(repo)) + assert recipe.supported + assert recipe.moves == [] + assert recipe.extracts == [ + { + "src": "model.py", + "dst": "newmod.py", + "symbols": ["foo"], + "future_import": True, + } + ] + + +def test_infer_recipe_scattered_new_module_extract(repo: Path) -> None: + """Scattered top-level defs cut into a new module (no staged trailing block) infer a scatter + extract with the authored header and target order, not UNSUPPORTED.""" + _write( + repo, + **{ + "common.py": ( + "import os\n" + "\n" + "\n" + "def keep():\n" + " return 0\n" + "\n" + "\n" + "def beta():\n" + " return 2\n" + "\n" + "\n" + "def stay():\n" + " return 9\n" + "\n" + "\n" + "def alpha():\n" + " return 1\n" + ), + }, + ) + _commit(repo, "base") + _write( + repo, + **{ + "common.py": ( + "import os\n" + "\n" + "\n" + "def keep():\n" + " return 0\n" + "\n" + "\n" + "def stay():\n" + " return 9\n" + ), + "alloc.py": ( + "from __future__ import annotations\n" + "\n" + "import logging\n" + "\n" + "logger = logging.getLogger(__name__)\n" + "\n" + "\n" + "def alpha():\n" + " return 1\n" + "\n" + "\n" + "def beta():\n" + " return 2\n" + ), + }, + ) + _commit(repo, "extract alpha, beta to alloc.py") + recipe = infer_recipe("HEAD", str(repo)) + assert recipe.supported + assert recipe.extracts == [] + assert recipe.moves == [] + assert len(recipe.scatter_extracts) == 1 + sx = recipe.scatter_extracts[0] + assert sx["src"] == "common.py" and sx["dst"] == "alloc.py" + assert sorted(sx["symbols"]) == ["alpha", "beta"] + assert sx["order"] == ["alpha", "beta"] + assert sx["header"].startswith("from __future__ import annotations\n") + assert "logger = logging.getLogger(__name__)" in sx["header"] + assert sx["drop_assigns"] == [] + script = recipe_to_script(recipe, "extract alpha, beta to alloc.py") + assert "extract_symbols_to_new_module" in script + + +def test_infer_recipe_scatter_extract_drops_relocated_constant(repo: Path) -> None: + """A module-level constant relocated into the new module is inferred as a drop_assign so the + scatter extract removes it from the source too; a constant the source keeps is not. + """ + _write( + repo, + **{ + "common.py": ( + "from u import is_hip\n" + "\n" + "_IS_HIP = is_hip()\n" + "logger = 1\n" + "\n" + "\n" + "def moved():\n" + " return _IS_HIP\n" + "\n" + "\n" + "def keep():\n" + " return logger\n" + ), + }, + ) + _commit(repo, "base") + _write( + repo, + **{ + "common.py": ("logger = 1\n\n\ndef keep():\n return logger\n"), + "alloc.py": ( + "from __future__ import annotations\n" + "\n" + "from u import is_hip\n" + "\n" + "_IS_HIP = is_hip()\n" + "\n" + "\n" + "def moved():\n" + " return _IS_HIP\n" + ), + }, + ) + _commit(repo, "extract moved to alloc.py") + recipe = infer_recipe("HEAD", str(repo)) + assert len(recipe.scatter_extracts) == 1 + assert recipe.scatter_extracts[0]["drop_assigns"] == ["_IS_HIP"] diff --git a/.claude/skills/mechanical-refactor-verify/scripts/tests/proof_generator/test_infer_imports.py b/.claude/skills/mechanical-refactor-verify/scripts/tests/proof_generator/test_infer_imports.py new file mode 100644 index 000000000..c8f445c2e --- /dev/null +++ b/.claude/skills/mechanical-refactor-verify/scripts/tests/proof_generator/test_infer_imports.py @@ -0,0 +1,168 @@ +import subprocess +import sys +from pathlib import Path + +import pytest + +sys.path.insert(0, str(Path(__file__).resolve().parents[2])) + +from generator_testlib import ( # noqa: F401 + _commit, + _free_function_move_with_module_level_caller, + _git, + _method_onto_class, + _write, +) +from mechanical_refactor_proof_generator import ( + infer_recipe, + recipe_to_script, +) + + +def test_infer_recipe_infers_added_module_imports(repo: Path) -> None: + """An import the destination module gains (the moved code needs it) is inferred.""" + _write( + repo, + **{ + "model.py": ( + "import gc\n" + "\n" + "class M:\n" + " @staticmethod\n" + " def foo(self):\n" + " gc.collect()\n" + " return 1\n" + "\n" + " def other(self):\n" + " return 0\n" + ), + "comp.py": "class C:\n def keep(self):\n return 1\n", + }, + ) + _commit(repo, "base") + _write( + repo, + **{ + "model.py": "class M:\n def other(self):\n return 0\n", + "comp.py": ( + "import gc\n" + "\n" + "class C:\n" + " def keep(self):\n" + " return 1\n" + "\n" + " def foo(self):\n" + " gc.collect()\n" + " return 1\n" + ), + }, + ) + _commit(repo, "move foo onto C") + recipe = infer_recipe("HEAD", str(repo)) + assert {"path": "comp.py", "text": "import gc"} in recipe.import_additions + + +def test_infer_recipe_module_level_import_repoint_realised_by_diff(repo: Path) -> None: + """A module-level consumer whose import is repointed old -> new yields a remove of the old + name and an add of the new -- not a reliance on the formatter pruning a duplicate. + """ + _free_function_move_with_module_level_caller(repo) + recipe = infer_recipe("HEAD", str(repo)) + assert recipe.repaths == [] + assert { + "path": "caller.py", + "module": "model", + "name": "resolve", + "asname": None, + } in recipe.module_import_removals + assert {"path": "caller.py", "text": "from util import resolve"} in ( + recipe.import_additions + ) + + +def test_infer_recipe_removes_an_import_the_source_no_longer_uses(repo: Path) -> None: + """When the moved body took the source's only use of an import, the source's lost name is + realised as a removal (deterministic, not left to the formatter).""" + _write( + repo, + **{ + "model.py": ( + "import gc\n" + "\n" + "class M:\n" + " @staticmethod\n" + " def foo(self):\n" + " gc.collect()\n" + " return 1\n" + "\n" + " def other(self):\n" + " return 0\n" + ), + "comp.py": "class C:\n def keep(self):\n return 1\n", + }, + ) + _commit(repo, "base") + _write( + repo, + **{ + "model.py": "class M:\n def other(self):\n return 0\n", + "comp.py": ( + "import gc\n" + "\n" + "class C:\n" + " def keep(self):\n" + " return 1\n" + "\n" + " def foo(self):\n" + " gc.collect()\n" + " return 1\n" + ), + }, + ) + _commit(repo, "move foo onto C") + recipe = infer_recipe("HEAD", str(repo)) + assert { + "path": "model.py", + "module": None, + "name": "gc", + "asname": None, + } in recipe.module_import_removals + + +def test_infer_recipe_adds_wholly_new_module_import_verbatim(repo: Path) -> None: + """An import gained from a module not present in base is captured as the target's verbatim + statement (so an exploded/magic-comma wrapping is reproduced, not collapsed per-name). + """ + _write( + repo, + **{ + "model.py": "def keep():\n return 0\n\n\ndef solve(x):\n return x\n", + "util.py": "import os\n", + "caller.py": ( + "from model import solve\n\n\ndef run():\n return solve(1)\n" + ), + }, + ) + _commit(repo, "base") + _write( + repo, + **{ + "model.py": "def keep():\n return 0\n", + "util.py": "import os\n\n\ndef solve(x):\n return x\n", + "caller.py": ( + "from util import (\n solve,\n)\n\n\ndef run():\n return solve(1)\n" + ), + }, + ) + _commit(repo, "move solve to util") + recipe = infer_recipe("HEAD", str(repo)) + caller_adds = [ + a["text"] for a in recipe.import_additions if a["path"] == "caller.py" + ] + assert "from util import (\n solve,\n)" in caller_adds + assert { + "path": "caller.py", + "module": "model", + "name": "solve", + "asname": None, + } in recipe.module_import_removals diff --git a/.claude/skills/mechanical-refactor-verify/scripts/tests/proof_generator/test_infer_moves.py b/.claude/skills/mechanical-refactor-verify/scripts/tests/proof_generator/test_infer_moves.py new file mode 100644 index 000000000..ada63842b --- /dev/null +++ b/.claude/skills/mechanical-refactor-verify/scripts/tests/proof_generator/test_infer_moves.py @@ -0,0 +1,290 @@ +import subprocess +import sys +from pathlib import Path + +import pytest + +sys.path.insert(0, str(Path(__file__).resolve().parents[2])) + +from generator_testlib import ( # noqa: F401 + _commit, + _free_function_move_with_module_level_caller, + _git, + _method_onto_class, + _write, +) +from mechanical_refactor_proof_generator import ( + infer_recipe, + recipe_to_script, +) + + +def test_infer_recipe_method_onto_class(repo: Path) -> None: + """A method move onto a class infers the move, the call-site lowering, and the orphaned + local import removal.""" + _method_onto_class(repo) + recipe = infer_recipe("HEAD", str(repo)) + assert recipe.supported + assert [ + (m["name"], m["src"], m["dst"], m["into_class"], m["dedent"]) + for m in recipe.moves + ] == [("foo", "model.py", "comp.py", "C", 0)] + assert recipe.lowerings == [ + {"name": "foo", "owner": "M", "path": "caller.py", "kind": "lower"} + ] + assert recipe.import_removals == [ + {"path": "caller.py", "text": "from model import M", "in_function": "run"} + ] + assert recipe.import_additions == [] + + +def test_infer_recipe_free_function_move_uses_requalify(repo: Path) -> None: + """A move to a module-level free function dedents and requalifies the call site + (drops the qualifier), rather than lowering a receiver.""" + _write( + repo, + **{ + "model.py": ( + "class M:\n" + " @staticmethod\n" + " def foo(x):\n" + " return x + 1\n" + "\n" + " def other(self):\n" + " return 0\n" + ), + "util.py": "import os\n", + "caller.py": ( + "class K:\n" + " def run(self):\n" + " from model import M\n" + "\n" + " return M.foo(9)\n" + ), + }, + ) + _commit(repo, "base") + _write( + repo, + **{ + "model.py": "class M:\n def other(self):\n return 0\n", + "util.py": "import os\n\n\ndef foo(x):\n return x + 1\n", + "caller.py": ("class K:\n def run(self):\n return foo(9)\n"), + }, + ) + _commit(repo, "move foo to util as a free function") + recipe = infer_recipe("HEAD", str(repo)) + assert [(m["name"], m["into_class"], m["dedent"]) for m in recipe.moves] == [ + ("foo", None, 4) + ] + assert recipe.lowerings == [ + {"name": "foo", "owner": "M", "path": "caller.py", "kind": "requalify"} + ] + + +def test_infer_recipe_excludes_the_moved_bodys_own_call(repo: Path) -> None: + """A same-named call on a different receiver inside the moved body is not a caller + lowering (only `M.foo(...)` is, not `worker.foo(...)`).""" + _write( + repo, + **{ + "model.py": ( + "class M:\n" + " @staticmethod\n" + " def foo(self, x):\n" + " worker.foo(x)\n" + " return x\n" + "\n" + " def other(self):\n" + " return 0\n" + ), + "comp.py": "class C:\n def keep(self):\n return 1\n", + }, + ) + _commit(repo, "base") + _write( + repo, + **{ + "model.py": "class M:\n def other(self):\n return 0\n", + "comp.py": ( + "class C:\n" + " def keep(self):\n" + " return 1\n" + "\n" + " def foo(self, x):\n" + " worker.foo(x)\n" + " return x\n" + ), + }, + ) + _commit(repo, "move foo onto C") + recipe = infer_recipe("HEAD", str(repo)) + assert recipe.lowerings == [] + + +def test_infer_recipe_skips_nested_functions(repo: Path) -> None: + """A def nested inside a moved method is not inferred as its own move.""" + _write( + repo, + **{ + "model.py": ( + "class M:\n" + " def wrap(self):\n" + " def inner(z):\n" + " return z\n" + " return inner\n" + "\n" + " def other(self):\n" + " return 0\n" + ), + "comp.py": "class C:\n def keep(self):\n return 1\n", + }, + ) + _commit(repo, "base") + _write( + repo, + **{ + "model.py": "class M:\n def other(self):\n return 0\n", + "comp.py": ( + "class C:\n" + " def keep(self):\n" + " return 1\n" + "\n" + " def wrap(self):\n" + " def inner(z):\n" + " return z\n" + " return inner\n" + ), + }, + ) + _commit(repo, "move wrap onto C") + recipe = infer_recipe("HEAD", str(repo)) + names = [m["name"] for m in recipe.moves] + assert names == ["wrap"] + assert any("inner" in n for n in recipe.notes) + + +def test_infer_recipe_free_function_source_move_repaths_caller(repo: Path) -> None: + """A free function moved to an existing module becomes a move_symbol with the call left + bare; a caller's function-scoped import is repathed.""" + _write( + repo, + **{ + "model.py": "def keep():\n return 0\n\n\ndef resolve(m):\n return m\n", + "util.py": "import os\n", + "caller.py": ( + "class K:\n" + " def run(self):\n" + " from model import resolve\n" + "\n" + " return resolve(self.m)\n" + ), + }, + ) + _commit(repo, "base") + _write( + repo, + **{ + "model.py": "def keep():\n return 0\n", + "util.py": "import os\n\n\ndef resolve(m):\n return m\n", + "caller.py": ( + "class K:\n" + " def run(self):\n" + " from util import resolve\n" + "\n" + " return resolve(self.m)\n" + ), + }, + ) + _commit(repo, "move resolve to util") + recipe = infer_recipe("HEAD", str(repo)) + assert recipe.supported + assert [(m["name"], m["src"], m["dst"], m["into_class"]) for m in recipe.moves] == [ + ("resolve", "model.py", "util.py", None) + ] + assert recipe.lowerings == [] + assert recipe.repaths == [ + { + "path": "caller.py", + "old_module": "model", + "new_module": "util", + "name": "resolve", + } + ] + + +def test_infer_recipe_survives_a_non_python_file_in_the_commit(repo: Path) -> None: + """A commit also touching a .md file infers the move and notes the non-Python path.""" + _write( + repo, + **{ + "model.py": "def foo():\n return 1\n\n\ndef keep():\n return 0\n", + "util.py": "x = 1\n", + "README.md": "hello\n", + }, + ) + _commit(repo, "base") + _write( + repo, + **{ + "model.py": "def keep():\n return 0\n", + "util.py": "x = 1\n\n\ndef foo():\n return 1\n", + "README.md": "hello world, this is plain markdown text\n", + }, + ) + commit = _commit(repo, "move foo and touch docs") + + recipe = infer_recipe(commit, str(repo)) + + assert [mv["name"] for mv in recipe.moves] == ["foo"] + assert any("README.md" in note for note in recipe.notes) + + +def test_infer_recipe_records_the_source_class_for_disambiguation(repo: Path) -> None: + """A method move carries from_class so the cut cannot hit a same-named other method.""" + _write( + repo, + **{ + "model.py": ( + "class M:\n" + " def foo(self, x):\n" + " return x + 1\n" + "\n" + "\n" + "class Other:\n" + " def foo(self, x):\n" + " return x + 2\n" + ), + "comp.py": "class C:\n def keep(self):\n return 1\n", + }, + ) + _commit(repo, "base") + _write( + repo, + **{ + "model.py": ( + "class M:\n" + " pass\n" + "\n" + "\n" + "class Other:\n" + " def foo(self, x):\n" + " return x + 2\n" + ), + "comp.py": ( + "class C:\n" + " def keep(self):\n" + " return 1\n" + "\n" + " def foo(self, x):\n" + " return x + 1\n" + ), + }, + ) + commit = _commit(repo, "move M.foo onto C") + + recipe = infer_recipe(commit, str(repo)) + + assert [mv["from_class"] for mv in recipe.moves] == ["M"] + script = recipe_to_script(recipe, "move M.foo onto C") + assert "from_class='M'" in script diff --git a/.claude/skills/mechanical-refactor-verify/scripts/tests/proof_generator/test_script_and_diff.py b/.claude/skills/mechanical-refactor-verify/scripts/tests/proof_generator/test_script_and_diff.py new file mode 100644 index 000000000..113d8a214 --- /dev/null +++ b/.claude/skills/mechanical-refactor-verify/scripts/tests/proof_generator/test_script_and_diff.py @@ -0,0 +1,54 @@ +import subprocess +import sys +from pathlib import Path + +import pytest + +sys.path.insert(0, str(Path(__file__).resolve().parents[2])) + +from generator_testlib import ( # noqa: F401 + _commit, + _free_function_move_with_module_level_caller, + _git, + _method_onto_class, + _write, +) +from mechanical_refactor_proof_generator import ( + infer_recipe, + recipe_to_script, +) + + +def test_recipe_to_script_is_self_contained_and_ordered(repo: Path) -> None: + """The emitted script imports only the reproduce util and lowers before moving.""" + _method_onto_class(repo) + script = recipe_to_script(infer_recipe("HEAD", str(repo)), "move foo onto C") + assert "from mechanical_refactor_reproduction_utils import Repro" in script + assert script.index("lower_call_sites") < script.index("move_symbol") + assert "r.run()" in script + # importing nothing else from the skill keeps the script auditable in isolation + assert "mechanical_refactor_verify_utils" not in script + assert "mechanical_refactor_proof_generator" not in script + + +def test_recipe_to_script_orders_import_ops_after_moves(repo: Path) -> None: + """The emitted script applies module-level import add/remove AFTER the move, matching + build_repro's run order so the script and the in-process verdict cannot diverge.""" + _free_function_move_with_module_level_caller(repo) + script = recipe_to_script(infer_recipe("HEAD", str(repo)), "move resolve to util") + assert script.index("move_symbol") < script.index("remove_imported_name") + assert script.index("move_symbol") < script.index("add_import") + + +def test_per_file_diff_keeps_content_lines_starting_with_plus_signs(repo: Path) -> None: + """An added content line beginning with '++' is collected, not mistaken for a header.""" + from mechanical_refactor_proof_generator import _per_file_diff + + _write(repo, **{"notes.py": "a = 1\n"}) + _commit(repo, "base") + _write(repo, **{"notes.py": 'a = 1\nb = "++x"\n'}) + commit = _commit(repo, "add plus-plus line") + + files = _per_file_diff(commit, str(repo)) + + assert files["notes.py"]["added"] == ['b = "++x"'] diff --git a/.claude/skills/mechanical-refactor-verify/scripts/tests/reproduction_utils/conftest.py b/.claude/skills/mechanical-refactor-verify/scripts/tests/reproduction_utils/conftest.py new file mode 100644 index 000000000..3cd9c5445 --- /dev/null +++ b/.claude/skills/mechanical-refactor-verify/scripts/tests/reproduction_utils/conftest.py @@ -0,0 +1,36 @@ +import subprocess +import sys +from pathlib import Path + +import pytest + +sys.path.insert(0, str(Path(__file__).resolve().parents[2])) + +import mechanical_refactor_reproduction_utils as rr +from mechanical_refactor_reproduction_utils import ( + Repro, + _def_span, + _find_class, + _find_def, + _replace_span, + _slice_span, + dedent, + exec_command, + git_add_and_commit, + verify_mechanical_refactor, +) +from reproduction_testlib import _git + + +@pytest.fixture +def repo(tmp_path: Path) -> Path: + root = tmp_path / "repo" + root.mkdir() + _git(root, "init", "-q") + _git(root, "config", "user.email", "test@example.com") + _git(root, "config", "user.name", "test") + _git(root, "config", "commit.gpgsign", "false") + return root + + +# --- exec_command -------------------------------------------------------------- diff --git a/.claude/skills/mechanical-refactor-verify/scripts/tests/reproduction_utils/reproduction_testlib.py b/.claude/skills/mechanical-refactor-verify/scripts/tests/reproduction_utils/reproduction_testlib.py new file mode 100644 index 000000000..6eb387621 --- /dev/null +++ b/.claude/skills/mechanical-refactor-verify/scripts/tests/reproduction_utils/reproduction_testlib.py @@ -0,0 +1,49 @@ +import subprocess +import sys +from pathlib import Path + +import pytest + +sys.path.insert(0, str(Path(__file__).resolve().parents[2])) + +import mechanical_refactor_reproduction_utils as rr +from mechanical_refactor_reproduction_utils import ( + Repro, + _def_span, + _find_class, + _find_def, + _replace_span, + _slice_span, + dedent, + exec_command, + git_add_and_commit, + verify_mechanical_refactor, +) + + +def _apply(repro: Repro, root: Path) -> None: + """Run a built Repro's recorded operations against a plain directory (no git).""" + for op in repro.ops: + op(root) + + +def _git(repo: Path, *args: str) -> str: + return subprocess.run( + ["git", *args], cwd=repo, check=True, capture_output=True, text=True + ).stdout.strip() + + +def _write(repo: Path, **files: str | None) -> None: + for name, content in files.items(): + path = repo / name.replace("__", "/") + if content is None: + path.unlink() + else: + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text(content) + + +def _commit(repo: Path, message: str) -> str: + _git(repo, "add", "-A") + _git(repo, "commit", "-q", "-m", message) + return _git(repo, "rev-parse", "HEAD") diff --git a/.claude/skills/mechanical-refactor-verify/scripts/tests/reproduction_utils/test_add_imports.py b/.claude/skills/mechanical-refactor-verify/scripts/tests/reproduction_utils/test_add_imports.py new file mode 100644 index 000000000..172ed4905 --- /dev/null +++ b/.claude/skills/mechanical-refactor-verify/scripts/tests/reproduction_utils/test_add_imports.py @@ -0,0 +1,142 @@ +import subprocess +import sys +from pathlib import Path + +import pytest + +sys.path.insert(0, str(Path(__file__).resolve().parents[2])) + +import mechanical_refactor_reproduction_utils as rr +from mechanical_refactor_reproduction_utils import ( + Repro, + _def_span, + _find_class, + _find_def, + _replace_span, + _slice_span, + dedent, + exec_command, + git_add_and_commit, + verify_mechanical_refactor, +) +from reproduction_testlib import _apply, _commit, _git, _write # noqa: F401 + +# --- add_import ---------------------------------------------------------------- + + +def test_add_import_appends_after_last_top_level_import(tmp_path: Path) -> None: + """A new import is inserted right after the last module-level import.""" + (tmp_path / "m.py").write_text("import os\nimport sys\n\nx = 1\n") + r = Repro("b", "t").add_import("m.py", "from pkg import Thing") + _apply(r, tmp_path) + assert ( + tmp_path / "m.py" + ).read_text() == "import os\nimport sys\nfrom pkg import Thing\n\nx = 1\n" + + +# --- repath_import / add_typechecking_import ----------------------------------- + + +def test_add_typechecking_import_inserts_in_block(tmp_path: Path) -> None: + """The import is appended inside the existing TYPE_CHECKING block.""" + (tmp_path / "m.py").write_text( + "from typing import TYPE_CHECKING\n" + "\n" + "if TYPE_CHECKING:\n" + " from a import X\n" + "\n" + "\n" + "def f():\n" + " pass\n" + ) + r = Repro("b", "t").add_typechecking_import("m.py", "from b import Y") + _apply(r, tmp_path) + assert (tmp_path / "m.py").read_text() == ( + "from typing import TYPE_CHECKING\n" + "\n" + "if TYPE_CHECKING:\n" + " from a import X\n" + " from b import Y\n" + "\n" + "\n" + "def f():\n" + " pass\n" + ) + + +def test_add_import_into_an_empty_file(tmp_path: Path) -> None: + """Adding an import to an empty file writes just the statement.""" + (tmp_path / "m.py").write_text("") + r = Repro("b", "t").add_import("m.py", "import os") + _apply(r, tmp_path) + assert (tmp_path / "m.py").read_text() == "import os\n" + + +def test_add_import_lands_below_a_module_docstring(tmp_path: Path) -> None: + """In a file with only a docstring, the new import must land below the docstring.""" + (tmp_path / "m.py").write_text('"""Module doc."""\n\nx = 1\n') + r = Repro("b", "t").add_import("m.py", "import os") + _apply(r, tmp_path) + assert (tmp_path / "m.py").read_text().startswith('"""Module doc."""') + + +def test_add_typechecking_import_matches_qualified_typing_form(tmp_path: Path) -> None: + """A `if typing.TYPE_CHECKING:` block is recognized and receives the import.""" + (tmp_path / "m.py").write_text( + "import typing\n" + "\n" + "if typing.TYPE_CHECKING:\n" + " from a import X\n" + "\n" + "\n" + "def f():\n" + " pass\n" + ) + r = Repro("b", "t").add_typechecking_import("m.py", "from b import Y") + _apply(r, tmp_path) + assert (tmp_path / "m.py").read_text() == ( + "import typing\n" + "\n" + "if typing.TYPE_CHECKING:\n" + " from a import X\n" + " from b import Y\n" + "\n" + "\n" + "def f():\n" + " pass\n" + ) + + +def test_add_typechecking_import_after_a_multiline_final_import(tmp_path: Path) -> None: + """The insert lands after the closing paren of a multi-line final guarded import.""" + (tmp_path / "m.py").write_text( + "from typing import TYPE_CHECKING\n" + "\n" + "if TYPE_CHECKING:\n" + " from a import (\n" + " X,\n" + " )\n" + "\n" + "x = 1\n" + ) + r = Repro("b", "t").add_typechecking_import("m.py", "from b import Y") + _apply(r, tmp_path) + assert (tmp_path / "m.py").read_text() == ( + "from typing import TYPE_CHECKING\n" + "\n" + "if TYPE_CHECKING:\n" + " from a import (\n" + " X,\n" + " )\n" + " from b import Y\n" + "\n" + "x = 1\n" + ) + + +def test_add_typechecking_import_raises_without_a_block(tmp_path: Path) -> None: + """A file lacking a TYPE_CHECKING block fails loudly.""" + (tmp_path / "m.py").write_text("import os\n\nx = 1\n") + r = Repro("b", "t").add_typechecking_import("m.py", "from b import Y") + with pytest.raises(AssertionError): + _apply(r, tmp_path) diff --git a/.claude/skills/mechanical-refactor-verify/scripts/tests/reproduction_utils/test_call_sites.py b/.claude/skills/mechanical-refactor-verify/scripts/tests/reproduction_utils/test_call_sites.py new file mode 100644 index 000000000..83d8cfa5f --- /dev/null +++ b/.claude/skills/mechanical-refactor-verify/scripts/tests/reproduction_utils/test_call_sites.py @@ -0,0 +1,158 @@ +import subprocess +import sys +from pathlib import Path + +import pytest + +sys.path.insert(0, str(Path(__file__).resolve().parents[2])) + +import mechanical_refactor_reproduction_utils as rr +from mechanical_refactor_reproduction_utils import ( + Repro, + _def_span, + _find_class, + _find_def, + _replace_span, + _slice_span, + dedent, + exec_command, + git_add_and_commit, + verify_mechanical_refactor, +) +from reproduction_testlib import _apply, _commit, _git, _write # noqa: F401 + + +def test_lowered_call_text_preserves_magic_trailing_comma(tmp_path: Path) -> None: + """A magic trailing comma in the original call survives the textual lowering.""" + (tmp_path / "m.py").write_text("x = Old.foo(\n self.r,\n a,\n b,\n)\n") + r = Repro("b", "t").lower_call_sites("foo", "Old", paths=["m.py"]) + _apply(r, tmp_path) + assert (tmp_path / "m.py").read_text() == "x = self.r.foo(\n a,\n b,\n)\n" + + +# --- lower_call_sites ---------------------------------------------------------- + + +def test_lower_call_sites_moves_receiver_out_of_args(tmp_path: Path) -> None: + """Owner.foo(receiver, rest) becomes receiver.foo(rest).""" + (tmp_path / "m.py").write_text("x = ModelRunner.foo(self.r, a, b)\n") + r = Repro("b", "t").lower_call_sites("foo", "ModelRunner", paths=["m.py"]) + _apply(r, tmp_path) + assert (tmp_path / "m.py").read_text() == "x = self.r.foo(a, b)\n" + + +def test_lower_call_sites_handles_only_receiver_arg(tmp_path: Path) -> None: + """Owner.foo(receiver) becomes receiver.foo() without re-lowering the result.""" + (tmp_path / "m.py").write_text("ModelRunner.foo(self.r)\n") + r = Repro("b", "t").lower_call_sites("foo", "ModelRunner", paths=["m.py"]) + _apply(r, tmp_path) + assert (tmp_path / "m.py").read_text() == "self.r.foo()\n" + + +def test_lower_call_sites_ignores_a_different_owner(tmp_path: Path) -> None: + """A same-named call on another receiver (e.g. the moved body's own call) is untouched.""" + (tmp_path / "m.py").write_text("worker.foo(zmq)\n") + r = Repro("b", "t").lower_call_sites("foo", "ModelRunner", paths=["m.py"]) + _apply(r, tmp_path) + assert (tmp_path / "m.py").read_text() == "worker.foo(zmq)\n" + + +def test_lower_call_sites_preserves_magic_trailing_comma(tmp_path: Path) -> None: + """A magic trailing comma is kept so the formatter re-explodes the lowered call.""" + (tmp_path / "m.py").write_text("ModelRunner.foo(\n self.r,\n a,\n)\n") + r = Repro("b", "t").lower_call_sites("foo", "ModelRunner", paths=["m.py"]) + _apply(r, tmp_path) + assert (tmp_path / "m.py").read_text() == "self.r.foo(\n a,\n)\n" + + +# --- requalify_call_sites ------------------------------------------------------ + + +# --- requalify_call_sites ------------------------------------------------------ + + +def test_requalify_call_sites_drops_the_qualifier(tmp_path: Path) -> None: + """Owner.bar(args) becomes bar(args) when bar moves to a free function.""" + (tmp_path / "m.py").write_text("y = ModelRunner.bar(a, b)\n") + r = Repro("b", "t").requalify_call_sites("bar", "ModelRunner", paths=["m.py"]) + _apply(r, tmp_path) + assert (tmp_path / "m.py").read_text() == "y = bar(a, b)\n" + + +# --- adversarial audit: call-site rewrites --------------------------------------- + + +# --- adversarial audit: call-site rewrites --------------------------------------- + + +def test_requalify_call_sites_matches_a_zero_argument_call(tmp_path: Path) -> None: + """Owner.bar() with no arguments is requalified to bar().""" + (tmp_path / "m.py").write_text("y = Owner.bar()\n") + r = Repro("b", "t").requalify_call_sites("bar", "Owner", paths=["m.py"]) + _apply(r, tmp_path) + assert (tmp_path / "m.py").read_text() == "y = bar()\n" + + +def test_lower_call_sites_preserves_comments_inside_a_multiline_call( + tmp_path: Path, +) -> None: + """A comment between arguments of the rewritten call must survive.""" + (tmp_path / "m.py").write_text( + "x = Old.foo(\n self.r,\n a, # keep me\n b,\n)\n" + ) + r = Repro("b", "t").lower_call_sites("foo", "Old", paths=["m.py"]) + _apply(r, tmp_path) + assert "# keep me" in (tmp_path / "m.py").read_text() + + +def test_lower_call_sites_preserves_arg_literal_spelling(tmp_path: Path) -> None: + """Hex literals and quote styles inside the rewritten call must not be normalized.""" + (tmp_path / "m.py").write_text('x = Old.foo(self.r, 0x10, "s")\n') + r = Repro("b", "t").lower_call_sites("foo", "Old", paths=["m.py"]) + _apply(r, tmp_path) + assert (tmp_path / "m.py").read_text() == 'x = self.r.foo(0x10, "s")\n' + + +def test_lower_call_sites_lowers_a_nested_matching_call_too(tmp_path: Path) -> None: + """A matching call nested inside another matching call is lowered as well.""" + (tmp_path / "m.py").write_text("x = Old.foo(self.r, Old.foo(self.q, 1))\n") + r = Repro("b", "t").lower_call_sites("foo", "Old", paths=["m.py"]) + _apply(r, tmp_path) + assert (tmp_path / "m.py").read_text() == "x = self.r.foo(self.q.foo(1))\n" + + +def test_lower_call_sites_magic_comma_with_sole_receiver_arg_stays_valid( + tmp_path: Path, +) -> None: + """Lowering a magic-comma call whose only argument is the receiver stays valid Python.""" + (tmp_path / "m.py").write_text("Owner.foo(\n self.r,\n)\n") + r = Repro("b", "t").lower_call_sites("foo", "Owner", paths=["m.py"]) + _apply(r, tmp_path) + out = (tmp_path / "m.py").read_text() + compile(out, "m.py", "exec") + + +def test_call_rewrite_is_column_accurate_on_non_ascii_lines(tmp_path: Path) -> None: + """A call after a non-ASCII string on the same line is rewritten at the right columns.""" + (tmp_path / "m.py").write_text('x = "中文"; y = Owner.foo(self.r, 1)\n') + r = Repro("b", "t").lower_call_sites("foo", "Owner", paths=["m.py"]) + _apply(r, tmp_path) + assert (tmp_path / "m.py").read_text() == 'x = "中文"; y = self.r.foo(1)\n' + + +def test_call_rewrite_survives_a_form_feed_line_start(tmp_path: Path) -> None: + """A form feed at a line start must not shift the rewrite onto the wrong line.""" + (tmp_path / "m.py").write_text("a = 1\n\x0cb = 2\ny = Owner.foo(self.r, 1)\n") + r = Repro("b", "t").lower_call_sites("foo", "Owner", paths=["m.py"]) + _apply(r, tmp_path) + assert (tmp_path / "m.py").read_text() == "a = 1\n\x0cb = 2\ny = self.r.foo(1)\n" + + +def test_requalify_call_sites_preserves_redundant_parens_in_kwargs( + tmp_path: Path, +) -> None: + """Redundant parentheses around a keyword value survive the requalification.""" + (tmp_path / "m.py").write_text("y = Old.bar(\n a=1,\n b=(2),\n)\n") + r = Repro("b", "t").requalify_call_sites("bar", "Old", paths=["m.py"]) + _apply(r, tmp_path) + assert "b=(2)" in (tmp_path / "m.py").read_text() diff --git a/.claude/skills/mechanical-refactor-verify/scripts/tests/reproduction_utils/test_delete_file.py b/.claude/skills/mechanical-refactor-verify/scripts/tests/reproduction_utils/test_delete_file.py new file mode 100644 index 000000000..5d874c88c --- /dev/null +++ b/.claude/skills/mechanical-refactor-verify/scripts/tests/reproduction_utils/test_delete_file.py @@ -0,0 +1,49 @@ +import subprocess +import sys +from pathlib import Path + +import pytest + +sys.path.insert(0, str(Path(__file__).resolve().parents[2])) + +import mechanical_refactor_reproduction_utils as rr +from mechanical_refactor_reproduction_utils import ( + Repro, + _def_span, + _find_class, + _find_def, + _replace_span, + _slice_span, + dedent, + exec_command, + git_add_and_commit, + verify_mechanical_refactor, +) +from reproduction_testlib import _apply, _commit, _git, _write # noqa: F401 + + +def test_delete_file_removes_emptied_source(tmp_path: Path) -> None: + """delete_file removes a source module left empty after its defs relocated.""" + (tmp_path / "gone.py").write_text("import os\n") + r = Repro("b", "t").delete_file("gone.py") + _apply(r, tmp_path) + assert not (tmp_path / "gone.py").exists() + + +def test_delete_file_refuses_a_file_with_remaining_definitions(tmp_path: Path) -> None: + """Deleting a module that still contains defs must fail loudly.""" + (tmp_path / "live.py").write_text("def still_used():\n return 42\n") + r = Repro("b", "t").delete_file("live.py") + with pytest.raises(AssertionError): + _apply(r, tmp_path) + assert (tmp_path / "live.py").exists() + + +def test_delete_file_on_a_missing_path_is_a_no_op(tmp_path: Path) -> None: + """Deleting an already-absent file does nothing and raises nothing.""" + r = Repro("b", "t").delete_file("nope.py") + _apply(r, tmp_path) + assert not (tmp_path / "nope.py").exists() + + +# --- adversarial audit: extract_function ----------------------------------------- diff --git a/.claude/skills/mechanical-refactor-verify/scripts/tests/reproduction_utils/test_extract_function.py b/.claude/skills/mechanical-refactor-verify/scripts/tests/reproduction_utils/test_extract_function.py new file mode 100644 index 000000000..1a822ed89 --- /dev/null +++ b/.claude/skills/mechanical-refactor-verify/scripts/tests/reproduction_utils/test_extract_function.py @@ -0,0 +1,190 @@ +import subprocess +import sys +from pathlib import Path + +import pytest + +sys.path.insert(0, str(Path(__file__).resolve().parents[2])) + +import mechanical_refactor_reproduction_utils as rr +from mechanical_refactor_reproduction_utils import ( + Repro, + _def_span, + _find_class, + _find_def, + _replace_span, + _slice_span, + dedent, + exec_command, + git_add_and_commit, + verify_mechanical_refactor, +) +from reproduction_testlib import _apply, _commit, _git, _write # noqa: F401 + +# --- extract_function ---------------------------------------------------------- + + +def test_extract_function_relocates_body_and_replaces_with_call(tmp_path: Path) -> None: + """An inline block is cut verbatim, re-indented under the new signature, and the call site + replaced; the body lands at function-body indent.""" + (tmp_path / "src.py").write_text( + "class Q:\n" + " def run(self, n):\n" + " total = 0\n" + " for i in range(n):\n" + " total += i * i\n" + " return total\n" + ) + (tmp_path / "dst.py").write_text("def existing():\n return 0\n") + body = " total = 0\n for i in range(n):\n total += i * i\n" + r = Repro("b", "t").extract_function( + "src.py", + "dst.py", + name="sum_squares", + signature="def sum_squares(n):", + body=body, + body_indent=8, + call=" total = sum_squares(n)\n", + return_text=" return total\n", + ) + _apply(r, tmp_path) + src_out = (tmp_path / "src.py").read_text() + assert " total = sum_squares(n)\n" in src_out + assert "for i in range(n)" not in src_out + assert ( + "def sum_squares(n):\n" + " total = 0\n" + " for i in range(n):\n" + " total += i * i\n" + " return total\n" + ) in (tmp_path / "dst.py").read_text() + + +def test_extract_function_inserts_before_named_sibling(tmp_path: Path) -> None: + """With before=, the new function lands immediately above that sibling at module level.""" + (tmp_path / "src.py").write_text("x = compute()\n") + (tmp_path / "dst.py").write_text( + "def a():\n return 1\n\n\ndef c():\n return 3\n" + ) + r = Repro("b", "t").extract_function( + "src.py", + "dst.py", + name="b", + signature="def b():", + body="x = compute()\n", + body_indent=0, + call="x = b()\n", + return_text=" return x\n", + before="c", + ) + _apply(r, tmp_path) + dst_out = (tmp_path / "dst.py").read_text() + assert dst_out.index("def a") < dst_out.index("def b") < dst_out.index("def c") + assert "x = b()\n" == (tmp_path / "src.py").read_text() + + +def test_extract_function_asserts_block_not_unique(tmp_path: Path) -> None: + """A block that occurs more than once in the source raises, so the cut is unambiguous.""" + (tmp_path / "src.py").write_text("p = f()\np = f()\n") + (tmp_path / "dst.py").write_text("def z():\n return 0\n") + r = Repro("b", "t").extract_function( + "src.py", + "dst.py", + name="g", + signature="def g():", + body="p = f()\n", + body_indent=0, + call="p = g()\n", + ) + with pytest.raises(AssertionError): + _apply(r, tmp_path) + + +# --- adversarial audit: module extraction ---------------------------------------- + + +# --- adversarial audit: extract_function ----------------------------------------- + + +def test_extract_function_does_not_pad_blank_lines_in_the_body(tmp_path: Path) -> None: + """Interior blank lines of the extracted body stay bare newlines, unpadded.""" + (tmp_path / "src.py").write_text(" a = 1\n\n b = 2\n") + (tmp_path / "dst.py").write_text("def z():\n return 0\n") + r = Repro("b", "t").extract_function( + "src.py", + "dst.py", + name="g", + signature="def g():", + body=" a = 1\n\n b = 2\n", + body_indent=8, + call=" g()\n", + ) + _apply(r, tmp_path) + assert (tmp_path / "src.py").read_text() == " g()\n" + assert (tmp_path / "dst.py").read_text() == ( + "def z():\n return 0\n\ndef g():\n a = 1\n\n b = 2\n" + ) + + +def test_extract_function_does_not_reindent_string_literal_interiors( + tmp_path: Path, +) -> None: + """Triple-quoted string interior lines keep their exact bytes through the extraction.""" + (tmp_path / "src.py").write_text( + "TEMPLATE = '''\nliteral line\n'''\nx = TEMPLATE\n" + ) + (tmp_path / "dst.py").write_text("def existing():\n return 0\n") + r = Repro("b", "t").extract_function( + "src.py", + "dst.py", + name="make", + signature="def make():", + body="TEMPLATE = '''\nliteral line\n'''\nx = TEMPLATE\n", + body_indent=0, + call="x = make()\n", + return_text=" return x\n", + ) + _apply(r, tmp_path) + assert "\nliteral line\n" in (tmp_path / "dst.py").read_text() + + +def test_extract_function_rejects_a_mid_line_substring_match(tmp_path: Path) -> None: + """A body that only matches mid-line must fail loudly instead of splicing the call.""" + (tmp_path / "src.py").write_text("value = prefix_total = 0\n") + (tmp_path / "dst.py").write_text("def z():\n return 0\n") + r = Repro("b", "t").extract_function( + "src.py", + "dst.py", + name="g", + signature="def g():", + body="total = 0\n", + body_indent=0, + call="total = g()\n", + ) + with pytest.raises(AssertionError): + _apply(r, tmp_path) + + +def test_extract_function_into_class_indents_body_to_method_depth( + tmp_path: Path, +) -> None: + """Extracting into a class must indent the relocated body to method depth.""" + (tmp_path / "src.py").write_text("val = compute_thing()\n") + (tmp_path / "dst.py").write_text( + "class H:\n def last(self):\n return 0\n" + ) + r = Repro("b", "t").extract_function( + "src.py", + "dst.py", + name="helper", + signature=" def helper(self):", + body="val = compute_thing()\n", + body_indent=0, + call="val = h.helper()\n", + return_text=" return val\n", + into_class="H", + ) + _apply(r, tmp_path) + out = (tmp_path / "dst.py").read_text() + compile(out, "dst.py", "exec") + assert " def helper(self):\n val = compute_thing()\n" in out diff --git a/.claude/skills/mechanical-refactor-verify/scripts/tests/reproduction_utils/test_extract_symbols_to_new_module.py b/.claude/skills/mechanical-refactor-verify/scripts/tests/reproduction_utils/test_extract_symbols_to_new_module.py new file mode 100644 index 000000000..ed12fbb20 --- /dev/null +++ b/.claude/skills/mechanical-refactor-verify/scripts/tests/reproduction_utils/test_extract_symbols_to_new_module.py @@ -0,0 +1,199 @@ +import subprocess +import sys +from pathlib import Path + +import pytest + +sys.path.insert(0, str(Path(__file__).resolve().parents[2])) + +import mechanical_refactor_reproduction_utils as rr +from mechanical_refactor_reproduction_utils import ( + Repro, + _def_span, + _find_class, + _find_def, + _replace_span, + _slice_span, + dedent, + exec_command, + git_add_and_commit, + verify_mechanical_refactor, +) +from reproduction_testlib import _apply, _commit, _git, _write # noqa: F401 + +# --- extract_symbols_to_new_module --------------------------------------------- + + +def test_extract_symbols_to_new_module_gathers_scattered_defs(tmp_path: Path) -> None: + """Scattered top-level defs are cut from the source and assembled under the authored header + in the given order; the source keeps everything else.""" + (tmp_path / "src.py").write_text( + "import os\n" + "\n" + "\n" + "def keep_a():\n" + " return 1\n" + "\n" + "\n" + "def moved_b():\n" + " return 2\n" + "\n" + "\n" + "def keep_c():\n" + " return 3\n" + "\n" + "\n" + "def moved_a():\n" + " return 4\n" + ) + header = ( + "from __future__ import annotations\n" + "\n" + "import logging\n" + "\n" + "logger = logging.getLogger(__name__)\n" + ) + r = Repro("b", "t").extract_symbols_to_new_module( + "src.py", + "new.py", + symbols=["moved_b", "moved_a"], + header=header, + order=["moved_a", "moved_b"], + ) + _apply(r, tmp_path) + src_out = (tmp_path / "src.py").read_text() + assert "def moved_a" not in src_out and "def moved_b" not in src_out + assert "def keep_a" in src_out and "def keep_c" in src_out + new_out = (tmp_path / "new.py").read_text() + assert new_out.startswith("from __future__ import annotations\n") + assert "logger = logging.getLogger(__name__)" in new_out + assert new_out.index("def moved_a") < new_out.index("def moved_b") + assert " return 4\n" in new_out and " return 2\n" in new_out + + +def test_extract_symbols_to_new_module_asserts_order_permutes_symbols( + tmp_path: Path, +) -> None: + """An order that is not a permutation of the symbols raises, so a wrong recipe fails.""" + (tmp_path / "src.py").write_text( + "def a():\n return 1\n\n\ndef b():\n return 2\n" + ) + r = Repro("b", "t").extract_symbols_to_new_module( + "src.py", "n.py", symbols=["a", "b"], header="", order=["a"] + ) + with pytest.raises(AssertionError): + _apply(r, tmp_path) + + +def test_extract_symbols_to_new_module_asserts_when_symbol_absent( + tmp_path: Path, +) -> None: + """A symbol that is not a top-level def/class in the source raises.""" + (tmp_path / "src.py").write_text("def a():\n return 1\n") + r = Repro("b", "t").extract_symbols_to_new_module( + "src.py", "n.py", symbols=["a", "missing"], header="", order=["a", "missing"] + ) + with pytest.raises(AssertionError): + _apply(r, tmp_path) + + +def test_extract_symbols_to_new_module_drops_relocated_assigns(tmp_path: Path) -> None: + """A module-level constant that moved into the new module's header is deleted from the + source (its copy lives in the authored header); a kept assignment stays.""" + (tmp_path / "src.py").write_text( + "import os\n" + "\n" + "_FLAG = os.cpu_count()\n" + "stay = 1\n" + "\n" + "\n" + "def moved():\n" + " return _FLAG\n" + ) + header = ( + "from __future__ import annotations\n" + "\n" + "import os\n" + "\n" + "_FLAG = os.cpu_count()\n" + ) + r = Repro("b", "t").extract_symbols_to_new_module( + "src.py", + "new.py", + symbols=["moved"], + header=header, + order=["moved"], + drop_assigns=["_FLAG"], + ) + _apply(r, tmp_path) + src_out = (tmp_path / "src.py").read_text() + assert "_FLAG = os.cpu_count()" not in src_out + assert "stay = 1" in src_out + assert "_FLAG = os.cpu_count()" in (tmp_path / "new.py").read_text() + + +def test_extract_symbols_to_new_module_asserts_unknown_drop_assign( + tmp_path: Path, +) -> None: + """A drop_assigns name that is not assigned at module level in the source raises.""" + (tmp_path / "src.py").write_text("X = 1\n\n\ndef m():\n return X\n") + r = Repro("b", "t").extract_symbols_to_new_module( + "src.py", "n.py", symbols=["m"], header="", order=["m"], drop_assigns=["Y"] + ) + with pytest.raises(AssertionError): + _apply(r, tmp_path) + + +# --- extract_function ---------------------------------------------------------- + + +def test_extract_symbols_to_new_module_joins_blocks_with_two_blank_lines( + tmp_path: Path, +) -> None: + """Relocated blocks are joined with exactly two blank lines (the formatter's spacing).""" + (tmp_path / "src.py").write_text( + "def moved_a():\n return 1\n\n\n\n\ndef moved_b():\n return 2\n" + ) + r = Repro("b", "t").extract_symbols_to_new_module( + "src.py", + "new.py", + symbols=["moved_a", "moved_b"], + header="", + order=["moved_a", "moved_b"], + ) + _apply(r, tmp_path) + assert (tmp_path / "new.py").read_text() == ( + "def moved_a():\n return 1\n\n\ndef moved_b():\n return 2\n" + ) + + +def test_extract_symbols_to_new_module_leaves_a_comment_above_a_moved_def( + tmp_path: Path, +) -> None: + """A section comment directly above a moved def stays behind in the source.""" + (tmp_path / "src.py").write_text( + "x = 1\n\n\n# --- movers ---\ndef moved():\n return 2\n" + ) + r = Repro("b", "t").extract_symbols_to_new_module( + "src.py", "new.py", symbols=["moved"], header="", order=["moved"] + ) + _apply(r, tmp_path) + assert (tmp_path / "src.py").read_text() == "x = 1\n\n\n# --- movers ---\n" + assert (tmp_path / "new.py").read_text() == "def moved():\n return 2\n" + + +def test_extract_symbols_drop_assigns_preserves_other_targets_of_chained_assign( + tmp_path: Path, +) -> None: + """Dropping A from `A = B = 1` must not delete B's binding from the source.""" + (tmp_path / "src.py").write_text("A = B = 1\n\n\ndef moved():\n return A\n") + r = Repro("b", "t").extract_symbols_to_new_module( + "src.py", + "new.py", + symbols=["moved"], + header="A = 1\n", + order=["moved"], + drop_assigns=["A"], + ) + _apply(r, tmp_path) + assert "B" in (tmp_path / "src.py").read_text() diff --git a/.claude/skills/mechanical-refactor-verify/scripts/tests/reproduction_utils/test_extract_to_new_module.py b/.claude/skills/mechanical-refactor-verify/scripts/tests/reproduction_utils/test_extract_to_new_module.py new file mode 100644 index 000000000..0c1540042 --- /dev/null +++ b/.claude/skills/mechanical-refactor-verify/scripts/tests/reproduction_utils/test_extract_to_new_module.py @@ -0,0 +1,126 @@ +import subprocess +import sys +from pathlib import Path + +import pytest + +sys.path.insert(0, str(Path(__file__).resolve().parents[2])) + +import mechanical_refactor_reproduction_utils as rr +from mechanical_refactor_reproduction_utils import ( + Repro, + _def_span, + _find_class, + _find_def, + _replace_span, + _slice_span, + dedent, + exec_command, + git_add_and_commit, + verify_mechanical_refactor, +) +from reproduction_testlib import _apply, _commit, _git, _write # noqa: F401 + +# --- extract_to_new_module ----------------------------------------------------- + + +def test_extract_to_new_module_cuts_trailing_block(tmp_path: Path) -> None: + """Cuts the trailing scaffolding+def block into a new file, prepending the future import.""" + (tmp_path / "src.py").write_text( + "class M:\n" + " def keep(self):\n" + " return 1\n" + "\n" + "\n" + "import logging\n" + "\n" + "logger = logging.getLogger(__name__)\n" + "\n" + "\n" + "def foo(x):\n" + " return x + 1\n" + ) + r = Repro("b", "t").extract_to_new_module( + "src.py", "new.py", symbols=["foo"], future_import=True + ) + _apply(r, tmp_path) + assert (tmp_path / "src.py").read_text() == ( + "class M:\n def keep(self):\n return 1\n\n\n" + ) + assert (tmp_path / "new.py").read_text() == ( + "from __future__ import annotations\n" + "import logging\n" + "\n" + "logger = logging.getLogger(__name__)\n" + "\n" + "\n" + "def foo(x):\n" + " return x + 1\n" + ) + + +def test_extract_to_new_module_carries_a_trailing_class(tmp_path: Path) -> None: + """A class in the staged tail (not just a def) travels with the cut block.""" + (tmp_path / "src.py").write_text( + "class M:\n" + " pass\n" + "\n" + "\n" + "from dataclasses import dataclass\n" + "\n" + "\n" + "@dataclass\n" + "class Cfg:\n" + " x: int\n" + "\n" + "\n" + "def foo():\n" + " return Cfg(1)\n" + ) + r = Repro("b", "t").extract_to_new_module( + "src.py", "new.py", symbols=["Cfg", "foo"], future_import=False + ) + _apply(r, tmp_path) + assert (tmp_path / "src.py").read_text() == "class M:\n pass\n\n\n" + assert "class Cfg:" in (tmp_path / "new.py").read_text() + assert "def foo():" in (tmp_path / "new.py").read_text() + + +# --- extract_symbols_to_new_module --------------------------------------------- + + +# --- adversarial audit: module extraction ---------------------------------------- + + +def test_extract_to_new_module_asserts_when_symbol_not_in_the_tail( + tmp_path: Path, +) -> None: + """A wanted symbol above a non-scaffolding statement is not in the tail and raises.""" + (tmp_path / "src.py").write_text( + "def wanted():\n return 1\n\n\nprint('side effect')\n" + ) + r = Repro("b", "t").extract_to_new_module("src.py", "n.py", symbols=["wanted"]) + with pytest.raises(AssertionError): + _apply(r, tmp_path) + + +def test_extract_to_new_module_refuses_a_trailing_main_guard(tmp_path: Path) -> None: + """A trailing __main__ guard is executable code, not scaffolding: the tail cut raises.""" + (tmp_path / "src.py").write_text( + "class Keep:\n" + " pass\n" + "\n" + "\n" + "def foo():\n" + " return 1\n" + "\n" + "\n" + 'if __name__ == "__main__":\n' + " foo()\n" + ) + r = Repro("b", "t").extract_to_new_module( + "src.py", "new.py", symbols=["foo"], future_import=False + ) + with pytest.raises(AssertionError): + _apply(r, tmp_path) + assert "__main__" in (tmp_path / "src.py").read_text() diff --git a/.claude/skills/mechanical-refactor-verify/scripts/tests/reproduction_utils/test_infra_helpers.py b/.claude/skills/mechanical-refactor-verify/scripts/tests/reproduction_utils/test_infra_helpers.py new file mode 100644 index 000000000..d433cc384 --- /dev/null +++ b/.claude/skills/mechanical-refactor-verify/scripts/tests/reproduction_utils/test_infra_helpers.py @@ -0,0 +1,153 @@ +import subprocess +import sys +from pathlib import Path + +import pytest + +sys.path.insert(0, str(Path(__file__).resolve().parents[2])) + +import mechanical_refactor_reproduction_utils as rr +from mechanical_refactor_reproduction_utils import ( + Repro, + _def_span, + _find_class, + _find_def, + _replace_span, + _slice_span, + dedent, + exec_command, + git_add_and_commit, + verify_mechanical_refactor, +) +from reproduction_testlib import _apply, _commit, _git, _write # noqa: F401 + +# --- exec_command -------------------------------------------------------------- + + +def test_exec_command_returns_stripped_stdout_on_success() -> None: + """A successful command returns its stdout with surrounding whitespace stripped.""" + assert exec_command("echo hello") == "hello" + + +def test_exec_command_respects_cwd(tmp_path: Path) -> None: + """The command runs in the supplied working directory.""" + sub = tmp_path / "workdir" + sub.mkdir() + assert exec_command("pwd", cwd=str(sub)) == str(sub.resolve()) + + +def test_exec_command_check_true_raises_on_failure() -> None: + """With check=True a non-zero exit status raises RuntimeError with the command.""" + with pytest.raises(RuntimeError, match="exit 7"): + exec_command("exit 7", check=True) + + +def test_exec_command_check_false_returns_stdout_without_exiting() -> None: + """With check=False a failing command returns its stdout and does not exit.""" + assert exec_command("echo partial; exit 3", check=False) == "partial" + + +# --- git_add_and_commit -------------------------------------------------------- + + +# --- git_add_and_commit -------------------------------------------------------- + + +def test_git_add_and_commit_stages_and_commits(repo: Path) -> None: + """It stages every change in the cwd and records a commit with the message.""" + _write(repo, **{"file.txt": "content\n"}) + git_add_and_commit("add file", cwd=str(repo)) + assert _git(repo, "log", "-1", "--format=%s") == "add file" + assert _git(repo, "status", "--porcelain") == "" + + +@pytest.mark.parametrize( + "message", + [ + "subject with spaces", + "has 'single' and \"double\" quotes", + "shell $HOME && rm -rf / ; metacharacters", + "trailing parens (a, b) and pipe | semicolon ;", + ], +) +def test_git_add_and_commit_message_round_trips_with_metacharacters( + repo: Path, message: str +) -> None: + """Messages with shell metacharacters are quoted safely and survive verbatim.""" + _write(repo, **{"file.txt": "content\n"}) + git_add_and_commit(message, cwd=str(repo)) + assert _git(repo, "log", "-1", "--format=%B") == message + + +# --- dedent -------------------------------------------------------------------- + + +# --- dedent -------------------------------------------------------------------- + + +def test_dedent_with_zero_leaves_text_unchanged() -> None: + """Dedenting by zero spaces returns the text untouched.""" + text = " indented\nplain\n" + assert dedent(text, 0) == text + + +def test_dedent_removes_exactly_n_leading_spaces() -> None: + """Exactly n leading spaces are removed from each qualifying line.""" + assert dedent(" four\n eight\n", 4) == "four\n eight\n" + + +def test_dedent_leaves_lines_with_fewer_than_n_spaces_unchanged() -> None: + """A line with fewer than n leading spaces is not modified at all.""" + assert dedent(" four\n two\nzero\n", 4) == "four\n two\nzero\n" + + +def test_dedent_does_not_strip_tabs() -> None: + """Tab characters are never treated as the spaces dedent removes.""" + assert dedent("\t\ttabbed\n", 2) == "\t\ttabbed\n" + + +def test_dedent_preserves_blank_lines_and_trailing_newline() -> None: + """Blank lines and a final newline are preserved across line boundaries.""" + assert dedent(" a\n\n b\n", 4) == "a\n\nb\n" + + +def test_dedent_preserves_absence_of_trailing_newline() -> None: + """A text without a trailing newline keeps it absent after dedenting.""" + assert dedent(" a\n b", 4) == "a\nb" + + +# --- span / call helpers ------------------------------------------------------- + + +# --- span / call helpers ------------------------------------------------------- + + +def test_replace_span_single_line() -> None: + """A span within one line is replaced in place.""" + assert _replace_span("ab cd ef\n", 1, 3, 1, 5, "XY") == "ab XY ef\n" + + +def test_replace_span_across_lines() -> None: + """A span crossing lines collapses to the replacement between the kept prefix/suffix.""" + text = "a = foo(\n x,\n) + 1\n" + assert _replace_span(text, 1, 4, 3, 1, "bar()") == "a = bar() + 1\n" + + +def test_slice_span_returns_the_overwritten_text() -> None: + """_slice_span returns exactly the region _replace_span would overwrite.""" + text = "a = foo(\n x,\n) + 1\n" + assert _slice_span(text, 1, 4, 3, 1) == "foo(\n x,\n)" + + +def test_find_def_span_includes_decorators() -> None: + """A def's span starts at its first decorator and ends at its last body line.""" + src = "class C:\n @staticmethod\n def foo(self):\n return 1\n" + node = _find_def(rr.ast.parse(src), "foo") + assert node is not None and _def_span(node) == (2, 4) + + +def test_find_class_returns_named_class_or_none() -> None: + """_find_class locates a class by name and returns None when absent.""" + tree = rr.ast.parse("class A:\n pass\nclass B:\n pass\n") + assert _find_class(tree, "B").name == "B" + assert _find_class(tree, "Z") is None diff --git a/.claude/skills/mechanical-refactor-verify/scripts/tests/reproduction_utils/test_move_symbol.py b/.claude/skills/mechanical-refactor-verify/scripts/tests/reproduction_utils/test_move_symbol.py new file mode 100644 index 000000000..cf6863ef7 --- /dev/null +++ b/.claude/skills/mechanical-refactor-verify/scripts/tests/reproduction_utils/test_move_symbol.py @@ -0,0 +1,360 @@ +import subprocess +import sys +from pathlib import Path + +import pytest + +sys.path.insert(0, str(Path(__file__).resolve().parents[2])) + +import mechanical_refactor_reproduction_utils as rr +from mechanical_refactor_reproduction_utils import ( + Repro, + _def_span, + _find_class, + _find_def, + _replace_span, + _slice_span, + dedent, + exec_command, + git_add_and_commit, + verify_mechanical_refactor, +) +from reproduction_testlib import _apply, _commit, _git, _write # noqa: F401 + + +def test_move_symbol_drops_self_annotation_into_class(tmp_path: Path) -> None: + """Moving a `def foo(self: Target)` into Target drops the now-redundant annotation.""" + (tmp_path / "src.py").write_text( + "class M:\n" + " @staticmethod\n" + " def foo(self: Target, x):\n" + " return self.y + x\n" + ) + (tmp_path / "dst.py").write_text( + "class Target:\n def keep(self):\n return 1\n" + ) + r = Repro("b", "t").move_symbol( + "foo", + src="src.py", + dst="dst.py", + into_class="Target", + dedent=0, + drop_self_annotation=True, + ) + _apply(r, tmp_path) + text = (tmp_path / "dst.py").read_text() + assert "def foo(self, x):" in text + assert "self: Target" not in text + + +# --- adversarial audit: import primitives ---------------------------------------- + + +# --- move_symbol --------------------------------------------------------------- + + +def test_move_symbol_into_class_drops_decorator_and_appends(tmp_path: Path) -> None: + """The def leaves the source, its @staticmethod is dropped, and it lands at the end of + the destination class with its body verbatim.""" + (tmp_path / "src.py").write_text( + "class Old:\n" + " @staticmethod\n" + " def foo(x):\n" + " return x + 1\n" + "\n" + " def keep(self):\n" + " return 0\n" + ) + (tmp_path / "dst.py").write_text( + "class New:\n def existing(self):\n return 1\n" + ) + r = Repro("b", "t").move_symbol("foo", src="src.py", dst="dst.py", into_class="New") + _apply(r, tmp_path) + src_out = (tmp_path / "src.py").read_text() + dst_out = (tmp_path / "dst.py").read_text() + assert "def foo" not in src_out and "def keep" in src_out + assert "@staticmethod" not in dst_out + assert dst_out.index("def existing") < dst_out.index("def foo") + assert " return x + 1\n" in dst_out + + +def test_move_symbol_to_module_level_with_dedent(tmp_path: Path) -> None: + """With into_class=None and a dedent, the def lands at module level, dedented.""" + (tmp_path / "src.py").write_text( + "class Old:\n @staticmethod\n def helper(x):\n return x * 2\n" + ) + (tmp_path / "dst.py").write_text("import os\n") + r = Repro("b", "t").move_symbol( + "helper", src="src.py", dst="dst.py", into_class=None, dedent=4 + ) + _apply(r, tmp_path) + assert "def helper(x):\n return x * 2\n" in (tmp_path / "dst.py").read_text() + assert "def helper" not in (tmp_path / "src.py").read_text() + + +def test_move_symbol_before_inserts_above_named_sibling(tmp_path: Path) -> None: + """With before=, the relocated def lands immediately above that sibling, not at the end.""" + (tmp_path / "src.py").write_text( + "class Old:\n @staticmethod\n def moved(self):\n return 1\n" + ) + (tmp_path / "dst.py").write_text( + "class New:\n" + " def first(self):\n return 0\n" + "\n" + " def last(self):\n return 2\n" + ) + r = Repro("b", "t").move_symbol( + "moved", src="src.py", dst="dst.py", into_class="New", before="last" + ) + _apply(r, tmp_path) + dst_out = (tmp_path / "dst.py").read_text() + assert ( + dst_out.index("def first") + < dst_out.index("def moved") + < dst_out.index("def last") + ) + + +# --- adversarial audit: move_symbol edge cases ----------------------------------- + + +def test_move_symbol_moves_an_async_def_verbatim(tmp_path: Path) -> None: + """An async def relocates with its `async` keyword and body byte-identical.""" + (tmp_path / "src.py").write_text( + "class Old:\n" + " async def foo(self):\n" + " return 1\n" + "\n" + " def keep(self):\n" + " return 0\n" + ) + (tmp_path / "dst.py").write_text("class New:\n def e(self):\n return 1\n") + r = Repro("b", "t").move_symbol("foo", src="src.py", dst="dst.py", into_class="New") + _apply(r, tmp_path) + assert (tmp_path / "src.py").read_text() == ( + "class Old:\n\n def keep(self):\n return 0\n" + ) + assert (tmp_path / "dst.py").read_text() == ( + "class New:\n" + " def e(self):\n" + " return 1\n" + "\n" + " async def foo(self):\n" + " return 1\n" + ) + + +def test_move_symbol_moves_a_def_within_the_same_file(tmp_path: Path) -> None: + """With src == dst the def is cut and re-inserted above its sibling in one file.""" + (tmp_path / "m.py").write_text( + "def a():\n return 1\n\n\ndef b():\n return 2\n" + ) + r = Repro("b", "t").move_symbol( + "b", src="m.py", dst="m.py", into_class=None, before="a" + ) + _apply(r, tmp_path) + assert (tmp_path / "m.py").read_text() == ( + "def b():\n return 2\n\ndef a():\n return 1\n\n\n" + ) + + +def test_move_symbol_prefers_a_module_level_def_over_an_earlier_class_method( + tmp_path: Path, +) -> None: + """When a class method and a module-level def share a name, the module-level def moves.""" + (tmp_path / "src.py").write_text( + "class C:\n" + " def foo(self):\n" + " return 'method'\n" + "\n" + "\n" + "def foo():\n" + " return 'module'\n" + ) + (tmp_path / "dst.py").write_text("x = 1\n") + r = Repro("b", "t").move_symbol("foo", src="src.py", dst="dst.py", into_class=None) + _apply(r, tmp_path) + assert (tmp_path / "src.py").read_text() == ( + "class C:\n def foo(self):\n return 'method'\n\n\n" + ) + assert (tmp_path / "dst.py").read_text() == ( + "x = 1\n\ndef foo():\n return 'module'\n" + ) + + +def test_move_symbol_keeps_a_real_decorator_while_dropping_classmethod( + tmp_path: Path, +) -> None: + """@classmethod is shed on the move but any other decorator travels verbatim.""" + (tmp_path / "src.py").write_text( + "import functools\n" + "\n" + "\n" + "class Old:\n" + " @classmethod\n" + " @functools.lru_cache(maxsize=None)\n" + " def foo(cls, x):\n" + " return x + 1\n" + ) + (tmp_path / "dst.py").write_text("def z():\n return 0\n") + r = Repro("b", "t").move_symbol( + "foo", src="src.py", dst="dst.py", into_class=None, dedent=4 + ) + _apply(r, tmp_path) + assert (tmp_path / "dst.py").read_text() == ( + "def z():\n" + " return 0\n" + "\n" + "@functools.lru_cache(maxsize=None)\n" + "def foo(cls, x):\n" + " return x + 1\n" + ) + + +def test_move_symbol_leaves_a_comment_above_the_def_in_the_source( + tmp_path: Path, +) -> None: + """A comment above the def is not part of its span, so it stays behind in the source.""" + (tmp_path / "src.py").write_text( + "# explains foo\ndef foo():\n return 1\n\n\ndef keep():\n return 2\n" + ) + (tmp_path / "dst.py").write_text("x = 1\n") + r = Repro("b", "t").move_symbol("foo", src="src.py", dst="dst.py", into_class=None) + _apply(r, tmp_path) + assert (tmp_path / "src.py").read_text() == ( + "# explains foo\n\n\ndef keep():\n return 2\n" + ) + assert (tmp_path / "dst.py").read_text() == "x = 1\n\ndef foo():\n return 1\n" + + +def test_move_symbol_without_trailing_newlines_keeps_moved_bytes( + tmp_path: Path, +) -> None: + """Files lacking a final newline lose no bytes of the moved def or the remainder.""" + (tmp_path / "src.py").write_text( + "def keep():\n return 0\n\n\ndef foo():\n return 1" + ) + (tmp_path / "dst.py").write_text("x = 1") + r = Repro("b", "t").move_symbol("foo", src="src.py", dst="dst.py", into_class=None) + _apply(r, tmp_path) + assert (tmp_path / "src.py").read_text() == "def keep():\n return 0\n\n\n" + assert (tmp_path / "dst.py").read_text() == "x = 1\ndef foo():\n return 1" + + +def test_move_symbol_dedent_leaves_string_literal_interior_lines( + tmp_path: Path, +) -> None: + """Dedent only strips lines with exactly n leading spaces, so string interiors survive.""" + (tmp_path / "src.py").write_text( + "class Old:\n" + " class Deep:\n" + " def foo(self):\n" + " s = '''raw\n" + " partial\n" + "'''\n" + " return s\n" + ) + (tmp_path / "dst.py").write_text("import os\n") + r = Repro("b", "t").move_symbol( + "foo", src="src.py", dst="dst.py", into_class=None, dedent=8 + ) + _apply(r, tmp_path) + assert (tmp_path / "dst.py").read_text() == ( + "import os\n" + "\n" + "def foo(self):\n" + " s = '''raw\n" + " partial\n" + "'''\n" + " return s\n" + ) + + +def test_move_symbol_asserts_when_destination_class_missing(tmp_path: Path) -> None: + """Naming an into_class absent from the destination fails loudly.""" + (tmp_path / "src.py").write_text("def foo():\n return 1\n") + (tmp_path / "dst.py").write_text("x = 1\n") + r = Repro("b", "t").move_symbol( + "foo", src="src.py", dst="dst.py", into_class="Nope" + ) + with pytest.raises(AssertionError): + _apply(r, tmp_path) + + +def test_move_symbol_preserves_staticmethod_inside_moved_body(tmp_path: Path) -> None: + """A @staticmethod on a nested def inside the moved body must survive the move.""" + (tmp_path / "src.py").write_text( + "class Old:\n" + " @staticmethod\n" + " def foo(x):\n" + " class Inner:\n" + " @staticmethod\n" + " def helper(y):\n" + " return y\n" + " return Inner.helper(x)\n" + ) + (tmp_path / "dst.py").write_text( + "class New:\n def keep(self):\n return 0\n" + ) + r = Repro("b", "t").move_symbol("foo", src="src.py", dst="dst.py", into_class="New") + _apply(r, tmp_path) + dst_out = (tmp_path / "dst.py").read_text() + assert " @staticmethod\n def helper(y):\n" in dst_out + + +def test_move_symbol_rejects_ambiguous_duplicate_names(tmp_path: Path) -> None: + """Two same-named defs at equal depth must raise instead of silently picking one.""" + (tmp_path / "src.py").write_text( + "class A:\n" + " def foo(self):\n" + " return 'A'\n" + "\n" + "class B:\n" + " def foo(self):\n" + " return 'B'\n" + ) + (tmp_path / "dst.py").write_text( + "class New:\n def keep(self):\n return 0\n" + ) + r = Repro("b", "t").move_symbol("foo", src="src.py", dst="dst.py", into_class="New") + with pytest.raises(AssertionError): + _apply(r, tmp_path) + + +def test_move_symbol_asserts_when_before_sibling_missing(tmp_path: Path) -> None: + """A before= anchor absent from the destination must raise, not fall back to append.""" + (tmp_path / "src.py").write_text("def moved():\n return 1\n") + (tmp_path / "dst.py").write_text("def z():\n return 0\n") + r = Repro("b", "t").move_symbol( + "moved", src="src.py", dst="dst.py", into_class=None, before="NO_SUCH_DEF" + ) + with pytest.raises(AssertionError): + _apply(r, tmp_path) + + +def test_move_symbol_preserves_crlf_line_endings(tmp_path: Path) -> None: + """Moving a def in a CRLF file must keep every line ending CRLF.""" + (tmp_path / "src.py").write_bytes( + b"class Old:\r\n def foo(self):\r\n return 1\r\n" + ) + (tmp_path / "dst.py").write_bytes( + b"class New:\r\n def keep(self):\r\n return 0\r\n" + ) + r = Repro("b", "t").move_symbol("foo", src="src.py", dst="dst.py", into_class="New") + _apply(r, tmp_path) + dst_bytes = (tmp_path / "dst.py").read_bytes() + assert dst_bytes.count(b"\n") == dst_bytes.count(b"\r\n") + + +def test_move_symbol_negative_dedent_indents_into_the_class(tmp_path: Path) -> None: + """Moving a module-level def into a class with dedent=-4 must indent it as a method.""" + (tmp_path / "src.py").write_text("def helper(x):\n return x\n") + (tmp_path / "dst.py").write_text("class New:\n def e(self):\n return 0\n") + r = Repro("b", "t").move_symbol( + "helper", src="src.py", dst="dst.py", into_class="New", dedent=-4 + ) + _apply(r, tmp_path) + assert " def helper(x):\n return x\n" in (tmp_path / "dst.py").read_text() + + +# --- adversarial audit: leave_delegate stubs ------------------------------------- diff --git a/.claude/skills/mechanical-refactor-verify/scripts/tests/reproduction_utils/test_move_symbol_delegate.py b/.claude/skills/mechanical-refactor-verify/scripts/tests/reproduction_utils/test_move_symbol_delegate.py new file mode 100644 index 000000000..b9e20da71 --- /dev/null +++ b/.claude/skills/mechanical-refactor-verify/scripts/tests/reproduction_utils/test_move_symbol_delegate.py @@ -0,0 +1,195 @@ +import subprocess +import sys +from pathlib import Path + +import pytest + +sys.path.insert(0, str(Path(__file__).resolve().parents[2])) + +import mechanical_refactor_reproduction_utils as rr +from mechanical_refactor_reproduction_utils import ( + Repro, + _def_span, + _find_class, + _find_def, + _replace_span, + _slice_span, + dedent, + exec_command, + git_add_and_commit, + verify_mechanical_refactor, +) +from reproduction_testlib import _apply, _commit, _git, _write # noqa: F401 + + +def test_move_symbol_leave_delegate_keeps_forwarding_stub(tmp_path: Path) -> None: + """With leave_delegate, the source keeps a forwarding stub through the named field and the + destination gets the full method body.""" + (tmp_path / "src.py").write_text( + "class Mixin:\n" + " def compute(self, n: int) -> int:\n" + " return n + self.cfg.base\n" + ) + (tmp_path / "dst.py").write_text( + "class Cfg:\n def existing(self):\n return 0\n" + ) + r = Repro("b", "t").move_symbol( + "compute", + src="src.py", + dst="dst.py", + into_class="Cfg", + leave_delegate="cfg", + ) + _apply(r, tmp_path) + src_out = (tmp_path / "src.py").read_text() + dst_out = (tmp_path / "dst.py").read_text() + assert "def compute(self, n: int) -> int:" in src_out + assert "return self.cfg.compute(n)" in src_out + assert "return n + self.cfg.base" not in src_out + assert "return n + self.cfg.base" in dst_out + + +def test_move_symbol_leave_delegate_does_not_absorb_leading_comments( + tmp_path: Path, +) -> None: + """A leading comment before the first statement is not an AST node, so it must not be + pulled into the forwarding stub -- the delegate is just the header plus the return. + """ + (tmp_path / "src.py").write_text( + "class Mixin:\n" + " def compute(self, n: int) -> int:\n" + " # explain the maths\n" + " # second comment line\n" + " return n + self.cfg.base\n" + ) + (tmp_path / "dst.py").write_text( + "class Cfg:\n def existing(self):\n return 0\n" + ) + r = Repro("b", "t").move_symbol( + "compute", src="src.py", dst="dst.py", into_class="Cfg", leave_delegate="cfg" + ) + _apply(r, tmp_path) + src_out = (tmp_path / "src.py").read_text() + dst_out = (tmp_path / "dst.py").read_text() + assert "# explain the maths" not in src_out + assert ( + src_out == "class Mixin:\n" + " def compute(self, n: int) -> int:\n" + " return self.cfg.compute(n)\n" + ) + assert "# explain the maths" in dst_out + + +# --- adversarial audit: move_symbol edge cases ----------------------------------- + + +# --- adversarial audit: leave_delegate stubs ------------------------------------- + + +def test_move_symbol_leave_delegate_keeps_a_multiline_signature_verbatim( + tmp_path: Path, +) -> None: + """A multi-line header is carried into the stub byte-for-byte via the bracket scan.""" + (tmp_path / "src.py").write_text( + "class Mixin:\n" + " def compute(\n" + " self,\n" + " n: int,\n" + " *,\n" + " scale: float = 1.0,\n" + " ) -> int:\n" + " return int(n * scale) + self.cfg.base\n" + ) + (tmp_path / "dst.py").write_text("class Cfg:\n def e(self):\n return 0\n") + r = Repro("b", "t").move_symbol( + "compute", src="src.py", dst="dst.py", into_class="Cfg", leave_delegate="cfg" + ) + _apply(r, tmp_path) + assert (tmp_path / "src.py").read_text() == ( + "class Mixin:\n" + " def compute(\n" + " self,\n" + " n: int,\n" + " *,\n" + " scale: float = 1.0,\n" + " ) -> int:\n" + " return self.cfg.compute(n, scale=scale)\n" + ) + assert (tmp_path / "dst.py").read_text() == ( + "class Cfg:\n" + " def e(self):\n" + " return 0\n" + "\n" + " def compute(\n" + " self,\n" + " n: int,\n" + " *,\n" + " scale: float = 1.0,\n" + " ) -> int:\n" + " return int(n * scale) + self.cfg.base\n" + ) + + +def test_move_symbol_leave_delegate_forwards_posonly_vararg_kwonly_kwargs( + tmp_path: Path, +) -> None: + """Every parameter kind is forwarded correctly in the delegate's return call.""" + (tmp_path / "src.py").write_text( + "class Mixin:\n" + " def compute(self, a, /, b, *args, c, d=3, **kw):\n" + " return a\n" + ) + (tmp_path / "dst.py").write_text( + "class Cfg:\n def keep(self):\n return 0\n" + ) + r = Repro("b", "t").move_symbol( + "compute", src="src.py", dst="dst.py", into_class="Cfg", leave_delegate="cfg" + ) + _apply(r, tmp_path) + assert (tmp_path / "src.py").read_text() == ( + "class Mixin:\n" + " def compute(self, a, /, b, *args, c, d=3, **kw):\n" + " return self.cfg.compute(a, b, *args, c=c, d=d, **kw)\n" + ) + + +def test_move_symbol_leave_delegate_survives_paren_in_string_default( + tmp_path: Path, +) -> None: + """A string default containing '(' must not break the delegate's header scan.""" + (tmp_path / "src.py").write_text( + "class Mixin:\n" + " def compute(\n" + " self,\n" + ' sep: str = "(",\n' + " n: int = 0,\n" + " ) -> int:\n" + " return n + self.cfg.base\n" + ) + (tmp_path / "dst.py").write_text( + "class Cfg:\n def keep(self):\n return 0\n" + ) + r = Repro("b", "t").move_symbol( + "compute", src="src.py", dst="dst.py", into_class="Cfg", leave_delegate="cfg" + ) + _apply(r, tmp_path) + src_out = (tmp_path / "src.py").read_text() + compile(src_out, "src.py", "exec") + assert "return self.cfg.compute(sep, n)" in src_out + + +def test_move_symbol_async_leave_delegate_awaits_the_forwarded_call( + tmp_path: Path, +) -> None: + """An async method's delegate stub must await the forwarded coroutine.""" + (tmp_path / "src.py").write_text( + "class Mixin:\n" + " async def compute(self, n):\n" + " return n + self.cfg.base\n" + ) + (tmp_path / "dst.py").write_text("class Cfg:\n def e(self):\n return 0\n") + r = Repro("b", "t").move_symbol( + "compute", src="src.py", dst="dst.py", into_class="Cfg", leave_delegate="cfg" + ) + _apply(r, tmp_path) + assert "return await self.cfg.compute(n)" in (tmp_path / "src.py").read_text() diff --git a/.claude/skills/mechanical-refactor-verify/scripts/tests/reproduction_utils/test_remove_import.py b/.claude/skills/mechanical-refactor-verify/scripts/tests/reproduction_utils/test_remove_import.py new file mode 100644 index 000000000..17e89cb2f --- /dev/null +++ b/.claude/skills/mechanical-refactor-verify/scripts/tests/reproduction_utils/test_remove_import.py @@ -0,0 +1,126 @@ +import subprocess +import sys +from pathlib import Path + +import pytest + +sys.path.insert(0, str(Path(__file__).resolve().parents[2])) + +import mechanical_refactor_reproduction_utils as rr +from mechanical_refactor_reproduction_utils import ( + Repro, + _def_span, + _find_class, + _find_def, + _replace_span, + _slice_span, + dedent, + exec_command, + git_add_and_commit, + verify_mechanical_refactor, +) +from reproduction_testlib import _apply, _commit, _git, _write # noqa: F401 + +# --- remove_import ------------------------------------------------------------- + + +def test_remove_import_scoped_leaves_module_level_same_text(tmp_path: Path) -> None: + """Scoped to a function, it removes the local import but not a same-text module-level + one (e.g. a TYPE_CHECKING guard), and drops the import's trailing blank line.""" + (tmp_path / "m.py").write_text( + "from typing import TYPE_CHECKING\n" + "\n" + "if TYPE_CHECKING:\n" + " from pkg.mod import Thing\n" + "\n" + "def caller(self):\n" + " from pkg.mod import Thing\n" + "\n" + " return Thing.go(self.x)\n" + ) + r = Repro("b", "t").remove_import( + "m.py", "from pkg.mod import Thing", in_function="caller" + ) + _apply(r, tmp_path) + out = (tmp_path / "m.py").read_text() + assert out.count("from pkg.mod import Thing") == 1 + assert "if TYPE_CHECKING:\n from pkg.mod import Thing" in out + assert "def caller(self):\n return Thing.go(self.x)\n" in out + + +def test_remove_import_removes_every_occurrence_in_scope(tmp_path: Path) -> None: + """All matching local imports in the function are removed, not just the first.""" + (tmp_path / "m.py").write_text( + "def caller(self):\n" + " from pkg import M\n" + "\n" + " M.a(self.x)\n" + " if cond:\n" + " from pkg import M\n" + "\n" + " M.b(self.y)\n" + ) + r = Repro("b", "t").remove_import("m.py", "from pkg import M", in_function="caller") + _apply(r, tmp_path) + assert "from pkg import M" not in (tmp_path / "m.py").read_text() + + +# --- remove_imported_name ------------------------------------------------------ + + +# --- adversarial audit: import primitives ---------------------------------------- + + +def test_remove_import_unscoped_removes_module_level_import_and_blank( + tmp_path: Path, +) -> None: + """Without in_function the matching module-level import and its trailing blank go.""" + (tmp_path / "m.py").write_text("import os\nfrom pkg import Thing\n\nx = Thing\n") + r = Repro("b", "t").remove_import("m.py", "from pkg import Thing") + _apply(r, tmp_path) + assert (tmp_path / "m.py").read_text() == "import os\nx = Thing\n" + + +def test_remove_import_keeps_a_code_line_directly_after_the_import( + tmp_path: Path, +) -> None: + """Only a blank line after the import is absorbed; a code line stays untouched.""" + (tmp_path / "m.py").write_text("import os\nx = 1\n") + r = Repro("b", "t").remove_import("m.py", "import os") + _apply(r, tmp_path) + assert (tmp_path / "m.py").read_text() == "x = 1\n" + + +def test_remove_import_asserts_when_text_absent(tmp_path: Path) -> None: + """Removing an import text that matches nothing fails loudly.""" + (tmp_path / "m.py").write_text("import os\n") + r = Repro("b", "t").remove_import("m.py", "from pkg import Q") + with pytest.raises(AssertionError): + _apply(r, tmp_path) + + +def test_remove_import_asserts_when_scope_function_missing(tmp_path: Path) -> None: + """Scoping to a function that does not exist fails loudly.""" + (tmp_path / "m.py").write_text("def f():\n import os\n") + r = Repro("b", "t").remove_import("m.py", "import os", in_function="nope") + with pytest.raises(AssertionError): + _apply(r, tmp_path) + + +def test_remove_import_leaves_other_statements_on_a_semicolon_line( + tmp_path: Path, +) -> None: + """Removing 'import os' from a semicolon-joined line must keep 'import sys'.""" + (tmp_path / "m.py").write_text("import os; import sys\nprint(sys.path)\n") + r = Repro("b", "t").remove_import("m.py", "import os") + _apply(r, tmp_path) + out = (tmp_path / "m.py").read_text() + assert "import sys" in out and "print(sys.path)" in out + + +def test_remove_import_does_not_overmatch_a_submodule_import(tmp_path: Path) -> None: + """Removing 'import os' must not also remove 'import os.path'.""" + (tmp_path / "m.py").write_text("import os\nimport os.path\nprint(os.path.sep)\n") + r = Repro("b", "t").remove_import("m.py", "import os") + _apply(r, tmp_path) + assert "import os.path\n" in (tmp_path / "m.py").read_text() diff --git a/.claude/skills/mechanical-refactor-verify/scripts/tests/reproduction_utils/test_remove_imported_name.py b/.claude/skills/mechanical-refactor-verify/scripts/tests/reproduction_utils/test_remove_imported_name.py new file mode 100644 index 000000000..3fe06a681 --- /dev/null +++ b/.claude/skills/mechanical-refactor-verify/scripts/tests/reproduction_utils/test_remove_imported_name.py @@ -0,0 +1,113 @@ +import subprocess +import sys +from pathlib import Path + +import pytest + +sys.path.insert(0, str(Path(__file__).resolve().parents[2])) + +import mechanical_refactor_reproduction_utils as rr +from mechanical_refactor_reproduction_utils import ( + Repro, + _def_span, + _find_class, + _find_def, + _replace_span, + _slice_span, + dedent, + exec_command, + git_add_and_commit, + verify_mechanical_refactor, +) +from reproduction_testlib import _apply, _commit, _git, _write # noqa: F401 + +# --- remove_imported_name ------------------------------------------------------ + + +def test_remove_imported_name_drops_one_name_from_a_multi_name_import( + tmp_path: Path, +) -> None: + """One name is dropped from a `from m import a, b, c`; the others stay on the line.""" + (tmp_path / "m.py").write_text("from pkg import a, moved, b\n\nx = a + b\n") + r = Repro("b", "t").remove_imported_name("m.py", module="pkg", name="moved") + _apply(r, tmp_path) + assert (tmp_path / "m.py").read_text() == "from pkg import a, b\n\nx = a + b\n" + + +def test_remove_imported_name_drops_whole_statement_when_sole_name( + tmp_path: Path, +) -> None: + """Dropping the only name removes the whole `from` statement.""" + (tmp_path / "m.py").write_text("from pkg import moved\nimport os\n\nx = 1\n") + r = Repro("b", "t").remove_imported_name("m.py", module="pkg", name="moved") + _apply(r, tmp_path) + assert (tmp_path / "m.py").read_text() == "import os\n\nx = 1\n" + + +def test_remove_imported_name_drops_a_plain_import_with_module_none( + tmp_path: Path, +) -> None: + """With module=None a plain `import name` statement is removed.""" + (tmp_path / "m.py").write_text("import gc\nimport os\n\nx = 1\n") + r = Repro("b", "t").remove_imported_name("m.py", module=None, name="gc") + _apply(r, tmp_path) + assert (tmp_path / "m.py").read_text() == "import os\n\nx = 1\n" + + +def test_remove_imported_name_matches_an_asname(tmp_path: Path) -> None: + """The alias is matched on both the name and the asname, so `import numpy as np` is found.""" + (tmp_path / "m.py").write_text("import numpy as np\nimport os\n\nx = 1\n") + r = Repro("b", "t").remove_imported_name( + "m.py", module=None, name="numpy", asname="np" + ) + _apply(r, tmp_path) + assert (tmp_path / "m.py").read_text() == "import os\n\nx = 1\n" + + +def test_remove_imported_name_asserts_when_absent(tmp_path: Path) -> None: + """Removing a name that is not imported raises, so a wrong recipe fails loudly.""" + (tmp_path / "m.py").write_text("from pkg import a, b\n") + r = Repro("b", "t").remove_imported_name("m.py", module="pkg", name="missing") + with pytest.raises(AssertionError): + _apply(r, tmp_path) + + +# --- add_import ---------------------------------------------------------------- + + +def test_remove_imported_name_collapses_a_multiline_import_to_one_line( + tmp_path: Path, +) -> None: + """Pruning a name from a parenthesized import rebuilds it as a single sorted-later line.""" + (tmp_path / "m.py").write_text( + "from pkg import (\n a,\n moved,\n b,\n)\n\nx = a + b\n" + ) + r = Repro("b", "t").remove_imported_name("m.py", module="pkg", name="moved") + _apply(r, tmp_path) + assert (tmp_path / "m.py").read_text() == "from pkg import a, b\n\nx = a + b\n" + + +def test_remove_imported_name_matches_a_relative_module(tmp_path: Path) -> None: + """A relative `from .pkg import` is matched via its level dots.""" + (tmp_path / "m.py").write_text("from .pkg import a, moved\n\nx = a\n") + r = Repro("b", "t").remove_imported_name("m.py", module=".pkg", name="moved") + _apply(r, tmp_path) + assert (tmp_path / "m.py").read_text() == "from .pkg import a\n\nx = a\n" + + +def test_remove_imported_name_preserves_comments_in_a_multiline_import( + tmp_path: Path, +) -> None: + """Comments on surviving lines of a pruned parenthesized import must not vanish.""" + (tmp_path / "m.py").write_text( + "from pkg import (\n" + " a, # used by frobnicator\n" + " moved,\n" + " b,\n" + ")\n" + "\n" + "x = a + b\n" + ) + r = Repro("b", "t").remove_imported_name("m.py", module="pkg", name="moved") + _apply(r, tmp_path) + assert "# used by frobnicator" in (tmp_path / "m.py").read_text() diff --git a/.claude/skills/mechanical-refactor-verify/scripts/tests/reproduction_utils/test_repath_import.py b/.claude/skills/mechanical-refactor-verify/scripts/tests/reproduction_utils/test_repath_import.py new file mode 100644 index 000000000..c248bce7a --- /dev/null +++ b/.claude/skills/mechanical-refactor-verify/scripts/tests/reproduction_utils/test_repath_import.py @@ -0,0 +1,94 @@ +import subprocess +import sys +from pathlib import Path + +import pytest + +sys.path.insert(0, str(Path(__file__).resolve().parents[2])) + +import mechanical_refactor_reproduction_utils as rr +from mechanical_refactor_reproduction_utils import ( + Repro, + _def_span, + _find_class, + _find_def, + _replace_span, + _slice_span, + dedent, + exec_command, + git_add_and_commit, + verify_mechanical_refactor, +) +from reproduction_testlib import _apply, _commit, _git, _write # noqa: F401 + +# --- repath_import / add_typechecking_import ----------------------------------- + + +def test_repath_import_rewrites_nested_import(tmp_path: Path) -> None: + """A function-scoped import is repathed in place; the bare call is untouched.""" + (tmp_path / "c.py").write_text( + "class K:\n" + " def run(self):\n" + " from old.mod import foo\n" + "\n" + " return foo(1)\n" + ) + r = Repro("b", "t").repath_import( + "c.py", old_module="old.mod", new_module="new.mod", name="foo" + ) + _apply(r, tmp_path) + assert (tmp_path / "c.py").read_text() == ( + "class K:\n" + " def run(self):\n" + " from new.mod import foo\n" + "\n" + " return foo(1)\n" + ) + + +def test_repath_import_leaves_a_module_level_import(tmp_path: Path) -> None: + """Only nested imports are repathed; a module-level import is left to the sorter.""" + (tmp_path / "c.py").write_text("from old.mod import foo\n\n\nx = foo(1)\n") + r = Repro("b", "t").repath_import( + "c.py", old_module="old.mod", new_module="new.mod", name="foo" + ) + with pytest.raises(AssertionError): + _apply(r, tmp_path) + + +def test_repath_import_repaths_a_multiline_aliased_nested_import( + tmp_path: Path, +) -> None: + """A nested multi-line from-import with an alias is repathed on its first line.""" + (tmp_path / "c.py").write_text( + "def run():\n" + " from old.mod import (\n" + " foo as f,\n" + " )\n" + "\n" + " return f(1)\n" + ) + r = Repro("b", "t").repath_import( + "c.py", old_module="old.mod", new_module="new.mod", name="foo" + ) + _apply(r, tmp_path) + assert (tmp_path / "c.py").read_text() == ( + "def run():\n" + " from new.mod import (\n" + " foo as f,\n" + " )\n" + "\n" + " return f(1)\n" + ) + + +def test_repath_import_rewrites_a_relative_nested_import(tmp_path: Path) -> None: + """A nested `from .mod import` matched by module name must actually be repathed.""" + (tmp_path / "c.py").write_text( + "def run():\n from .mod import foo\n\n return foo(1)\n" + ) + r = Repro("b", "t").repath_import( + "c.py", old_module="mod", new_module="pkg.mod", name="foo" + ) + _apply(r, tmp_path) + assert "from pkg.mod import foo" in (tmp_path / "c.py").read_text() diff --git a/.claude/skills/mechanical-refactor-verify/scripts/tests/reproduction_utils/test_verify_and_run.py b/.claude/skills/mechanical-refactor-verify/scripts/tests/reproduction_utils/test_verify_and_run.py new file mode 100644 index 000000000..c458cadc5 --- /dev/null +++ b/.claude/skills/mechanical-refactor-verify/scripts/tests/reproduction_utils/test_verify_and_run.py @@ -0,0 +1,180 @@ +import subprocess +import sys +from pathlib import Path + +import pytest + +sys.path.insert(0, str(Path(__file__).resolve().parents[2])) + +import mechanical_refactor_reproduction_utils as rr +from mechanical_refactor_reproduction_utils import ( + Repro, + _def_span, + _find_class, + _find_def, + _replace_span, + _slice_span, + dedent, + exec_command, + git_add_and_commit, + verify_mechanical_refactor, +) +from reproduction_testlib import _apply, _commit, _git, _write # noqa: F401 + +# --- verify_mechanical_refactor ------------------------------------------------ + + +def _silence_precommit(monkeypatch) -> None: + real = rr.exec_command + + def fake(cmd: str, cwd=None, check=True): + if cmd.startswith("pre-commit"): + return "" + return real(cmd, cwd=cwd, check=check) + + monkeypatch.setattr(rr, "exec_command", fake) + + +def test_reproduce_passes_when_transform_matches_target( + repo: Path, tmp_path: Path, monkeypatch, capsys +) -> None: + """A transform that recreates the target tree reports PASS and does not exit.""" + _write(repo, **{"src.py": "line1\nline2\nline3\n"}) + base = _commit(repo, "base") + _write(repo, **{"src.py": None, "a.py": "line1\nline2\n", "b.py": "line3\n"}) + target = _commit(repo, "split") + + def transform(root: Path) -> None: + lines = (root / "src.py").read_text().splitlines(keepends=True) + (root / "a.py").write_text("".join(lines[0:2])) + (root / "b.py").write_text("".join(lines[2:3])) + (root / "src.py").unlink() + rr.git_add_and_commit("split", cwd=str(root)) + + monkeypatch.chdir(repo) + monkeypatch.setattr(rr.tempfile, "mkdtemp", lambda prefix="": str(tmp_path / "wt")) + _silence_precommit(monkeypatch) + + verify_mechanical_refactor(base, target, transform) + assert "PASS" in capsys.readouterr().out + + +def test_reproduce_exits_when_transform_diverges( + repo: Path, tmp_path: Path, monkeypatch +) -> None: + """A transform that produces a different tree fails with a non-zero exit.""" + _write(repo, **{"src.py": "line1\nline2\nline3\n"}) + base = _commit(repo, "base") + _write(repo, **{"src.py": None, "a.py": "line1\nline2\n", "b.py": "line3\n"}) + target = _commit(repo, "split") + + def wrong_transform(root: Path) -> None: + (root / "a.py").write_text("WRONG\n") + (root / "b.py").write_text("line3\n") + (root / "src.py").unlink() + rr.git_add_and_commit("split", cwd=str(root)) + + monkeypatch.chdir(repo) + monkeypatch.setattr(rr.tempfile, "mkdtemp", lambda prefix="": str(tmp_path / "wt")) + _silence_precommit(monkeypatch) + + with pytest.raises(SystemExit): + verify_mechanical_refactor(base, target, wrong_transform) + + +def test_reproduce_creates_verify_branch_on_pass( + repo: Path, tmp_path: Path, monkeypatch, capsys +) -> None: + """A PASS run leaves a verify-mechanical- branch in the repo.""" + _write(repo, **{"src.py": "line1\nline2\nline3\n"}) + base = _commit(repo, "base") + _write(repo, **{"src.py": None, "a.py": "line1\nline2\n", "b.py": "line3\n"}) + target = _commit(repo, "split") + + def transform(root: Path) -> None: + lines = (root / "src.py").read_text().splitlines(keepends=True) + (root / "a.py").write_text("".join(lines[0:2])) + (root / "b.py").write_text("".join(lines[2:3])) + (root / "src.py").unlink() + rr.git_add_and_commit("split", cwd=str(root)) + + monkeypatch.chdir(repo) + monkeypatch.setattr(rr.tempfile, "mkdtemp", lambda prefix="": str(tmp_path / "wt")) + _silence_precommit(monkeypatch) + + verify_mechanical_refactor(base, target, transform) + assert "PASS" in capsys.readouterr().out + branch = f"verify-mechanical-{base[:8]}" + assert _git(repo, "branch", "--list", branch).endswith(branch) + + +def _precommit_writes_file(monkeypatch, filename: str, contents: str) -> None: + real = rr.exec_command + + def fake(cmd: str, cwd=None, check=True): + if cmd.startswith("pre-commit"): + (Path(cwd) / filename).write_text(contents) + return "" + return real(cmd, cwd=cwd, check=check) + + monkeypatch.setattr(rr, "exec_command", fake) + + +def test_reproduce_commits_pre_commit_fixes_when_tree_left_dirty( + repo: Path, tmp_path: Path, monkeypatch, capsys +) -> None: + """When pre-commit reformats and leaves the tree dirty, a 'pre-commit fixes' commit + is created on top of the transform commit.""" + _write(repo, **{"src.py": "hello\n"}) + base = _commit(repo, "base") + _write(repo, **{"src.py": "hello world\n", "formatted.py": "auto\n"}) + target = _commit(repo, "edit") + + def transform(root: Path) -> None: + (root / "src.py").write_text("hello world\n") + rr.git_add_and_commit("transform", cwd=str(root)) + + monkeypatch.chdir(repo) + monkeypatch.setattr(rr.tempfile, "mkdtemp", lambda prefix="": str(tmp_path / "wt")) + _precommit_writes_file(monkeypatch, "formatted.py", "auto\n") + + verify_mechanical_refactor(base, target, transform) + assert "PASS" in capsys.readouterr().out + branch = f"verify-mechanical-{base[:8]}" + subjects = _git(repo, "log", "--format=%s", "-2", branch).splitlines() + assert subjects == ["pre-commit fixes", "transform"] + + +# --- Repro.run end-to-end ------------------------------------------------------ + + +def test_repro_run_passes_on_a_faithful_call_site_lowering( + repo: Path, monkeypatch, capsys +) -> None: + """End-to-end: a lowering reproduces the commit byte-for-byte (pre-commit stubbed).""" + _write(repo, **{"c.py": "r = Old.foo(self.n, 5)\n"}) + base = _commit(repo, "base") + _write(repo, **{"c.py": "r = self.n.foo(5)\n"}) + target = _commit(repo, "lower the call site") + monkeypatch.chdir(repo) + _silence_precommit(monkeypatch) + + diff = Repro(base, target).lower_call_sites("foo", "Old", paths=["c.py"]).run() + assert diff == "" + assert "PASS" in capsys.readouterr().out + + +def test_repro_run_reports_residual_when_a_change_is_bundled( + repo: Path, monkeypatch, capsys +) -> None: + """A bundled non-relocation change surfaces as a non-empty residual diff.""" + _write(repo, **{"c.py": "r = Old.foo(self.n, 5)\nUNRELATED = 1\n"}) + base = _commit(repo, "base") + _write(repo, **{"c.py": "r = self.n.foo(5)\nUNRELATED = 2\n"}) + target = _commit(repo, "lower the call AND change a constant") + monkeypatch.chdir(repo) + _silence_precommit(monkeypatch) + + diff = Repro(base, target).lower_call_sites("foo", "Old", paths=["c.py"]).run() + assert "UNRELATED" in diff + assert "RESIDUAL" in capsys.readouterr().out diff --git a/.claude/skills/mechanical-refactor-verify/spec-reproduction-utils.md b/.claude/skills/mechanical-refactor-verify/spec-reproduction-utils.md new file mode 100644 index 000000000..ca9c0f3e9 --- /dev/null +++ b/.claude/skills/mechanical-refactor-verify/spec-reproduction-utils.md @@ -0,0 +1,164 @@ +# Reproduction utils — specification (source of truth) + +## 1. Scope + +- Source of truth for `scripts/mechanical_refactor_reproduction_utils.py`: the + **clean-move property** its primitives implement (§2), each primitive's contract (§3), + and the byte-diff arbiter's semantics (§4). +- The module, its tests, and the guides defer to this file; on any disagreement, this + file wins. +- Elsewhere: commit splitting → `guide-split.md`; producing a proof → + `guide-construct-proof.md`; reading one → `guide-verify-proof.md`. + +## 2. The property — a "clean move" + +> A commit is a **clean move** iff every change it makes is code **relocated in the same +> order** — allowing one **uniform indentation shift** of the whole block — plus a small +> fixed set of **move artifacts**, and nothing else. + +- Equivalently: the commit is reproducible by composing only the primitives of §3. +- The whitelist (§2.1) is exactly what they do; the not-allowed list (§2.2) is what they + refuse, so it surfaces as a residual diff. + +### 2.1 Allowed — the whole whitelist + +- A line **relocated in order**, modulo one **uniform** leading-indentation shift of the + whole block. +- **Defs/classes gathered from scattered positions** into a **new module**, each cut + verbatim, assembled under an **audited authored header**: + - the byte diff certifies the bodies; the header is reproduced from the target; + - the header audit accepts only: imports, a docstring, a TYPE_CHECKING import block, + a `logging.getLogger(__name__)` logger, or an unparse-equivalent copy of an + assignment actually deleted from the source (`drop_assigns`, e.g. + `_is_hip = is_hip()`); + - every dropped assignment must reappear in the header — anything else raises instead + of certifying. +- The **body of an extracted function** — an inline block relocated verbatim into a new + def; the `def` signature, an optional `return`, and the replacing `call` are authored. + Faithful **only** when the body moves unchanged; a de-self, control-flow restructure, or + bookkeeping consolidation is semantic and goes in its own commit first. +- **Import statements** — added, removed, or repathed; single-line or parenthesised. + Realised directly from the target (a wholly new module's statement verbatim, wrapping + preserved); a new-module move may add `from __future__ import annotations`. +- A one-sided **`@staticmethod` / `@classmethod`** — method ↔ free function. +- A **`self` type annotation dropped** from the moved definition — relocating + `@staticmethod def foo(self: Target)` into `Target` as `def foo(self)`. +- A **call-site requalification** — `Owner.foo(x)` → `foo(x)`: same symbol, same argument + bytes, only the qualifier dropped. (An `Old.foo(x)` → `New.foo(x)` owner swap is not a + primitive; it surfaces as a residual.) +- A **call-site lowering** — `Owner.method(receiver, rest)` → `receiver.method(rest)`: + the receiver moves out of the argument list. +- **Deleting a source file the relocation emptied** — nothing left beyond a docstring, + imports, or a `TYPE_CHECKING` block (`delete_file` refuses anything else). +- **Blank-line changes** — ignored (§2.3). + +### 2.2 Not allowed — the commit is **not** a clean move + +- A **reorder** of lines within the moved block. +- A **statement-level reorder** that relocates no definition — it changes evaluation + order: a reshape a human must confirm, not a certifiable relocation. +- A **non-uniform** indentation change — it can change Python semantics. +- A **trailing-whitespace** change, an internal-whitespace change, or a **line + merge/split**. +- A **changed argument** in an otherwise-requalified call. +- A **call rewrite for a symbol that did not move** in this commit. +- A **signature change** other than dropping the `self` annotation. +- A **rename** of the moved symbol (even a privacy flip `_foo` → `foo`). +- **Scaffolding or a constant authored into an existing module** — a logger, a module + constant, a `TYPE_CHECKING` guard, a re-derived `_flag = compute_flag()`. (A *new* + module's header is authored from the target, §2.1; an existing module's body is not a + place to author fresh code.) +- A **changed body in an extracted function** — de-self, control-flow restructure, or a + folded-in bookkeeping change: a semantic rewrite, not a relocation. + +- Reshape work (rename, fresh scaffolding, statement reorder, changed extraction body) + belongs in the prepare/postpare phases of `guide-split.md`. +- The proof reports it as a residual — never certifies it. + +### 2.3 Blank lines are ignored + +- A blank line never changes Python behavior; PEP 8 separator blanks legitimately collapse + on relocation. +- The formatter normalises both the reproduced and target sides, so a blank-line-only + difference cannot reach the byte diff. +- Assumption: the **target commit is itself pre-commit-clean** (true for any commit that + passed this repo's hooks); a target that skipped the formatter can show blank-line + residuals. + +## 3. The faithful relocation primitives + +- Each primitive does only a relocation-faithful edit — AST-located, spliced as original + source text, never regenerated. +- Therefore a byte match after the formatter certifies the commit is *exactly* that + relocation. + +- `move_symbol(name, *, src, dst, into_class, from_class, dedent, drop_self_annotation, + before, leave_delegate, delegate_name)`: + - cuts a `def` (functions only; a class moves via the extract primitives) with its + decorators; drops its own `@staticmethod`/`@classmethod`; + - shifts indentation uniformly (negative `dedent` indents into a class); + - pastes at a class end, at module level, or above the named sibling `before`; + - same-named defs need `from_class`; an ambiguous name or missing anchor raises; + - `leave_delegate` **authors** a forwarding stub in the source (original header + one + `return self..(...)`, `await`ed for async) — audit it like any header. +- `extract_to_new_module(src, dst, *, symbols, future_import)`: + - cuts the contiguous source tail: the moved defs/classes plus leading scaffolding + (imports, TYPE_CHECKING guards, name-target assignments only); + - an executable trailing statement stops the cut; + - prepends `from __future__ import annotations` when the move adds it. +- `extract_symbols_to_new_module(src, dst, *, symbols, header, order, drop_assigns)`: + - cuts the named defs/classes from **scattered** positions; assembles the new module + under the audited `header` (§2.1); + - `drop_assigns` deletes a relocated module-level constant from the source; a chained + `A = B = 1` keeps the surviving bindings. +- `extract_function(src, dst, *, name, signature, body, body_indent, call, return_text, + before, into_class)`: + - cuts an inline `body` verbatim (must match at a line boundary); + - re-indents under the authored `signature` — multi-line string interiors keep their + exact bytes; + - replaces the block with the authored `call`. +- `lower_call_sites(name, owner, *, paths)` — `Owner.m(receiver, rest)` → + `receiver.m(rest)` by splicing the original argument bytes (literal spelling, comments, + magic trailing comma survive); nested matching calls are all rewritten. +- `requalify_call_sites(name, owner, *, paths)` — `Owner.m(args)` → `m(args)`; only the + qualifier span changes. +- `remove_import(rel, import_text, *, in_function)` — function-scoped or module-level; + whole-statement match with token boundaries (`import os` cannot hit `import os.path`); + removes exactly the matched import even on a semicolon-joined line. +- `remove_imported_name(rel, *, module, name, asname)` — drops one name from a + `from m import a, b` (or a plain `import x`), realising a lost import directly (this + repo's ruff has no F811); an import carrying comments loses only the dropped alias's own + line. +- `add_import(rel, import_stmt)` — the import sorter places it; with no existing imports + it lands below the module docstring. +- `add_typechecking_import(rel, import_stmt)` — appends inside the destination's + `if TYPE_CHECKING:` block; the sorter orders it. +- `repath_import(rel, *, old_module, new_module, name)` — repaths a function-scoped + `from old import … name …` (relative imports included) in place; module-level repaths + fall out of add/remove + the sorter. +- `delete_file(path)` — deletes a source module the relocation emptied; refuses anything + beyond a docstring, imports, or a `TYPE_CHECKING` block. + +Cross-cutting guarantees: + +- CRLF sources round-trip byte-for-byte; synthesized lines follow the file's newline + style. +- Column arithmetic is UTF-8-byte-accurate; non-ASCII text does not shift a rewrite. + +## 4. The arbiter — reproduce and byte-diff + +`Repro.run()` (and the lower-level `verify_mechanical_refactor`): + +- checks out the base commit in a throwaway worktree; +- replays the recorded primitives; +- runs the repo's pre-commit hooks on the changed files; +- byte-diffs against the target commit — an empty diff is the proof; a non-empty diff is + returned as the residual, exactly what the relocation does not account for. + +Properties: + +- It runs the **real formatter**: a call split across an `= (` line, or a reflow leaving a + closing bracket as context, reproduces exactly — no diff-shape heuristic to fool. +- Explicit tradeoff: whatever the **pre-commit hooks auto-fix is absorbed** on both sides + (e.g. ruff's F401 removing a now-unused import). A hook-introduced change rides under a + byte match, so the hook set is part of the trusted base.