Repository navigation
Conversation
|
Warning Review limit reachedNext included review available in 25 minutes. View limit detailsLimit details: You’ve used all 8 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Repository: vitali87/code-graph-rag/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| 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 |
There was a problem hiding this comment.
_module_bound walks every AST scope, so an unaliased import inside a function, class, or conditional is treated as a module-level binding. Move planning can then skip adding the required top-level import even though rewritten module-level references cannot access that nested binding. The moved code subsequently fails with NameError when it evaluates the rewritten reference.
Artifacts
- Self-contained harness that AST-extracts and executes the exact changed helper, then runs the broken and repaired rewritten-consumer paths; it provides a reproducible proof of the finding.
- Executed broken planning scenario showing all three nested scopes return True, the planner does not add an import, and the rewritten consumer raises NameError; the claim holds.
- Executed counterpart with the required module-level import, where the same rewritten consumer completes successfully; the missing top-level binding is the failure cause.
- Shows `_module_bound` is imported by the move entry module and records that the requested virtualenv interpreter was absent; the helper is reachable from move planning.
- Captured combined command output for compilation plus both executed scenarios; the broken path fails at runtime and the repaired path succeeds.
| 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]}" |
There was a problem hiding this comment.
When moved code crosses Python package boundaries, this branch copies its relative import spelling unchanged. For example, moving from .alpha import value from app.src to app.dst makes Python resolve app.dst.alpha rather than the original app.src.alpha, so the moved definition can import the wrong dependency or fail when the destination module does not exist.
Artifacts
- Authored and executed script that loads the checked-out helper and imports temporary source and destination packages, demonstrating the relocation semantics.
- Executed control run uses `from ..src.alpha import value` after relocation and imports the source-package sentinel successfully, establishing the expected behavior.
Error trace from the failing run
- Executed candidate run shows the helper preserving `from .alpha import value`, importing the destination-package sentinel, and failing the source-package assertion.
| 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 |
There was a problem hiding this comment.
When a moved definition needs client from import client, { helper } from "./api", this code searches only the named-import group and returns None. The destination therefore receives no import for the required default binding, leaving the moved JavaScript or TypeScript definition unresolved.
Artifacts
- This authored harness loads the exact on-disk helper and invokes it for `client` in a mixed import, providing the executable reproduction.
- The captured command output shows the exact helper path, mixed import input, selected `client` alias, and `result=None`, proving the binding is dropped.
- This authored control harness invokes the same exact helper for the named `helper` binding in the same mixed import.
- The captured control output returns a rewritten import for `helper`, isolating the defect to selecting the default binding.
| if _PY_IMPORT.match(statement): | ||
| return statement.strip() | ||
| return statement.strip() |
There was a problem hiding this comment.
Non-blocking concern: selecting a from import a, b returns the entire statement instead of retaining only a. The destination unnecessarily imports b, which can add avoidable import-time work or bindings and defeats the helper's intended import reduction.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Artifacts
- Authored harness loads the exact checked-out helper and compares its control and candidate behavior, proving the candidate path is exercised.
- Ran the harness control case against the exact helper; it narrowed `from pkg import a, b` to `from pkg import a`, establishing the intended comparison behavior.
Error trace from the failing run
- Ran the harness candidate case against the exact helper; it returned `import a, b` instead of `import a` and raised the expected assertion, proving the defect.
- Attempted the required repository virtual-environment interpreter; its dangling target caused a not-found error, so the exact-source harness used the available interpreter instead.
Comments Outside DiffThese findings sit on lines the diff does not cover, so they could not be posted inline. Each one leaves this list once its file changes.
|
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Critical and moderate import-handling issues must be addressed before approval.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 3
Open (6)
What changed in this PR
Adds parsing and import-narrowing helpers for the move refactoring workflow.
Changes:
- Selects definitions and cut spans with tree-sitter.
- Adds module binding and import insertion-point helpers.
- Narrows Python, JavaScript, and TypeScript imports.
| File | Summary | Status |
|---|---|---|
codebase_rag/editing/move_scope.py |
Implements move span, binding, and import helpers. | Unresolved critical and moderate findings remain. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
|
||
| def _import_block_end(source: bytes, root: Node) -> int: | ||
| """Byte offset just after the last top-level import (0 when none).""" | ||
| end = 0 |
| 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() |
| from .move_types import _Cut | ||
|
|
||
| _IDENTIFIER = r"(?<![\w.])%s(?!\w)" | ||
| _JS_LANGUAGES = frozenset({cs.SupportedLanguage.JS, cs.SupportedLanguage.TS}) |
| 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 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: |
|
Closing as superseded: this is part of the split of #1549, which was closed without merging, and it targets the stale Generated by Claude Code |
Add tree-sitter span selection, module binding, and import-statement narrowing helpers for move.
Files changed
codebase_rag/editing/move_scope.pyDiff stats
+185 additions, -0 deletions (185 LOC)
Dependencies
This PR depends on:
pr-1Dependency graph
Merge in this order: