Repository navigation
Conversation
|
Warning Review limit reachedNext included review available in 2 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 (4)
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}
| tx.stage(report.new_path, new_content) | ||
| broken = [key for key, result in results.items() if result.parses is False] |
There was a problem hiding this comment.
When the destination does not exist, its generated content is staged directly with tx.stage, but the parse-failure check only covers files returned by patcher.stage_into. Moving Python code to a new path such as pkg/target.go therefore commits unchecked Python text as a Go file. Validate the new destination using its target grammar, or reject incompatible target grammars before committing.
Artifacts
- A minimal executable harness creates a Python source file, moves its function to a new Go destination, asserts the commit succeeds, and checks the destination with gofmt; it is the executed source proving the issue.
- Command output captures the exact executed harness and SHA-256 digest; it identifies the source used for the behavioral check.
- Captured source excerpt shows `new_content` is staged at line 312 after `patcher.stage_into`, while line 313 checks only Patcher results; it establishes the bypass condition.
- Executed harness output records a successful two-file transaction that creates Python content in `pkg/target.go`; gofmt rejects it, proving unchecked invalid destination code was committed.
| patcher.replace_span( | ||
| new_path, (len(existing), len(existing)), sep + paste.lstrip("\n") | ||
| ) |
There was a problem hiding this comment.
If the destination already defines the same top-level name, this code appends the moved definition without checking for a collision. In Python, the new definition silently replaces the destination module's prior binding at runtime; in languages that reject duplicate declarations, this breaks compilation. Refuse the move or resolve the existing definition before appending.
| ) | ||
| if verdict.ok: | ||
| return report._replace(verdict=verdict) | ||
| undo_last(self.repo_root) |
There was a problem hiding this comment.
When postcondition enforcement fails, this calls undo_last rather than undoing report.transaction_id. Because the commit lock is released before reingestion and contract verification, another transaction can commit in between. The failure path then reverts that unrelated edit while leaving the invalid move applied. Roll back the recorded move transaction explicitly and report a conflict when it is no longer the newest transaction.
Artifacts
- Executable focused harness that loads the candidate `_enforce_contract` implementation, uses the real transaction/history implementation, forces a failed postcondition after a newer commit, and asserts resulting file/history state.
- Successful candidate-path execution. It shows the move remained applied, the newer transaction was reverted, and only the move transaction remained in history.
- Successful comparison execution using transaction-ID-targeted undo. It shows refusal because the move was not newest, preserving the newer transaction.
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
Planning currently swallows non-“missing file” patch errors (e.g., outside-repo paths) and _add_import can insert before Python docstrings/shebangs, both of which can break correctness.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Open (3)
What changed in this PR
Implements the core “move refactoring” operation: it plans a move of a definition between modules, stages the required file edits transactionally (cut/paste + import retargeting), and optionally enforces a postcondition via re-ingest + contract verification.
Changes:
- Added
Mover.plan()to compute and stage a move (including cycle refusal and importer retargeting). - Added
Mover.apply()to commit staged edits viaEditTransactionand surface parse-failure rollback. - Added contract enforcement path to validate the move’s graph delta and revert on postcondition failure.
| File | Description |
|---|---|
codebase_rag/editing/move_operation.py |
New move operation implementation (planning, patch staging, transactional commit, and postcondition enforcement). |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| 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`). |
| at = _import_block_end(source, root) | ||
| if language in _JS_LANGUAGES: |
| 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 |
Two failures on #2074, unrelated to each other. `test_every_graph_reader_is_guarded` named `move`. The enum member lands on this layer ahead of its MCP handler, so nothing dispatches it and it reads nothing yet. Classified as a non-reader beside RENAME and CHANGE_SIGNATURE, which e5e9f1c settled: these are graph-driven EDITS that run their own re-ingest behind the incomplete-run marker, so refusing up front on a partial graph would block the operation that repairs it. When the handler lands it routes through `_graph_query` and must move to `_READS_THE_GRAPH`, or the dispatcher skips its refusal. `test_extra_positional_to_a_keyword_only_callee_is_too_many`: the too-many-positionals suppressor counted `kwonlyargs`. Keyword-only parameters absorb no surplus positional -- CPython raises TypeError -- so a real verdict was dropped. Same fix as #2107, which carries the full reasoning; the oracle here restated the buggy expression too, so it is replaced with one that decides by calling the function.
Co-authored-by: vitali87 <22404335+vitali87@users.noreply.github.com>
Co-authored-by: vitali87 <22404335+vitali87@users.noreply.github.com>
fix: classify move, and stop kwonly params suppressing TOO_MANY
|
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 |
Implement move planning, transactional application, and postcondition enforcement.
Files changed
codebase_rag/editing/move_operation.pyDiff stats
+372 additions, -0 deletions (372 LOC)
Dependencies
This PR depends on:
pr-5Dependency graph
Merge in this order: