From 787bb748ba99f01d9d4daa52e633a50f53b1e2d9 Mon Sep 17 00:00:00 2001 From: Wolfvin Date: Sun, 12 Jul 2026 19:56:42 +0700 Subject: [PATCH] feat(impact): --action rename with call-site checklist (closes #241) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit impact_engine.analyze_impact(action="delete"|"modify") already did most of what a "safe to change" sandbox needs — the actual blast-radius traversal is action-agnostic (uses `if action == "delete"` guards that simply no-op for other values, no engine changes needed here). What was missing: rename, the most common refactor an AI agent performs, and the one most likely to be done unsafely without a checklist. `impact . --check impact --name X --action rename --new-name Y` now returns `rename_checklist` (every statically-resolved call site: file, line, caller) plus an explicit `rename_caveat` — this is static analysis only, it does NOT catch dynamic import(), reflection, string-keyed dispatch, or the name appearing in comments/docs. `--action rename` without `--new-name` errors immediately instead of silently running as if it were a no-op action. Verified end-to-end on a real workspace: renaming a symbol with 4 real call sites produced the correct checklist with file/line/caller for each, and the missing --new-name case errors clearly instead of proceeding. --- scripts/commands/impact.py | 40 +++++++++++++- tests/test_impact_command.py | 100 +++++++++++++++++++++++++++++++++++ 2 files changed, 139 insertions(+), 1 deletion(-) create mode 100644 tests/test_impact_command.py diff --git a/scripts/commands/impact.py b/scripts/commands/impact.py index 55f9415c..f36b31fc 100644 --- a/scripts/commands/impact.py +++ b/scripts/commands/impact.py @@ -66,8 +66,11 @@ def add_args(parser): f"Choices: {', '.join(ALL_CHECKS)}. Default: impact.") parser.add_argument("--name", default=None, help="impact: symbol name to analyze") - parser.add_argument("--action", choices=["modify", "delete"], default="modify", + parser.add_argument("--action", choices=["modify", "delete", "rename"], default="modify", help="impact: planned action (default: modify)") + parser.add_argument("--new-name", default=None, + help="impact: new symbol name, required with --action rename " + "(issue #241)") parser.add_argument("--domain", default="auto", help="impact: frontend|backend|auto (default: auto)") parser.add_argument("--depth", type=int, default=None, @@ -118,7 +121,42 @@ def _run_legacy_impact(args, workspace): action = getattr(args, "action", "modify") domain = getattr(args, "domain", "auto") depth = getattr(args, "depth", None) or 5 + new_name = getattr(args, "new_name", None) + + # Issue #241: rename simulation — every real call site needs updating + # to the new name, unlike modify (same signature, callers unaffected) + # or delete (callers need to stop referencing it entirely). + if action == "rename" and not new_name: + return { + "status": "error", + "error": "--action rename requires --new-name ", + } + result = analyze_impact(name, workspace, action=action, domain=domain, depth=depth) + if action == "rename" and result.get("status") == "ok": + result["new_name"] = new_name + checklist = [] + for item in result.get("affected", {}).get("direct", []): + checklist.append({ + "file": item.get("file"), + "line": item.get("line"), + "caller": item.get("name"), + }) + result["rename_checklist"] = checklist + result["rename_caveat"] = ( + f"This lists {len(checklist)} statically-resolved call site(s) that " + f"reference '{name}' and need updating to '{new_name}'. It does NOT " + "catch dynamic/string-based references (e.g. dynamic import(), " + "reflection, string-keyed dispatch tables, or the identifier " + "appearing in comments/docs) — grep for the old name as a final " + "check before considering the rename complete." + ) + result.setdefault("recommendations", []).insert( + 0, + f"Update {len(checklist)} call site(s) listed in rename_checklist, " + f"then grep for remaining '{name}' references (dynamic/string-based " + "usage is not covered by this analysis).", + ) if result.get("status") == "ok": engine_risk = result.get("risk", "low") stats = result.get("stats", {}) diff --git a/tests/test_impact_command.py b/tests/test_impact_command.py new file mode 100644 index 00000000..faf0922b --- /dev/null +++ b/tests/test_impact_command.py @@ -0,0 +1,100 @@ +"""Tests for the impact command's --action rename support (issue #241). + +`impact_engine.analyze_impact(action="delete"|"modify")` already computed +most of what a "safe to change" sandbox needs — this extends it to rename, +the most common refactor an AI agent performs, by attaching a concrete +checklist of every statically-resolved call site that needs updating to +the new name, plus an explicit caveat about what's NOT covered (dynamic/ +string-based references). +""" + +import argparse +import os +import sys +from unittest import mock + +import pytest + +SCRIPT_DIR = os.path.join( + os.path.dirname(os.path.dirname(os.path.abspath(__file__))), "scripts" +) +if SCRIPT_DIR not in sys.path: + sys.path.insert(0, SCRIPT_DIR) + +from commands.impact import _run_legacy_impact # noqa: E402 + + +def _args(**overrides): + ns = argparse.Namespace( + name="oldName", action="modify", domain="auto", depth=None, new_name=None, + ) + for k, v in overrides.items(): + setattr(ns, k, v) + return ns + + +def _fake_analyze_impact_result(): + return { + "status": "ok", + "symbol": "oldName", + "action": "rename", + "risk": "medium", + "affected": { + "direct": [ + {"name": "callerA", "file": "a.ts", "line": 10}, + {"name": "callerB", "file": "b.ts", "line": 22}, + ], + "indirect": [], + "files": ["a.ts", "b.ts"], + "tests": [], + }, + "stats": { + "direct_dependents": 2, + "indirect_dependents": 0, + "affected_files": 2, + "test_files_found": 0, + }, + } + + +class TestRenameAction: + def test_rename_without_new_name_errors(self): + result = _run_legacy_impact(_args(action="rename"), ".") + assert result["status"] == "error" + assert "--new-name" in result["error"] + + def test_rename_with_new_name_produces_checklist(self): + with mock.patch( + "impact_engine.analyze_impact", + return_value=_fake_analyze_impact_result(), + ): + result = _run_legacy_impact( + _args(action="rename", new_name="newName"), "." + ) + + assert result["status"] == "ok" + assert result["new_name"] == "newName" + assert len(result["rename_checklist"]) == 2 + assert result["rename_checklist"][0] == { + "file": "a.ts", "line": 10, "caller": "callerA", + } + assert "dynamic" in result["rename_caveat"] + assert "rename_checklist" in result["recommendations"][0] + + def test_modify_action_unaffected_by_rename_logic(self): + """--action modify (the default) must not get rename_checklist or + the rename-specific error path — regression guard for the new + branching added alongside rename support.""" + with mock.patch( + "impact_engine.analyze_impact", + return_value={ + "status": "ok", "risk": "low", + "affected": {"direct": [], "indirect": [], "files": [], "tests": []}, + "stats": {"direct_dependents": 0, "indirect_dependents": 0, + "affected_files": 0, "test_files_found": 0}, + }, + ): + result = _run_legacy_impact(_args(action="modify"), ".") + + assert "rename_checklist" not in result + assert "new_name" not in result