From 67b64af9fe4d68dae15909c25496f0105391c842 Mon Sep 17 00:00:00 2001 From: vitali87 Date: Sun, 20 Sep 2026 23:56:04 +0100 Subject: [PATCH] test: cover move MCP wiring --- codebase_rag/constants/cli.py | 10 + codebase_rag/constants/graph.py | 1 + codebase_rag/constants/mcp.py | 3 + codebase_rag/cypher_queries.py | 6 + codebase_rag/editing/__init__.py | 4 + codebase_rag/editing/move.py | 65 +++ codebase_rag/editing/move_cycles.py | 44 +++ codebase_rag/editing/move_operation.py | 372 ++++++++++++++++++ codebase_rag/editing/move_retarget.py | 108 +++++ codebase_rag/editing/move_scope.py | 185 +++++++++ codebase_rag/editing/move_types.py | 42 ++ codebase_rag/mcp/tools.py | 87 +++- codebase_rag/structural_delta.py | 40 +- codebase_rag/tests/move_helpers.py | 79 ++++ codebase_rag/tests/test_change_signature.py | 3 +- .../tests/test_move_import_presence.py | 50 +++ codebase_rag/tests/test_move_mcp.py | 55 +++ codebase_rag/tests/test_move_operation.py | 201 ++++++++++ .../tests/test_variadic_header_detection.py | 78 ++++ codebase_rag/tools/tool_descriptions.py | 19 + evals/cgr_graph.py | 89 +++-- 21 files changed, 1501 insertions(+), 40 deletions(-) create mode 100644 codebase_rag/editing/move.py create mode 100644 codebase_rag/editing/move_cycles.py create mode 100644 codebase_rag/editing/move_operation.py create mode 100644 codebase_rag/editing/move_retarget.py create mode 100644 codebase_rag/editing/move_scope.py create mode 100644 codebase_rag/editing/move_types.py create mode 100644 codebase_rag/tests/move_helpers.py create mode 100644 codebase_rag/tests/test_move_import_presence.py create mode 100644 codebase_rag/tests/test_move_mcp.py create mode 100644 codebase_rag/tests/test_move_operation.py create mode 100644 codebase_rag/tests/test_variadic_header_detection.py diff --git a/codebase_rag/constants/cli.py b/codebase_rag/constants/cli.py index 8851800932..78b3da9d68 100644 --- a/codebase_rag/constants/cli.py +++ b/codebase_rag/constants/cli.py @@ -648,6 +648,16 @@ class DiffMarker: SIGNATURE_NEEDS_KEYWORDS = "the mapping needs keyword arguments this language lacks" SIGNATURE_UNMAPPED_PARAM = "the site passes no value for {name}" SIGNATURE_SURPLUS_ARGS = "the site passes more arguments than the definition declares" +# move (issue #1534). +MOVE_UNKNOWN = "No definition named {qn} in the graph" +MOVE_METHOD = "{qn} is a method; move its class instead" +MOVE_SAME_MODULE = "{path} already holds the definition" +MOVE_NO_GRAMMAR = "No grammar for {path}; the definition cannot be moved" +MOVE_NO_DEFINITION_TOKEN = "Could not locate the definition of {qn} in {path}" +MOVE_CYCLE = "Refusing to move: it would create the import cycle {cycle}" +MOVE_PLANNED = "{importers} importer(s) would be rewritten, {unchanged} left unchanged" +MOVE_PARSE_FAILED = "Move rolled back: {files} would no longer parse" +MOVE_CONTRACT_FAILED = "Move rolled back, postcondition failed: {reasons}" SIGNATURE_DEFAULT_ORDER = "Parameter {name} has no default but follows one that does" # Postcondition contract (issue #1531). CONTRACT_OP_RENAME = "rename" diff --git a/codebase_rag/constants/graph.py b/codebase_rag/constants/graph.py index e5c2a8c2e5..ce06b08078 100644 --- a/codebase_rag/constants/graph.py +++ b/codebase_rag/constants/graph.py @@ -63,6 +63,7 @@ KEY_STRUCTURAL_DELTA = "structural_delta" KEY_UNMAPPED = "unmapped" KEY_VERDICT = "verdict" +KEY_CYCLE = "cycle" KEY_DISPATCH_LITERAL = "dispatch_literal" diff --git a/codebase_rag/constants/mcp.py b/codebase_rag/constants/mcp.py index ab1e1083fb..47f02d4788 100644 --- a/codebase_rag/constants/mcp.py +++ b/codebase_rag/constants/mcp.py @@ -22,6 +22,7 @@ class MCPToolName(StrEnum): # Graph-driven edit operations (issue #1532). RENAME = "rename" CHANGE_SIGNATURE = "change_signature" + MOVE = "move" QUERY_CODE_GRAPH = "query_code_graph" GET_CODE_SNIPPET = "get_code_snippet" SURGICAL_REPLACE_CODE = "surgical_replace_code" @@ -112,6 +113,8 @@ class MCPParamName(StrEnum): NEW_NAME = "new_name" ALLOW_HEURISTIC = "allow_heuristic" NEW_PARAMS = "new_params" + TARGET_MODULE = "target_module" + KEEP_ALIAS = "keep_alias" # MCP server constants diff --git a/codebase_rag/cypher_queries.py b/codebase_rag/cypher_queries.py index 3f756b5c3e..a43f3caada 100644 --- a/codebase_rag/cypher_queries.py +++ b/codebase_rag/cypher_queries.py @@ -815,6 +815,12 @@ def build_create_relationship_query( RETURN n.positional_params AS positional_params, n.param_types AS param_types, n.return_type AS return_type LIMIT 1""" +# What one module imports, with each statement's site (issue #1534). +CYPHER_GRAPH_IMPORTS_OF = """MATCH (m:Module)-[r:IMPORTS]->(target) +WHERE m.qualified_name = $qn AND m.qualified_name STARTS WITH $project_prefix +RETURN target.qualified_name AS to_qn, r.line AS line, r.col AS col, + r.end_line AS end_line, r.end_col AS end_col, r.alias AS alias, + r.imported_name AS imported_name""" CYPHER_GRAPH_IMPORTERS = """MATCH (m:Module)-[r:IMPORTS]->(target) WHERE target.qualified_name = $qn AND m.qualified_name STARTS WITH $project_prefix RETURN m.qualified_name AS qualified_name, m.path AS path, r.line AS line, diff --git a/codebase_rag/editing/__init__.py b/codebase_rag/editing/__init__.py index c1c80f48d6..14547689fe 100644 --- a/codebase_rag/editing/__init__.py +++ b/codebase_rag/editing/__init__.py @@ -11,6 +11,7 @@ verify, ) from .imports import ImportRewriter, ImportSite, Rewrite, RewriteError, SymbolMove +from .move import MoveRefused, MoveReport, move from .patcher import ( Patcher, PatcherError, @@ -43,6 +44,9 @@ __all__ = [ "Expectation", + "MoveRefused", + "MoveReport", + "move", "ParamSpec", "SignatureRefused", "SignatureReport", diff --git a/codebase_rag/editing/move.py b/codebase_rag/editing/move.py new file mode 100644 index 0000000000..89233d9e88 --- /dev/null +++ b/codebase_rag/editing/move.py @@ -0,0 +1,65 @@ +"""`move(qn, target_module)`: edit-algebra operation 3 (issue #1534). + +Moving a symbol out of a shared dumping ground into the one package that +uses it is the refactor that shrinks affected sets, and by hand it is +tedious: the definition, its own imports, every importer, re-exports. The +graph knows the definition's span, the imports its module binds, and every +importer's statement (issue #1522), so the whole move is one transaction: + +- cut the definition (decorators, docstring, adjacent comments included) + and paste it into the target with the imports it needs; +- rewrite every importer through the import rewriter; +- give the old module an import of the moved name when it still uses it, + and optionally a deprecation re-export (`keep_alias=True`); +- refuse before touching a file when the move would create an import + cycle, naming the cycle. +""" + +from __future__ import annotations + +from collections.abc import Callable +from pathlib import Path + +from ..graph_query import QueryFn +from .contract import Reingest +from .move_scope import _module_bound +from .move_types import MoveRefused, MoveReport +from .transaction import StagedTree, VerificationResult + + +def __getattr__(name: str): + if name == "Mover": + from .move_operation import Mover + + return Mover + raise AttributeError(name) + + +def move( + repo_root: Path, + fetch_all: QueryFn, + project_name: str, + qualified_name: str, + target_module: str, + keep_alias: bool = False, + dry_run: bool = False, + verify: Callable[[StagedTree], VerificationResult | bool | None] | None = None, + reingest: Reingest | None = None, +) -> MoveReport: + """The op: plan (refusing on a cycle) or plan and apply.""" + from .move_operation import move as _move + + return _move( + repo_root, + fetch_all, + project_name, + qualified_name, + target_module, + keep_alias=keep_alias, + dry_run=dry_run, + verify=verify, + reingest=reingest, + ) + + +__all__ = ["MoveRefused", "MoveReport", "move", "_module_bound"] diff --git a/codebase_rag/editing/move_cycles.py b/codebase_rag/editing/move_cycles.py new file mode 100644 index 0000000000..6cfe7521ab --- /dev/null +++ b/codebase_rag/editing/move_cycles.py @@ -0,0 +1,44 @@ +"""Import-cycle refusal for move planning.""" + +from __future__ import annotations + +from .. import constants as cs +from ..graph_query import QueryFn +from ..structural_delta import import_cycles, snapshot +from .move_types import MoveRefused + + +class _MoveCycleMixin: + fetch_all: QueryFn + project: str + + def _refuse_cycles( + self, + old_module: str, + new_module: str, + copied_targets: list[str], + needs_old: bool, + old_needs_new: bool, + old_path: str, + new_path: str, + ) -> None: + graph = { + qn: set(targets) + for qn, targets in snapshot( + self.fetch_all, self.project, [old_path, new_path] + ).imports.items() + } + before = import_cycles({qn: frozenset(t) for qn, t in graph.items()}) + graph.setdefault(new_module, set()).update(t for t in copied_targets if t) + if needs_old: + graph.setdefault(new_module, set()).add(old_module) + if old_needs_new: + graph.setdefault(old_module, set()).add(new_module) + for importer, targets in graph.items(): + if old_module in targets and importer not in (old_module, new_module): + targets.add(new_module) + after = import_cycles({qn: frozenset(t) for qn, t in graph.items()}) + fresh = [c for c in after - before if new_module in c or old_module in c] + if fresh: + cycle = tuple(sorted(fresh[0])) + raise MoveRefused(cs.MOVE_CYCLE.format(cycle=" -> ".join(cycle)), cycle) diff --git a/codebase_rag/editing/move_operation.py b/codebase_rag/editing/move_operation.py new file mode 100644 index 0000000000..f7ceb1f7fa --- /dev/null +++ b/codebase_rag/editing/move_operation.py @@ -0,0 +1,372 @@ +"""Implementation of the move refactoring operation.""" + +from __future__ import annotations + +from collections.abc import Callable +from pathlib import Path + +from tree_sitter import Node + +from .. import constants as cs +from .. import cypher_queries as cq +from .. import graph_query +from ..graph_query import QueryFn +from ..language_spec import get_language_for_extension +from ..parser_loader import load_parsers +from ..utils.path_utils import base_module_qn +from .contract import Reingest, measure, move_expectation, verify +from .imports import ImportSite, _relative_specifier +from .move_cycles import _MoveCycleMixin +from .move_retarget import _MoveRetargetMixin +from .move_scope import ( + _JS_LANGUAGES, + _cut_span, + _definition_at, + _import_block_end, + _narrow_statement, + _statement_text, + _strip_project, + _uses, +) +from .move_types import MoveRefused, MoveReport, _NeededImport +from .patcher import Patcher, PatcherError +from .rename import _name_token +from .transaction import EditTransaction, StagedTree, VerificationResult, undo_last + + +class Mover(_MoveRetargetMixin, _MoveCycleMixin): + def __init__( + self, + repo_root: Path, + fetch_all: QueryFn, + project_name: str, + verify: Callable[[StagedTree], VerificationResult | bool | None] | None = None, + reingest: Reingest | None = None, + ) -> None: + self.repo_root = repo_root.resolve() + self.fetch_all = fetch_all + self.project = project_name + self.verify = verify + self.reingest = reingest + self._parsers = load_parsers()[0] + + def _parse( + self, path: str, source: bytes + ) -> tuple[cs.SupportedLanguage | None, Node]: + language = get_language_for_extension(Path(path).suffix) + parser = self._parsers.get(language) if language is not None else None + if parser is None: + raise MoveRefused(cs.MOVE_NO_GRAMMAR.format(path=path)) + return language, parser.parse(source).root_node + + def _target_path(self, target: str, old_path: str) -> str: + suffix = Path(old_path).suffix + if target.endswith(suffix) or "/" in target: + return Path(target).as_posix() + dotted = _strip_project(target, self.project) + return Path(*dotted.split(cs.SEPARATOR_DOT)).with_suffix(suffix).as_posix() + + # --- planning --------------------------------------------------------------------- + + def plan( + self, qn: str, target: str, keep_alias: bool = False + ) -> tuple[MoveReport, Patcher, str | None]: + definition = graph_query.definition( + self.fetch_all, self.project, qn, self.repo_root + ) + if not definition["found"] or not definition["path"]: + raise MoveRefused(cs.MOVE_UNKNOWN.format(qn=qn)) + if definition["label"] == cs.NodeLabel.METHOD.value: + raise MoveRefused(cs.MOVE_METHOD.format(qn=qn)) + old_path = definition["path"] + name = definition["name"] or qn.rsplit(cs.SEPARATOR_DOT, 1)[-1] + new_path = self._target_path(target, old_path) + if new_path == old_path: + raise MoveRefused(cs.MOVE_SAME_MODULE.format(path=old_path)) + old_module = base_module_qn(Path(old_path), self.project) + new_module = base_module_qn(Path(new_path), self.project) + patcher = Patcher(self.repo_root) + source = patcher.source(old_path) + language, root = self._parse(old_path, source) + token = _name_token( + source, + language, + definition["start_line"] or 1, + definition["end_line"] or 1, + name, + ) + node = _definition_at(root, *token) if token else None + if node is None: + raise MoveRefused(cs.MOVE_NO_DEFINITION_TOKEN.format(qn=qn, path=old_path)) + cut = _cut_span(source, node) + remainder = (source[: cut.start] + source[cut.end :]).decode( + cs.ENCODING_UTF8, errors="replace" + ) + + needed = self._needed_imports( + old_module, old_path, new_path, cut.text, language + ) + from_old = self._needed_from_old(old_path, cut.text, name) + old_uses = _uses(remainder, name) + # The cycle check runs on the graph BEFORE any file is touched. + self._refuse_cycles( + old_module, + new_module, + [n.target_qn for n in needed], + bool(from_old), + old_uses or keep_alias, + old_path, + new_path, + ) + + old_spelled = _strip_project(old_module, self.project) + new_spelled = _strip_project(new_module, self.project) + # 1. Cut from the old module, import the name back when still used. + replacement = "" + patcher.replace_span(old_path, (cut.start, cut.end), replacement) + if old_uses or keep_alias: + self._add_import( + patcher, + old_path, + source, + root, + language, + new_spelled, + new_path, + name, + export=keep_alias and not old_uses, + ) + # 2. Paste into the target with what it needs. + paste = self._paste_text( + cut.text, needed, from_old, old_spelled, old_path, new_path, language + ) + new_content: str | None = None + try: + existing = patcher.source(new_path) + except PatcherError: + new_content = paste.lstrip("\n") + else: + sep = ( + "" + if not existing.strip() + else ("\n" if existing.endswith(b"\n\n") else "\n\n") + ) + if existing and not existing.endswith(b"\n"): + sep = "\n" + sep + patcher.replace_span( + new_path, (len(existing), len(existing)), sep + paste.lstrip("\n") + ) + # 3. Importers, and uses through a module import (`pkg.util.helper`). + self._retarget_attribute_uses( + patcher, qn, old_spelled, new_spelled, name, language + ) + importers, unchanged = self._retarget_importers( + patcher, old_module, old_spelled, new_spelled, new_path, name, language + ) + files = sorted(set(patcher.pending) | {new_path}) + report = MoveReport( + qualified_name=qn, + new_qualified_name=f"{new_module}{cs.SEPARATOR_DOT}{name}", + old_path=old_path, + new_path=new_path, + applied=False, + transaction_id="", + files=tuple(files), + importers=tuple(sorted(importers)), + unchanged_importers=tuple(sorted(unchanged)), + copied_imports=tuple(n.statement for n in needed), + diff="", + message=cs.MOVE_PLANNED.format( + importers=len(importers), unchanged=len(unchanged) + ), + ) + return report, patcher, new_content + + def _needed_imports( + self, + old_module: str, + old_path: str, + new_path: str, + moved_text: str, + language: cs.SupportedLanguage | None, + ) -> list[_NeededImport]: + """The old module's import statements the moved text relies on.""" + rows = self.fetch_all( + cq.CYPHER_GRAPH_IMPORTS_OF, + { + cs.KEY_PROJECT_PREFIX: f"{self.project}{cs.SEPARATOR_DOT}", + cs.KEY_QN: old_module, + }, + ) + source = Patcher(self.repo_root).source(old_path) + out: dict[str, _NeededImport] = {} + for row in rows: + alias = row.get(cs.KEY_ALIAS) + line, col = row.get(cs.KEY_LINE), row.get(cs.KEY_COL) + end_line, end_col = row.get(cs.KEY_END_LINE), row.get(cs.KEY_END_COL) + if ( + not isinstance(alias, str) + or not isinstance(line, int) + or not isinstance(col, int) + ): + continue + bound = alias.split(cs.SEPARATOR_DOT)[0] + imported_raw = row.get(cs.KEY_IMPORTED_NAME) + imported = imported_raw if isinstance(imported_raw, str) else None + if not _uses(moved_text, bound): + continue + site = ImportSite( + old_path, + line, + col, + end_line if isinstance(end_line, int) else line, + end_col if isinstance(end_col, int) else col, + alias, + imported, + ) + statement = _statement_text(source, site) + narrowed = _narrow_statement(statement, alias, language, old_path, new_path) + if narrowed is None: + continue + target = str(row.get(cs.KEY_TO_QN) or "") + out.setdefault(narrowed, _NeededImport(narrowed, target)) + return list(out.values()) + + def _needed_from_old(self, old_path: str, moved_text: str, name: str) -> list[str]: + """Old-module definitions the moved text still refers to.""" + rows = self.fetch_all( + cq.CYPHER_DELTA_DEFINITIONS, + { + cs.KEY_PROJECT_PREFIX: f"{self.project}{cs.SEPARATOR_DOT}", + cs.CYPHER_PARAM_PATHS: [old_path], + }, + ) + names: set[str] = set() + for row in rows: + other = row.get(cs.KEY_NAME) + qn = str(row.get(cs.KEY_QUALIFIED_NAME) or "") + if ( + not isinstance(other, str) + or other == name + or cs.SEPARATOR_DOT + in qn[len(base_module_qn(Path(old_path), self.project)) + 1 :] + ): + continue + if _uses(moved_text, other): + names.add(other) + return sorted(names) + + def _add_import( + self, + patcher: Patcher, + path: str, + source: bytes, + root: Node, + language: cs.SupportedLanguage | None, + new_spelled: str, + new_path: str, + name: str, + export: bool, + ) -> None: + at = _import_block_end(source, root) + if language in _JS_LANGUAGES: + spec = _relative_specifier(path, new_path) + line = ( + f"export {{ {name} }} from '{spec}';\n" + if export + else f"import {{ {name} }} from '{spec}';\n" + ) + else: + line = f"from {new_spelled} import {name}\n" + if export: + line = f"from {new_spelled} import {name} # noqa: F401 (moved; re-exported for compatibility)\n" + patcher.replace_span(path, (at, at), line if at else line + "\n") + + def _paste_text( + self, + moved: str, + needed: list[_NeededImport], + from_old: list[str], + old_spelled: str, + old_path: str, + new_path: str, + language: cs.SupportedLanguage | None, + ) -> str: + lines = [n.statement.rstrip("\n") for n in needed] + if from_old: + if language in _JS_LANGUAGES: + spec = _relative_specifier(new_path, old_path) + lines.append(f"import {{ {', '.join(from_old)} }} from '{spec}';") + else: + lines.append(f"from {old_spelled} import {', '.join(from_old)}") + header = "\n".join(lines) + return (header + "\n\n\n" if header else "") + moved.rstrip("\n") + "\n" + + # --- applying --------------------------------------------------------------------- + + def apply(self, qn: str, target: str, keep_alias: bool = False) -> MoveReport: + report, patcher, new_content = self.plan(qn, target, keep_alias) + tx = EditTransaction(self.repo_root) + results = patcher.stage_into(tx) + if new_content is not None: + tx.stage(report.new_path, new_content) + broken = [key for key, result in results.items() if result.parses is False] + if broken: + tx.rollback() + return report._replace( + message=cs.MOVE_PARSE_FAILED.format(files=", ".join(broken)) + ) + + def verifier(tree: StagedTree) -> VerificationResult | bool | None: + return self.verify(tree) if self.verify is not None else True + + outcome = tx.commit(verifier) + report = report._replace( + applied=outcome.applied, + transaction_id=outcome.transaction_id, + files=outcome.files, + diff=outcome.diff, + message=outcome.message, + ) + if outcome.applied and self.reingest is not None: + report = self._enforce_contract(report) + return report + + def _enforce_contract(self, report: MoveReport) -> MoveReport: + assert self.reingest is not None + delta = measure( + self.fetch_all, self.project, self.repo_root, report.files, self.reingest + ) + verdict = verify( + move_expectation(report.qualified_name, report.new_qualified_name), delta + ) + if verdict.ok: + return report._replace(verdict=verdict) + undo_last(self.repo_root) + self.reingest(list(report.files)) + return report._replace( + applied=False, + verdict=verdict, + message=cs.MOVE_CONTRACT_FAILED.format(reasons="; ".join(verdict.failures)), + ) + + +def move( + repo_root: Path, + fetch_all: QueryFn, + project_name: str, + qualified_name: str, + target_module: str, + keep_alias: bool = False, + dry_run: bool = False, + verify: Callable[[StagedTree], VerificationResult | bool | None] | None = None, + reingest: Reingest | None = None, +) -> MoveReport: + """The op: plan (refusing on a cycle) or plan and apply.""" + mover = Mover(repo_root, fetch_all, project_name, verify=verify, reingest=reingest) + if dry_run: + report, _patcher, _content = mover.plan( + qualified_name, target_module, keep_alias + ) + return report + return mover.apply(qualified_name, target_module, keep_alias) diff --git a/codebase_rag/editing/move_retarget.py b/codebase_rag/editing/move_retarget.py new file mode 100644 index 0000000000..c4eec1e047 --- /dev/null +++ b/codebase_rag/editing/move_retarget.py @@ -0,0 +1,108 @@ +"""Importer retargeting for the move refactoring.""" + +from __future__ import annotations + +from pathlib import Path + +from tree_sitter import Node + +from .. import constants as cs +from .. import graph_query +from ..graph_query import QueryFn +from .imports import _JS_SPEC, ImportRewriter, ImportSite, SymbolMove +from .move_scope import _JS_LANGUAGES, _import_block_end, _module_bound, _statement_text +from .patcher import Patcher, PatcherError + + +class _MoveRetargetMixin: + repo_root: Path + fetch_all: QueryFn + project: str + + def _parse( + self, path: str, source: bytes + ) -> tuple[cs.SupportedLanguage | None, Node]: ... + + def _retarget_importers( + self, + patcher: Patcher, + old_module: str, + old_spelled: str, + new_spelled: str, + new_path: str, + name: str, + language: cs.SupportedLanguage | None, + ) -> tuple[list[str], list[str]]: + rewriter = ImportRewriter(self.repo_root, patcher) + importers: set[str] = set() + unchanged: set[str] = set() + for row in graph_query.importers(self.fetch_all, self.project, old_module): + if ( + row["imported_name"] != name + or row["path"] is None + or row["line"] is None + ): + continue + site = ImportSite( + row["path"], + row["line"], + row["col"] or 0, + row["end_line"] or row["line"], + row["end_col"] or 0, + row["alias"], + row["imported_name"], + ) + spelled = old_spelled + if language in _JS_LANGUAGES: + statement = _statement_text(patcher.source(site.path), site) + spec = _JS_SPEC.search(statement) + spelled = spec.group("spec") if spec else old_spelled + move = SymbolMove(name, spelled, new_spelled, new_module_path=new_path) + if rewriter.retarget([site], move): + importers.add(site.path) + else: + unchanged.add(f"{site.path}:{site.line}") + return sorted(importers), sorted(unchanged) + + def _retarget_attribute_uses( + self, + patcher: Patcher, + qn: str, + old_spelled: str, + new_spelled: str, + name: str, + language: cs.SupportedLanguage | None, + ) -> None: + """`pkg.util.helper(...)` through `import pkg.util` follows the move.""" + if language != cs.SupportedLanguage.PYTHON: + return + from .patcher import line_col_to_byte + + old_attr = f"{old_spelled}{cs.SEPARATOR_DOT}{name}".encode(cs.ENCODING_UTF8) + new_attr = f"{new_spelled}{cs.SEPARATOR_DOT}{name}" + touched: set[str] = set() + for row in graph_query.callers(self.fetch_all, self.project, qn): + path, line, col = row["path"], row["line"], row["col"] + if not isinstance(path, str) or line is None or col is None: + continue + try: + source = patcher.source(path) + except PatcherError: + continue + start = line_col_to_byte(source, line, col) + end = line_col_to_byte( + source, row["end_line"] or line, row["end_col"] or col + ) + at = source.find(old_attr, start, end) + if at < 0: + continue + patcher.replace_span(path, (at, at + len(old_attr)), new_attr) + if path not in touched: + touched.add(path) + _language, root = self._parse(path, source) + text = source.decode(cs.ENCODING_UTF8, errors="replace") + if not _module_bound(text, new_spelled): + at_import = _import_block_end(source, root) + patcher.replace_span( + path, (at_import, at_import), f"import {new_spelled}\n" + ) diff --git a/codebase_rag/editing/move_scope.py b/codebase_rag/editing/move_scope.py new file mode 100644 index 0000000000..94b9112eb0 --- /dev/null +++ b/codebase_rag/editing/move_scope.py @@ -0,0 +1,185 @@ +"""Parsing and import-statement helpers for move planning.""" + +from __future__ import annotations + +import ast +import re +from pathlib import Path + +from tree_sitter import Node + +from .. import constants as cs +from .imports import ( + _JS_NAMED, + _JS_SPEC, + _PY_IMPORT, + ImportSite, + _local_name, + _match_py_from, + _relative_specifier, + _split_names, +) +from .move_types import _Cut + +_IDENTIFIER = r"(? str: + if node is None or node.text is None: + return "" + return node.text.decode(cs.ENCODING_UTF8, errors="replace") + + +def _module_bound(text: str, module: str) -> bool: + """Whether `text` binds `module`'s root name via a plain `import`. + + The move rewrites call sites to `pkg.new.helper(...)`, which needs + `pkg` bound. Only a plain `import pkg.new` does that -- or a deeper + `import pkg.new.sub`, which binds `pkg` just the same (verified + against a real interpreter; it must keep answering True). + + Deciding this by searching the raw text was wrong in the direction + that breaks code. `import pkg.new as n` binds only `n`; a comment or + a string containing the words binds nothing. Each made the caller + skip adding the real import AFTER the call sites had been rewritten, + and both versions parse, so the postcondition could not catch it + either -- the program died at runtime with + `NameError: name 'pkg' is not defined` on the line the move wrote. + + Unparseable input answers False: the caller then adds an import it + may not need, which is recoverable, rather than omitting one it does. + """ + try: + tree = ast.parse(text) + except SyntaxError: + return False + wanted = module.split(".") + for node in ast.walk(tree): + if not isinstance(node, ast.Import): + continue + for alias in node.names: + # `import x as y` binds only `y`, never x's root. + if alias.asname is None and alias.name.split(".")[: len(wanted)] == wanted: + return True + return False + + +def _uses(text: str, name: str) -> bool: + return re.search(_IDENTIFIER % re.escape(name), text) is not None + + +def _definition_at(root: Node, line: int, col: int) -> Node | None: + """The outermost definition whose own name token starts at (line, col).""" + stack = [root] + while stack: + node = stack.pop() + named = node.child_by_field_name(cs.FIELD_NAME) + if ( + named is not None + and named.start_point == (line - 1, col) + and (node.type not in (cs.TS_IDENTIFIER, cs.TS_PY_IDENTIFIER)) + ): + return node + if node.start_point[0] <= line - 1 <= node.end_point[0]: + stack.extend(reversed(node.children)) + return None + + +def _cut_span(source: bytes, node: Node) -> _Cut: + """Whole lines of the definition plus decorators, export and comments.""" + target = node + if target.parent is not None and target.parent.type in _WRAPPERS: + target = target.parent + first = target + sibling = target.prev_named_sibling + while ( + sibling is not None + and sibling.type == cs.TS_COMMENT + and sibling.end_point[0] + 1 == first.start_point[0] + ): + first = sibling + sibling = sibling.prev_named_sibling + start = source.rfind(b"\n", 0, first.start_byte) + 1 + end = source.find(b"\n", target.end_byte) + end = len(source) if end < 0 else end + 1 + text = source[start:end].decode(cs.ENCODING_UTF8, errors="replace") + # Swallow the blank lines that separated it from what follows. + while source[end : end + 1] == b"\n": + end += 1 + return _Cut(start, end, text) + + +def _import_block_end(source: bytes, root: Node) -> int: + """Byte offset just after the last top-level import (0 when none).""" + end = 0 + for child in root.children: + if child.type in _IMPORT_TYPES: + end = source.find(b"\n", child.end_byte) + end = len(source) if end < 0 else end + 1 + return end + + +def _strip_project(qn: str, project: str) -> str: + prefix = f"{project}{cs.SEPARATOR_DOT}" + return qn[len(prefix) :] if qn.startswith(prefix) else qn + + +# --- statement helpers ----------------------------------------------------------------- + + +def _statement_text(source: bytes, site: ImportSite) -> str: + from .patcher import line_col_to_byte + + start = line_col_to_byte(source, site.line, site.col) + end = line_col_to_byte(source, site.end_line, site.end_col) + return source[start:end].decode(cs.ENCODING_UTF8, errors="replace") + + +def _narrow_statement( + statement: str, + alias: str, + language: cs.SupportedLanguage | None, + old_path: str, + new_path: str, +) -> str | None: + """The statement reduced to the entry binding `alias`, respelled for + the new file where the specifier is relative.""" + if language in _JS_LANGUAGES: + spec = _JS_SPEC.search(statement) + if spec is None: + return None + text = statement + if spec.group("spec").startswith("."): + target = (Path(old_path).parent / spec.group("spec")).as_posix() + text = ( + statement[: spec.start("spec")] + + _relative_specifier(new_path, target) + + statement[spec.end("spec") :] + ) + named = _JS_NAMED.search(text) + if named is None: + return text.strip() + entries = [e.strip() for e in named.group("names").split(",") if e.strip()] + kept = [e for e in entries if _local_name(e) == alias] + if not kept: + return None + return ( + text[: named.start()] + "{ " + ", ".join(kept) + " }" + text[named.end() :] + ).strip() + if parsed := _match_py_from(statement): + # main replaced the _PY_FROM regex with a token parser returning + # (lead, module, mid, names); only those two fields are needed here. + _lead, module, _mid, raw_names = parsed + entries, _open, _close = _split_names(raw_names) + kept = [e for e in entries if _local_name(e) == alias] + if not kept: + return None + return f"from {module} import {kept[0]}" + if _PY_IMPORT.match(statement): + return statement.strip() + return statement.strip() diff --git a/codebase_rag/editing/move_types.py b/codebase_rag/editing/move_types.py new file mode 100644 index 0000000000..ac2e19599a --- /dev/null +++ b/codebase_rag/editing/move_types.py @@ -0,0 +1,42 @@ +"""Public report and refusal types for the move refactoring.""" + +from __future__ import annotations + +from typing import NamedTuple + +from .contract import Verdict + + +class MoveRefused(ValueError): + """The move cannot be planned as asked; nothing was written.""" + + def __init__(self, message: str, cycle: tuple[str, ...] = ()) -> None: + super().__init__(message) + self.cycle = cycle + + +class MoveReport(NamedTuple): + qualified_name: str + new_qualified_name: str + old_path: str + new_path: str + applied: bool + transaction_id: str + files: tuple[str, ...] + importers: tuple[str, ...] + unchanged_importers: tuple[str, ...] + copied_imports: tuple[str, ...] + diff: str + message: str + verdict: Verdict | None = None + + +class _Cut(NamedTuple): + start: int + end: int + text: str + + +class _NeededImport(NamedTuple): + statement: str + target_qn: str diff --git a/codebase_rag/mcp/tools.py b/codebase_rag/mcp/tools.py index c4725f4faf..305b7fe136 100644 --- a/codebase_rag/mcp/tools.py +++ b/codebase_rag/mcp/tools.py @@ -125,6 +125,11 @@ def _read_file_slice(full_path: Path, start: int, limit: int | None) -> str: # result to the postcondition contract, so refusing up front on a # partial graph would block the operation that repairs it. cs.MCPToolName.CHANGE_SIGNATURE, + # An EDIT like RENAME and CHANGE_SIGNATURE above: it runs its own + # re-ingest behind the incomplete-run marker and holds the result to + # the postcondition contract, so refusing up front on a partial graph + # would block the operation that repairs it (issue #1534). + cs.MCPToolName.MOVE, cs.MCPToolName.SURGICAL_REPLACE_CODE, cs.MCPToolName.READ_FILE, cs.MCPToolName.WRITE_FILE, @@ -492,6 +497,7 @@ def __init__( ), cs.MCPToolName.RENAME: self._rename_tool(), cs.MCPToolName.CHANGE_SIGNATURE: self._change_signature_tool(), + cs.MCPToolName.MOVE: self._move_tool(), cs.MCPToolName.QUERY_CODE_GRAPH: ToolMetadata( name=cs.MCPToolName.QUERY_CODE_GRAPH, description=td.MCP_TOOLS[cs.MCPToolName.QUERY_CODE_GRAPH], @@ -2662,10 +2668,89 @@ def _run_change_signature( payload[cs.KEY_VERDICT] = report.verdict._asdict() return payload + def _move_tool(self) -> ToolMetadata: + def prop(kind: cs.MCPSchemaType, description: str) -> MCPInputSchemaProperty: + return MCPInputSchemaProperty(type=kind, description=description) + + return ToolMetadata( + name=cs.MCPToolName.MOVE, + description=td.MCP_TOOLS[cs.MCPToolName.MOVE], + input_schema=MCPInputSchema( + type=cs.MCPSchemaType.OBJECT, + properties={ + cs.MCPParamName.QUALIFIED_NAME: prop( + cs.MCPSchemaType.STRING, td.MCP_PARAM_QUALIFIED_NAME + ), + cs.MCPParamName.TARGET_MODULE: prop( + cs.MCPSchemaType.STRING, td.MCP_PARAM_TARGET_MODULE + ), + cs.MCPParamName.KEEP_ALIAS: prop( + cs.MCPSchemaType.BOOLEAN, td.MCP_PARAM_KEEP_ALIAS + ), + cs.MCPParamName.DRY_RUN: prop( + cs.MCPSchemaType.BOOLEAN, td.MCP_PARAM_RENAME_DRY_RUN + ), + cs.MCPParamName.PROJECT: prop( + cs.MCPSchemaType.STRING, td.MCP_PARAM_PROJECT + ), + }, + required=[ + cs.MCPParamName.QUALIFIED_NAME, + cs.MCPParamName.TARGET_MODULE, + ], + ), + handler=self.move, + returns_json=True, + ) + + async def move( + self, + qualified_name: str, + target_module: str, + keep_alias: bool = False, + dry_run: bool = False, + project: str | None = None, + ) -> object: + return await self._graph_query( + cs.MCPToolName.MOVE, + project, + lambda name: self._run_move( + name, qualified_name, target_module, keep_alias, dry_run + ), + ) + + def _run_move( + self, + project_name: str, + qualified_name: str, + target_module: str, + keep_alias: bool, + dry_run: bool, + ) -> object: + from codebase_rag.editing.move import MoveRefused, move + + try: + report = move( + Path(self.project_root), + self.ingestor.fetch_all, + project_name, + qualified_name, + target_module, + keep_alias=keep_alias, + dry_run=dry_run, + reingest=self._reingest_for_contract(project_name), + ) + except MoveRefused as refused: + return {cs.DICT_KEY_ERROR: str(refused), cs.KEY_CYCLE: list(refused.cycle)} + payload = dict(report._asdict()) + if report.verdict is not None: + payload[cs.KEY_VERDICT] = report.verdict._asdict() + return payload + def _reingest_for_contract( self, project_name: str ) -> Callable[[list[str]], ReingestReport] | None: - """The re-ingest an edit's postcondition contract measures against. + """The live updater's re-ingest for an edit's postcondition contract. Delegates to `_guarded_rename_reingest`, which returns None for a project with no graph to measure (issue #1531) and wraps the callback diff --git a/codebase_rag/structural_delta.py b/codebase_rag/structural_delta.py index c9275f2cdd..9aa08ef8b9 100644 --- a/codebase_rag/structural_delta.py +++ b/codebase_rag/structural_delta.py @@ -17,7 +17,9 @@ from __future__ import annotations +import ast import re +import textwrap import time from collections.abc import Callable, Iterable from pathlib import Path @@ -554,6 +556,37 @@ def _dangling( _VARIADIC = re.compile(r"(? bool: + """Whether a `def` header declares `*args` or keyword-only params. + + Parsed, not scanned. Scanning got both directions wrong, and this is + the sole suppressor of a too-many-arguments verdict, so each costs + something different: a false positive hides a real arity error, and a + false negative makes a CORRECT edit fail its postcondition and roll + back. + + A `*` inside a default (`b=2*3`) or a string (`doc='a*b'`) is not + `*args`, and a real `*rest` can follow a default that itself contains + a `)` -- which the old first-`)` cut discarded. `ast` is the right + oracle because it is the same parser that decides whether the call + raises at runtime. + + An unparseable header answers False, keeping the arity check ACTIVE: + for a suppressor, refusing to suppress is the safe direction. + """ + body = textwrap.dedent(header).strip() + if not body: + return False + try: + tree = ast.parse(body + "\n pass\n") + except SyntaxError: + return False + node = tree.body[0] if tree.body else None + if not isinstance(node, ast.FunctionDef | ast.AsyncFunctionDef): + return False + return node.args.vararg is not None or bool(node.args.kwonlyargs) + + def _is_variadic(definition: Definition, repo_root: Path | None) -> bool: """Whether the Python definition's header declares `*args`. @@ -577,12 +610,7 @@ def _is_variadic(definition: Definition, repo_root: Path | None) -> bool: header.append(line) if ")" in line: break - text = "\n".join(header) - open_at = text.find("(") - close_at = text.find(")", open_at + 1) - if open_at < 0 or close_at < 0: - return False - return _VARIADIC.search(text[open_at:close_at]) is not None + return _header_is_variadic("\n".join(header)) def _arity_verdict( diff --git a/codebase_rag/tests/move_helpers.py b/codebase_rag/tests/move_helpers.py new file mode 100644 index 0000000000..2741f822f1 --- /dev/null +++ b/codebase_rag/tests/move_helpers.py @@ -0,0 +1,79 @@ +from __future__ import annotations + +import subprocess +import sys +from pathlib import Path + +import pytest + +from codebase_rag.graph_updater import GraphUpdater +from codebase_rag.parser_loader import load_parsers +from evals.cgr_graph import _StatefulIngestor + +PROJECT = "move_fixture" +FIXTURE: dict[str, str] = { + "pkg/__init__.py": "", + "pkg/util.py": ( + "import os\n" + "from pathlib import Path\n\n\n" + "def other():\n return Path('.')\n\n\n" + "# joins with the platform separator\n" + 'def helper(a):\n """Join."""\n return os.sep.join(a)\n\n\n' + "def unrelated():\n return 1\n" + ), + "pkg/a.py": "from pkg.util import helper\n\n\ndef run():\n return helper(['x', 'y'])\n", + "pkg/b.py": "from pkg.util import helper, other\n\n\ndef go():\n return helper([]) + str(other())\n", + "pkg/c.py": "import pkg.util\n\n\ndef via_module():\n return pkg.util.helper(['q'])\n", + "tests/__init__.py": "", + "tests/test_app.py": ( + "import os\n\nfrom pkg.a import run\n\n\ndef test_run():\n assert run() == 'x' + os.sep + 'y'\n" + ), +} + + +def write_file(root: Path, rel: str, text: str) -> None: + path = root / rel + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text(text) + + +def index_repo(root: Path) -> tuple[_StatefulIngestor, GraphUpdater]: + parsers, queries = load_parsers() + store = _StatefulIngestor() + updater = GraphUpdater( + ingestor=store, + repo_path=root, + parsers=parsers, + queries=queries, + project_name=PROJECT, + ) + updater.run(force=True) + return store, updater + + +def materialise(temp_repo: Path, fixture: dict[str, str]) -> Path: + root = temp_repo / PROJECT + root.mkdir() + for rel, text in fixture.items(): + write_file(root, rel, text) + return root + + +@pytest.fixture(name="move_repo") +def _move_repo(temp_repo: Path) -> tuple[Path, _StatefulIngestor, GraphUpdater]: + root = materialise(temp_repo, FIXTURE) + store, updater = index_repo(root) + return root, store, updater + + +def qn(rel: str) -> str: + return f"{PROJECT}.{rel}" + + +def smoke(root: Path) -> None: + subprocess.run( + [sys.executable, "-m", "pytest", "-q", "-p", "no:cacheprovider", "tests"], + cwd=root, + check=True, + capture_output=True, + ) diff --git a/codebase_rag/tests/test_change_signature.py b/codebase_rag/tests/test_change_signature.py index 348717ebef..c0c334fbe9 100644 --- a/codebase_rag/tests/test_change_signature.py +++ b/codebase_rag/tests/test_change_signature.py @@ -173,7 +173,8 @@ def test_heuristic_site_is_listed_as_unmapped( and e[2] == "CALLS" and e[4] == _qn("pkg.util.helper") ) - store.edge_props[edge][cs.KEY_RESOLUTION] = cs.EdgeResolution.HEURISTIC.value + for site in store.sites_of(edge): + site[cs.KEY_RESOLUTION] = cs.EdgeResolution.HEURISTIC.value report = change_signature( root, store.fetch_all, diff --git a/codebase_rag/tests/test_move_import_presence.py b/codebase_rag/tests/test_move_import_presence.py new file mode 100644 index 0000000000..f058ad3d5e --- /dev/null +++ b/codebase_rag/tests/test_move_import_presence.py @@ -0,0 +1,50 @@ +"""`move` must not skip an import because the text appears somewhere. + +The move rewrites call sites to `pkg.new.helper(...)` and then adds +`import pkg.new` unless it is already there. "Already there" was decided +by a regex over the raw file, so an aliased import, a comment or a string +literal all counted -- and the import was never added while the call +sites had already been rewritten. Both files parse, so a parse-based +postcondition cannot catch it; the program dies at runtime with +`NameError: name 'pkg' is not defined` on the line the move just wrote. +""" + +from __future__ import annotations + +import pytest + +from codebase_rag.editing.move import _module_bound + +BINDS = [ + pytest.param("import pkg.new\n", id="plain"), + # `import pkg.new.sub` binds `pkg`, so `pkg.new.helper()` resolves. + # Verified against a real interpreter: it must stay True. + pytest.param("import pkg.new.sub\n", id="submodule-still-binds"), +] + +DOES_NOT_BIND = [ + pytest.param("import pkg.new as n\nn.helper()\n", id="aliased"), + pytest.param("# TODO: import pkg.new later\n", id="comment"), + pytest.param('DOC = "import pkg.new"\n', id="string-literal"), + pytest.param("helper()\n", id="absent"), + pytest.param("from pkg import new\n", id="from-import-binds-new-not-pkg"), +] + + +@pytest.mark.parametrize("source", BINDS) +def test_the_module_is_bound(source: str) -> None: + assert _module_bound(source, "pkg.new") is True + + +@pytest.mark.parametrize("source", DOES_NOT_BIND) +def test_the_module_is_not_bound(source: str) -> None: + assert _module_bound(source, "pkg.new") is False + + +def test_an_alias_does_not_count_as_the_import() -> None: + """The case that breaks working code, spelled out. + + `import pkg.new as n` binds only `n`. Treating it as the import means + the move emits `pkg.new.helper()` with `pkg` unbound. + """ + assert _module_bound("import pkg.new as n\n", "pkg.new") is False diff --git a/codebase_rag/tests/test_move_mcp.py b/codebase_rag/tests/test_move_mcp.py new file mode 100644 index 0000000000..15c9998d1b --- /dev/null +++ b/codebase_rag/tests/test_move_mcp.py @@ -0,0 +1,55 @@ +from __future__ import annotations + +import asyncio +from pathlib import Path +from unittest.mock import MagicMock + +from move_helpers import ( # noqa: F401 + PROJECT, + _move_repo, # noqa: F401 + qn, +) + +from codebase_rag import constants as cs +from codebase_rag.graph_updater import GraphUpdater +from codebase_rag.mcp.tools import MCPToolsRegistry +from evals.cgr_graph import _StatefulIngestor + + +def test_mcp_move_tool_reports_and_refuses( + move_repo: tuple[Path, _StatefulIngestor, GraphUpdater], +) -> None: + root, store, updater = move_repo + ingestor = MagicMock() + ingestor.fetch_all = store.fetch_all + ingestor.list_projects.return_value = [PROJECT] + registry = MCPToolsRegistry( + project_root=str(root), ingestor=ingestor, cypher_gen=MagicMock() + ) + registry._live_updater = updater + schema = next( + s for s in registry.get_tool_schemas() if s.name == cs.MCPToolName.MOVE + ) + assert set(schema.inputSchema["required"]) == { + cs.MCPParamName.QUALIFIED_NAME, + cs.MCPParamName.TARGET_MODULE, + } + refused = asyncio.run( + registry.move( + qualified_name=qn("pkg.util.helper"), + target_module="pkg.util", + project=PROJECT, + ) + ) + assert isinstance(refused, dict) and cs.DICT_KEY_ERROR in refused + payload = asyncio.run( + registry.move( + qualified_name=qn("pkg.util.helper"), + target_module="pkg.core", + project=PROJECT, + ) + ) + assert isinstance(payload, dict) + assert payload["applied"] is True + assert payload["new_qualified_name"] == qn("pkg.core.helper") + assert payload[cs.KEY_VERDICT]["ok"] is True diff --git a/codebase_rag/tests/test_move_operation.py b/codebase_rag/tests/test_move_operation.py new file mode 100644 index 0000000000..e21b6523e5 --- /dev/null +++ b/codebase_rag/tests/test_move_operation.py @@ -0,0 +1,201 @@ +from __future__ import annotations + +import subprocess +import sys +from pathlib import Path + +import pytest +from move_helpers import ( + FIXTURE, + PROJECT, + _move_repo, # noqa: F401 + index_repo, + materialise, + qn, + smoke, + write_file, +) + +from codebase_rag import constants as cs +from codebase_rag.editing import MoveRefused, move +from codebase_rag.editing.transaction import load_history +from codebase_rag.graph_updater import GraphUpdater +from evals.cgr_graph import _StatefulIngestor + + +def test_move_updates_three_importers_without_a_cycle( + move_repo: tuple[Path, _StatefulIngestor, GraphUpdater], +) -> None: + root, store, updater = move_repo + report = move( + root, + store.fetch_all, + PROJECT, + qn("pkg.util.helper"), + "pkg.core", + reingest=updater.reingest, + ) + assert report.applied, report.message + assert report.new_qualified_name == qn("pkg.core.helper") + core = (root / "pkg/core.py").read_text() + # The definition travelled with its comment, docstring and the one + # import it needs; the unrelated import stayed behind. + assert core == ( + "import os\n\n\n" + "# joins with the platform separator\n" + 'def helper(a):\n """Join."""\n return os.sep.join(a)\n' + ) + util = (root / "pkg/util.py").read_text() + assert "def helper" not in util and "def other" in util and "def unrelated" in util + assert "from pathlib import Path" in util + assert (root / "pkg/a.py").read_text().startswith("from pkg.core import helper\n") + assert ( + (root / "pkg/b.py") + .read_text() + .startswith("from pkg.util import other\nfrom pkg.core import helper\n") + ) + assert set(report.importers) == {"pkg/a.py", "pkg/b.py"} + # `pkg.util.helper(...)` through a module import is not an import of + # the symbol: it is left alone and named. + assert report.unchanged_importers == () + assert report.copied_imports == ("import os",) + assert report.verdict is not None and report.verdict.ok + assert [t["qualified_name"] for t in report.verdict.affected_tests] == [ + qn("tests.test_app.test_run") + ] + smoke(root) + + +def test_keep_alias_leaves_a_working_re_export( + move_repo: tuple[Path, _StatefulIngestor, GraphUpdater], +) -> None: + root, store, updater = move_repo + report = move( + root, + store.fetch_all, + PROJECT, + qn("pkg.util.helper"), + "pkg/core.py", + keep_alias=True, + reingest=updater.reingest, + ) + assert report.applied, report.message + util = (root / "pkg/util.py").read_text() + assert "from pkg.core import helper # noqa: F401" in util + # The old import path still works, and so does the module-attribute use. + probe = subprocess.run( + [ + sys.executable, + "-c", + "from pkg.util import helper; import pkg.c; print(helper(['a', 'b']), pkg.c.via_module())", + ], + cwd=root, + check=True, + capture_output=True, + encoding=cs.ENCODING_UTF8, + ) + assert "q" in probe.stdout + smoke(root) + + +def test_move_that_would_create_a_cycle_is_refused_before_writing( + temp_repo: Path, +) -> None: + fixture = dict(FIXTURE) + # helper needs `other` from util, and util's own code still calls helper: + # after the move util imports core and core imports util. + fixture["pkg/util.py"] = ( + "import os\n\n\n" + "def other():\n return 'o'\n\n\n" + "def helper(a):\n return other() + os.sep.join(a)\n\n\n" + "def run():\n return helper(['x'])\n" + ) + root = materialise(temp_repo, fixture) + store, updater = index_repo(root) + before = {rel: (root / rel).read_text() for rel in fixture} + with pytest.raises(MoveRefused) as excinfo: + move( + root, + store.fetch_all, + PROJECT, + qn("pkg.util.helper"), + "pkg.core", + reingest=updater.reingest, + ) + assert excinfo.value.cycle == (qn("pkg.core"), qn("pkg.util")) + assert "import cycle" in str(excinfo.value) + for rel, text in before.items(): + assert (root / rel).read_text() == text + assert not (root / "pkg/core.py").exists() + assert load_history(root) == [] + + +def test_move_into_an_existing_module_appends( + move_repo: tuple[Path, _StatefulIngestor, GraphUpdater], +) -> None: + root, store, updater = move_repo + write_file(root, "pkg/core.py", "VERSION = 1\n") + updater.reingest(["pkg/core.py"]) + report = move( + root, + store.fetch_all, + PROJECT, + qn("pkg.util.helper"), + "pkg.core", + reingest=updater.reingest, + ) + assert report.applied, report.message + core = (root / "pkg/core.py").read_text() + assert core.startswith("VERSION = 1\n\n\nimport os\n\n\n# joins") + smoke(root) + + +def test_old_module_that_still_uses_the_name_imports_it(temp_repo: Path) -> None: + fixture = dict(FIXTURE) + fixture["pkg/util.py"] = ( + FIXTURE["pkg/util.py"] + "\n\ndef run():\n return helper(['x'])\n" + ) + root = materialise(temp_repo, fixture) + store, updater = index_repo(root) + report = move( + root, + store.fetch_all, + PROJECT, + qn("pkg.util.helper"), + "pkg.core", + reingest=updater.reingest, + ) + assert report.applied, report.message + util = (root / "pkg/util.py").read_text() + assert util.startswith( + "import os\nfrom pathlib import Path\nfrom pkg.core import helper\n" + ) + assert "def run():\n return helper(['x'])" in util + smoke(root) + + +def test_refusals(move_repo: tuple[Path, _StatefulIngestor, GraphUpdater]) -> None: + root, store, _updater = move_repo + with pytest.raises(MoveRefused, match="No definition"): + move( + root, + store.fetch_all, + PROJECT, + qn("pkg.util.nothing"), + "pkg.core", + dry_run=True, + ) + with pytest.raises(MoveRefused, match="already holds"): + move( + root, + store.fetch_all, + PROJECT, + qn("pkg.util.helper"), + "pkg.util", + dry_run=True, + ) + report = move( + root, store.fetch_all, PROJECT, qn("pkg.util.helper"), "pkg.core", dry_run=True + ) + assert not report.applied and report.new_path == "pkg/core.py" + assert (root / "pkg/util.py").read_text() == FIXTURE["pkg/util.py"] diff --git a/codebase_rag/tests/test_variadic_header_detection.py b/codebase_rag/tests/test_variadic_header_detection.py new file mode 100644 index 0000000000..4e437d6c35 --- /dev/null +++ b/codebase_rag/tests/test_variadic_header_detection.py @@ -0,0 +1,78 @@ +"""`*args` detection must parse the header, not scan it. + +`_is_variadic` is the SOLE suppressor of a too-many-arguments verdict, so +both error directions do damage: a false positive hides a real arity +error, and a false negative makes a CORRECT edit fail its own +postcondition and roll back. + +Scanning raw text got both wrong. A `*` inside a default (`b=2*3`) or a +string (`doc='a*b'`) reads as `*args`, and the header window was cut at +the first `)`, which can fall inside a default -- hiding a real `*rest` +that comes after it. + +`ast` is the ground truth here: it is the same parser that decides +whether the call actually raises at runtime. +""" + +from __future__ import annotations + +import ast + +import pytest + +from codebase_rag.structural_delta import _header_is_variadic + + +def _truth(header: str) -> bool: + """What Python itself says about the header.""" + fn = ast.parse(header + "\n pass\n").body[0] + assert isinstance(fn, ast.FunctionDef) + return fn.args.vararg is not None or bool(fn.args.kwonlyargs) + + +HEADERS = [ + pytest.param("def f(a):", id="plain"), + pytest.param("def f(a, *rest):", id="star-args"), + pytest.param("def f(a, *, k):", id="bare-star-keyword-only"), + pytest.param("def f(*args, **kw):", id="star-and-kwargs"), + # The regex read the `*` in the default as `*args`. + pytest.param("def f(a, b=2*3):", id="star-in-arithmetic-default"), + pytest.param("def f(a, doc='a*b'):", id="star-inside-a-string"), + # `find(")")` stopped inside the default, hiding the real `*rest`. + pytest.param("def f(a=(1, 2), *rest):", id="star-after-tuple-default"), + pytest.param("def f(a=g(), *rest):", id="star-after-call-default"), + pytest.param("def f(a, cb=lambda x: (x)):", id="lambda-default"), +] + + +@pytest.mark.parametrize("header", HEADERS) +def test_it_agrees_with_python(header: str) -> None: + assert _header_is_variadic(header) is _truth(header) + + +def test_a_star_in_a_default_is_not_varargs() -> None: + """False positive: silently suppressed a real too-many-arguments verdict.""" + assert _header_is_variadic("def f(a, b=2*3):") is False + + +def test_a_star_after_a_tuple_default_is_varargs() -> None: + """False negative: made a correct edit roll back.""" + assert _header_is_variadic("def f(a=(1, 2), *rest):") is True + + +def test_a_multibyte_identifier_is_handled() -> None: + """A valid non-ASCII name must not break the parse. + + `café` is a legal Python identifier whose `é` is two UTF-8 bytes, + neither valid alone -- so any byte-wise handling of the header + misreads it. (`©` is NOT a legal identifier character, so it tests + the parser's rejection rather than multi-byte handling.) + """ + assert _header_is_variadic("def café(a, *rest):") is True + assert _header_is_variadic("def café(a):") is False + + +def test_an_unparseable_header_is_not_variadic() -> None: + # Refuse to guess: not-variadic keeps the arity check ACTIVE, which + # is the safe direction for a suppressor. + assert _header_is_variadic("def f(a, ") is False diff --git a/codebase_rag/tools/tool_descriptions.py b/codebase_rag/tools/tool_descriptions.py index 616d50a953..be4193af9b 100644 --- a/codebase_rag/tools/tool_descriptions.py +++ b/codebase_rag/tools/tool_descriptions.py @@ -303,6 +303,24 @@ class AgenticToolName(StrEnum): "one transaction and held to the postcondition contract; set `dry_run` to " "see the plan." ) +MCP_MOVE = ( + "Move a module-level function or class to another module. The definition " + "(decorators, docstring, adjacent comments) is cut and pasted into " + "`target_module` (a dotted module name or a repo-relative path; created " + "when missing) with the imports it needs, every importer is rewritten, " + "uses through a module import (`pkg.util.helper`) follow, and the old " + "module keeps an import of the name when it still uses it. `keep_alias` " + "leaves a re-export at the old location so the old import path keeps " + "working. Refused before any file is touched when the move would create " + "an import cycle, naming the cycle. One transaction, held to the " + "postcondition contract; set `dry_run` to see the plan." +) +MCP_PARAM_TARGET_MODULE = ( + "Destination module: a dotted module name (`pkg.core`) or a repo-relative path." +) +MCP_PARAM_KEEP_ALIAS = ( + "Leave a re-export at the old location so the old import path still works." +) MCP_PARAM_NEW_PARAMS = ( "The new parameter list, in order, as `name[:annotation][=default][@source]` specs." ) @@ -493,6 +511,7 @@ class AgenticToolName(StrEnum): MCPToolName.GLOSSES: MCP_GLOSSES, MCPToolName.RENAME: MCP_RENAME, MCPToolName.CHANGE_SIGNATURE: MCP_CHANGE_SIGNATURE, + MCPToolName.MOVE: MCP_MOVE, MCPToolName.QUERY_CODE_GRAPH: MCP_QUERY_CODE_GRAPH, MCPToolName.GET_CODE_SNIPPET: MCP_GET_CODE_SNIPPET, MCPToolName.SURGICAL_REPLACE_CODE: MCP_SURGICAL_REPLACE_CODE, diff --git a/evals/cgr_graph.py b/evals/cgr_graph.py index 7a99633d0d..c1a7207162 100644 --- a/evals/cgr_graph.py +++ b/evals/cgr_graph.py @@ -206,6 +206,10 @@ def __init__(self) -> None: self.nodes: dict[_NodeId, PropertyDict] = {} self.edges: set[_RelTuple] = set() self.edge_props: dict[_RelTuple, PropertyDict] = {} + # Every distinct site of an endpoint pair (issue #1522): the real + # store keeps one edge per site; `edge_props` keeps the last one + # for the callers that only need the pair. + self.edge_sites: dict[_RelTuple, list[PropertyDict]] = {} # Adjacency indexes so a subtree delete costs the subtree, not a # scan of every edge per node (the real store has indexes too). self._out: dict[_NodeId, set[_RelTuple]] = {} @@ -232,6 +236,13 @@ def ensure_relationship_batch( self._in.setdefault((str(to_label), to_val), set()).add(edge) if properties: self.edge_props[edge] = dict(properties) + sites = self.edge_sites.setdefault(edge, []) + if dict(properties) not in sites: + sites.append(dict(properties)) + + def sites_of(self, edge: _RelTuple) -> list[PropertyDict]: + """Every site of an edge (one empty record for a site-less edge).""" + return self.edge_sites.get(edge) or [self.edge_props.get(edge, {})] # --- structural delta reads (issue #1525) -------------------------------- @@ -310,20 +321,20 @@ def _delta_sites(self, prefix: str, paths: set[str]) -> list[ResultRow]: to_path = self._delta_path_of(tl, tv) if from_path not in paths and to_path not in paths: continue - props = self.edge_props.get(edge, {}) - rows.append( - { - cs.KEY_FROM_QN: _result(fv), - cs.KEY_FROM_PATH: _result(from_path), - cs.KEY_REL_TYPE: _result(rel), - cs.KEY_TO_QN: _result(tv), - cs.KEY_TO_PATH: _result(to_path), - cs.KEY_LINE: _result(props.get(cs.KEY_LINE)), - cs.KEY_COL: _result(props.get(cs.KEY_COL)), - cs.KEY_ARG_COUNT: _result(props.get(cs.KEY_ARG_COUNT)), - cs.KEY_KWARG_NAMES: _result(props.get(cs.KEY_KWARG_NAMES)), - } - ) + for props in self.sites_of(edge): + rows.append( + { + cs.KEY_FROM_QN: _result(fv), + cs.KEY_FROM_PATH: _result(from_path), + cs.KEY_REL_TYPE: _result(rel), + cs.KEY_TO_QN: _result(tv), + cs.KEY_TO_PATH: _result(to_path), + cs.KEY_LINE: _result(props.get(cs.KEY_LINE)), + cs.KEY_COL: _result(props.get(cs.KEY_COL)), + cs.KEY_ARG_COUNT: _result(props.get(cs.KEY_ARG_COUNT)), + cs.KEY_KWARG_NAMES: _result(props.get(cs.KEY_KWARG_NAMES)), + } + ) return rows def _delta_callers_of(self, prefix: str, qns: set[str]) -> list[ResultRow]: @@ -424,21 +435,23 @@ def _graph_node_ids(self, qn: str) -> list[_NodeId]: if uid == qn and label not in (_FILE_LABEL, _FOLDER_LABEL) ] - def _graph_edge_row( + def _graph_edge_rows( self, edge: _RelTuple, keys: tuple[str, ...], node: _NodeId - ) -> ResultRow: + ) -> list[ResultRow]: label, uid = node props = self.nodes.get(node, {}) - row: ResultRow = { - cs.KEY_LABEL: label, - cs.KEY_QUALIFIED_NAME: _result(uid), - cs.KEY_PATH: _result(props.get(cs.KEY_PATH)), - cs.KEY_REL_TYPE: edge[2], - } - edge_props = self.edge_props.get(edge, {}) - for key in keys: - row[key] = _result(edge_props.get(key)) - return row + rows: list[ResultRow] = [] + for edge_props in self.sites_of(edge): + row: ResultRow = { + cs.KEY_LABEL: label, + cs.KEY_QUALIFIED_NAME: _result(uid), + cs.KEY_PATH: _result(props.get(cs.KEY_PATH)), + cs.KEY_REL_TYPE: edge[2], + } + for key in keys: + row[key] = _result(edge_props.get(key)) + rows.append(row) + return rows def _project_root_rows(self, params: PropertyDict) -> list[ResultRow]: """The Project node's stored root, as `source_root_for` reads it.""" @@ -494,18 +507,27 @@ def _graph_rows(self, query: str, params: PropertyDict) -> list[ResultRow]: for edge in self._in.get(target, ()): source = (edge[0], edge[1]) if edge[2] in wanted and source in self.nodes: - rows.append( - self._graph_edge_row(edge, self._GRAPH_SITE_KEYS, source) + rows.extend( + self._graph_edge_rows(edge, self._GRAPH_SITE_KEYS, source) ) elif query == cq.CYPHER_GRAPH_OVERRIDES: overrides = cs.RelationshipType.OVERRIDES.value for target in targets: for edge in self._in.get(target, ()): if edge[2] == overrides and (edge[0], edge[1]) in self.nodes: - rows.append(self._graph_edge_row(edge, (), (edge[0], edge[1]))) + rows.extend(self._graph_edge_rows(edge, (), (edge[0], edge[1]))) for edge in self._out.get(target, ()): if edge[2] == overrides and (edge[3], edge[4]) in self.nodes: - rows.append(self._graph_edge_row(edge, (), (edge[3], edge[4]))) + rows.extend(self._graph_edge_rows(edge, (), (edge[3], edge[4]))) + elif query == cq.CYPHER_GRAPH_IMPORTS_OF: + for target in targets: + for edge in self._out.get(target, ()): + if edge[2] == cs.RelationshipType.IMPORTS.value: + for row in self._graph_edge_rows( + edge, self._GRAPH_IMPORT_KEYS, (edge[3], edge[4]) + ): + row[cs.KEY_TO_QN] = _result(edge[4]) + rows.append(row) elif query == cq.CYPHER_GRAPH_IMPORTERS: for target in targets: for edge in self._in.get(target, ()): @@ -514,8 +536,8 @@ def _graph_rows(self, query: str, params: PropertyDict) -> list[ResultRow]: edge[2] == cs.RelationshipType.IMPORTS.value and source in self.nodes ): - rows.append( - self._graph_edge_row(edge, self._GRAPH_IMPORT_KEYS, source) + rows.extend( + self._graph_edge_rows(edge, self._GRAPH_IMPORT_KEYS, source) ) return rows @@ -580,6 +602,7 @@ def _delta_rows(self, query: str, params: PropertyDict) -> list[ResultRow]: def reset_edges(self) -> None: self.edges = set() self.edge_props = {} + self.edge_sites = {} self._out = {} self._in = {} @@ -615,6 +638,7 @@ def fetch_all( | cq.CYPHER_GRAPH_OVERRIDES | cq.CYPHER_GRAPH_IMPORTERS | cq.CYPHER_GRAPH_SIGNATURE + | cq.CYPHER_GRAPH_IMPORTS_OF ): return self._graph_rows(query, params or {}) case cs.CYPHER_ALL_FOLDER_PATHS: @@ -1158,6 +1182,7 @@ def _detach_delete(self, doomed: set[_NodeId]) -> None: for edge in touched: self.edges.discard(edge) self.edge_props.pop(edge, None) + self.edge_sites.pop(edge, None) self._out.get((edge[0], edge[1]), set()).discard(edge) self._in.get((edge[3], edge[4]), set()).discard(edge)