Repository navigation
Conversation
|
Warning Review limit reachedNext included review available in 8 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 (11)
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}
| spec.from_index != index for index, spec in enumerate(kept) | ||
| ): | ||
| raise SignatureRefused(cs.SIGNATURE_POSITIONAL_ONLY_MOVED) | ||
| rendered.insert(receivers + boundary, "/") |
There was a problem hiding this comment.
For def m(self, a, /, b), the rewrite counts self in the positional-only boundary even though the requested parameter specs exclude the receiver. It rewrites the unchanged signature as def m(self, a, b, /), making b positional-only. Existing calls such as C().m(1, b=2) then raise TypeError.
Artifacts
- Authored script that loads the checkout implementation, invokes rewrite_definition with unchanged specs, and executes both original and rewritten methods; it reproduces the contract change.
- Captured command output for the original `def m(self, a, /, b)` showing `C().m(1, b=2)` returned 3; the original contract permits the keyword.
- Captured command output after executing rewrite_definition showing `def m(self, a, b, /)` parses but `C().m(1, b=2)` raises TypeError; the rewrite changes the contract.
| rendered.append("*") | ||
| rendered.extend(_text(p.node) for p in keyword_only) | ||
| text = "(" + ", ".join(rendered) + ")" | ||
| patcher.replace_span(path, (params_node.start_byte, params_node.end_byte), text) |
There was a problem hiding this comment.
Rebuilding the complete parameter span drops comments between parameters even when every parameter is retained. For example, def f(a, # explanation\n b) becomes def f(a, b). This is non-blocking, but a routine signature edit silently removes user-authored documentation.
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
- This script parses an in-memory Python definition, calls the production signature rewrite with both original parameters retained, and asserts that the comment disappears, proving the focused execution path.
- This captured command output shows the actual before and after definition, confirms both parameters were retained and the comment was absent afterward, and ends with a zero exit code.
| values: dict[int, tuple[str, bool]] = {} | ||
| for index, node in enumerate(positional): | ||
| values[index] = (_text(node), False) |
There was a problem hiding this comment.
An inline Python comment is assigned a positional argument index. For f(a, # note\n b), the mapper consumes a and the comment, then reports b as a surplus argument instead of rewriting the otherwise unchanged call. This is non-blocking, but it prevents signature changes from updating valid commented call sites.
Artifacts
- This authored script parses both call forms with the checkout's Tree-sitter dependencies and invokes `_split_call_arguments` and `_map_arguments`, demonstrating the mapping outcome.
- The executed baseline command maps `f(a, b)` with unchanged two-parameter specs to `['a', 'b']` and exits zero, establishing expected behavior.
- The executed commented-call command shows the comment in `named_children` and `positional`, then returns the surplus-argument reason with passing assertions, proving the false refusal.
| def _text(node: Node | None) -> str: | ||
| if node is None or node.text is None: | ||
| return "" | ||
| return node.text.decode(cs.ENCODING_UTF8, errors="replace") |
There was a problem hiding this comment.
The new _text helper extracts and UTF-8-decodes tree-sitter node source, but its name does not describe that behavior or return value. This violates the repository directive requiring function names to clearly describe what they return and do. Rename it clearly, and apply the same requirement to similarly generic new helpers such as _call_at and _compatible. This repository requirement must be satisfied before merging.
Rule Used: Use descriptive function names that clearly indicate what the function returns and its purpose. Prefer names like _get_grouping_key over _find_matching_pattern when the function's primary purpose is to return a value for a specific use case. (source)
Learned From
vitali87/code-graph-rag#221
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!
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
There is a confirmed correctness bug in Python positional-only (/) restoration when receiver parameters exist, which can refuse valid rewrites or place the separator incorrectly.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Open (3)
What changed in this PR
This PR introduces the core helper modules for a forthcoming change_signature edit operation, including parameter spec parsing, definition/call-site rewrite helpers, and supporting constants, plus a small extension to the postcondition contract to record whether heuristic resolutions were allowed.
Changes:
- Add new
change_signaturedata types and parsing utilities (parameter specs, reporting types). - Add signature rewrite helpers for definitions and call sites (argument remapping, positional-only restoration, literal/type helpers).
- Extend contract + constants to support
change_signatureexpectations and MCP/CLI wiring.
| File | Description |
|---|---|
| codebase_rag/editing/contract.py | Adds heuristic_allowed support to change_signature_expectation. |
| codebase_rag/editing/signature_rewrite.py | New helpers to rewrite definitions and call sites based on parameter specs. |
| codebase_rag/editing/signature_types.py | New public types and CLI/MCP parameter spec parsing for change_signature. |
| codebase_rag/editing/signature_params.py | New parameter extraction/rendering logic, including Python positional-only handling. |
| codebase_rag/editing/signature_calls.py | New call-site argument mapping logic for change_signature. |
| codebase_rag/editing/signature_literals.py | New literal/type-compatibility helpers for default validation. |
| codebase_rag/constants/mcp.py | Adds MCP tool/param constants for change_signature. |
| codebase_rag/constants/graph.py | Adds graph keys for unmapped/verdict reporting. |
| codebase_rag/constants/cli.py | Adds user-facing CLI error/status messages for change_signature. |
| codebase_rag/constants/ast_python.py | Adds tree-sitter node constants for Python call-site splats. |
| codebase_rag/constants/ast_nodes.py | Adds tree-sitter node constant for spread_element. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| boundary = _positional_only_count(node) | ||
| if boundary is None: | ||
| return | ||
| receivers = sum(1 for p in old if p.receiver) | ||
| kept = specs[:boundary] | ||
| if len(specs) < boundary or any( | ||
| spec.from_index != index for index, spec in enumerate(kept) | ||
| ): | ||
| raise SignatureRefused(cs.SIGNATURE_POSITIONAL_ONLY_MOVED) | ||
| rendered.insert(receivers + boundary, "/") |
| def change_signature_expectation( | ||
| unmapped: Iterable[str], heuristic_allowed: bool = False | ||
| ) -> Expectation: | ||
| return Expectation( | ||
| operation=cs.CONTRACT_OP_CHANGE_SIGNATURE, | ||
| unmapped=tuple(sorted(set(unmapped))), | ||
| # A parameter added with a default changes no caller count. | ||
| caller_count_unchanged=True, | ||
| heuristic_allowed=heuristic_allowed, | ||
| ) |
| def rewrite_site( | ||
| patcher: Patcher, | ||
| row: graph_query.CallSiteRow | ResultRow, | ||
| owner_qn: str, | ||
| old_names: list[str], | ||
| specs: list[ParamSpec], |
|
Evidence that retargeting this PR to This PR targets The check that matters: The base branch tip is fully contained in Two consequences worth noting:
|
Rewrite helpers plus postcondition contract expectation support.
Files changed
codebase_rag/editing/contract.pycodebase_rag/editing/signature_rewrite.pyDiff stats
+143 additions, -1 deletions (144 LOC)
Dependencies
This PR depends on:
pr-1,pr-2,pr-3Dependency graph
Merge in this order: