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 (14)
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}
| A keyword naming a keyword-only parameter is not part of the positional | ||
| mapping and is carried through exactly as written. | ||
| """ | ||
| positional, keyword = _split_call_arguments(args_node) |
There was a problem hiding this comment.
For F(y: 2, x: 1), the changed mapping receives 2 and 1 as positional arguments after the C# argument wrappers discard y: and x:. Reordering the declaration from (x, y) to (y, x) then emits F(1, 2), which reverses the original bindings and changes program behavior. Preserve the C# argument names during mapping, or refuse to rewrite named C# call sites. This must be fixed before merging.
Knowledge Base Used: Multi-language parsing (codebase_rag/parsers)
Artifacts
- Authored harness extracts and executes the changed splitter and mapper against the C# named-argument AST shape, showing the binding-changing rewrite.
- Command wrapper runs the authored C# named-argument harness and captures the before and after evidence logs.
- The executed Git command confirms the newly added signature-call module is absent from the parent commit, establishing the pre-change baseline.
- The executed harness shows C# `F(y: 2, x: 1)` split as positional `2, 1` and rewritten to `F(1, 2)`, confirming changed bindings.
- The captured command output contains the exact authored harness and wrapper used to produce the observed result, making the reproduction traceable.
| def _render_param(spec: ParamSpec, language: cs.SupportedLanguage | None) -> str: | ||
| """A brand-new parameter in the target grammar's spelling.""" | ||
| annotation = spec.annotation | ||
| default = spec.default if spec.default is not None else spec.literal | ||
| if language == cs.SupportedLanguage.GO: | ||
| return f"{spec.name} {annotation}" if annotation else spec.name | ||
| if language == cs.SupportedLanguage.JAVA: | ||
| return f"{annotation} {spec.name}" if annotation else spec.name | ||
| if language == cs.SupportedLanguage.RUST: | ||
| return f"{spec.name}: {annotation}" if annotation else spec.name | ||
| text = spec.name | ||
| if annotation: | ||
| text += f": {annotation}" | ||
| if default is not None: | ||
| text += f" = {default}" if annotation else f"={default}" | ||
| return text |
There was a problem hiding this comment.
The renderer falls back to Python-style syntax for supported languages without dedicated handling, so a C# or C++ addition such as count:int becomes count: int. It also accepts typeless additions for Java, Go, and Rust, emitting only count. These generated definitions do not compile, so signature edits can leave affected source files invalid. Add language-specific rendering and reject additions whose required type information is absent. This must be fixed before merging.
Knowledge Base Used: Multi-language parsing (codebase_rag/parsers)
Artifacts
- This authored script invokes the changed renderer for the claimed language/specification combinations and compiles its C++, Go, and Rust output, showing the invalid definitions.
- This executable command records the working directory and runs the authored targeted harness, showing the invalid output.
- The executed harness rendered C# and C++ annotations with Python syntax and produced typeless Java, Go, and Rust additions; real C++, Go, and Rust compilers reject the rendered definitions, confirming the finding.
- This executable command compiles valid C++, Go, and Rust parameter forms for side-by-side comparison with the rendered output.
- The executed baseline compiled `int count`, `count int`, and `count: i32` successfully, establishing that the compiler failures are caused by the renderer's generated spellings.
| old = _parameters(node, language) | ||
| if any(p.node.type in _VARIADIC_PARAM_TYPES for p in old): | ||
| raise SignatureRefused(cs.SIGNATURE_VARIADIC.format(qn=qn)) |
There was a problem hiding this comment.
A Go parameter declared as args ...string is not recognized by the variadic refusal guard. Changing its annotation is planned as args int, silently converting a variadic API into a single-argument API while leaving syntactically valid Go code. Refuse Go variadic parameter edits or preserve the ... grammar explicitly. This must be fixed before merging.
Knowledge Base Used: Multi-language parsing (codebase_rag/parsers)
Artifacts
- A temporary Python harness creates a Go variadic definition, invokes the changed planner with graph responses, and optionally enables the proposed guard, demonstrating the semantic-loss path.
- The executed current-code run plans the Go edit and rewrites `args ...string` to `args int` while the patch remains parseable and formatted, confirming variadic semantics are removed.
- The executed control run adds the Go variadic AST node to the refusal set and the same request is refused, showing the required guard prevents the loss.
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
One or more issues must be addressed before approval.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 8
Open (12)
What changed in this PR
Adds graph-backed planning for change-signature edits across definitions and callers.
Changes:
- Adds signature metadata Cypher queries and evaluator support.
- Introduces parameter parsing, type checks, definition rewriting, and call-site mapping.
- Extends edit contracts and constants for signature-change reporting.
| File | Description |
|---|---|
| evals/cgr_graph.py | Updated as part of this pull request. |
| codebase_rag/editing/signature_types.py | Updated as part of this pull request. |
| codebase_rag/editing/signature_rewrite.py | Updated as part of this pull request. |
| codebase_rag/editing/signature_planner.py | Updated as part of this pull request. |
| codebase_rag/editing/signature_params.py | Updated as part of this pull request. |
| codebase_rag/editing/signature_literals.py | Updated as part of this pull request. |
| codebase_rag/editing/signature_calls.py | Updated as part of this pull request. |
| codebase_rag/editing/contract.py | Updated as part of this pull request. |
| codebase_rag/cypher_queries.py | Updated as part of this pull request. |
| codebase_rag/constants/mcp.py | Updated as part of this pull request. |
| codebase_rag/constants/graph.py | Updated as part of this pull request. |
| codebase_rag/constants/cli.py | Updated as part of this pull request. |
| codebase_rag/constants/ast_python.py | Updated as part of this pull request. |
| codebase_rag/constants/ast_nodes.py | Updated as part of this pull request. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| positional, keyword = _split_call_arguments(args_node) | ||
| if any(child.type in _SPLAT_TYPES for child in args_node.named_children): | ||
| return cs.SIGNATURE_SPLAT |
| for child in params.named_children: | ||
| if child.type == cs.TS_PY_KEYWORD_SEPARATOR: | ||
| keyword_only = True | ||
| continue | ||
| if child.type in _COMMENT_TYPES or child.type in _SEPARATOR_TYPES: |
| text = spec.name | ||
| if annotation: | ||
| text += f": {annotation}" | ||
| if default is not None: | ||
| text += f" = {default}" if annotation else f"={default}" |
| ) -> list[ParamSpec]: | ||
| names = [p.name for p in old if not p.receiver and not p.keyword_only] | ||
| resolved: list[ParamSpec] = [] | ||
| seen: set[str] = set() |
| if any(p.node.type in _VARIADIC_PARAM_TYPES for p in old): | ||
| raise SignatureRefused(cs.SIGNATURE_VARIADIC.format(qn=qn)) |
| for label, uid in targets: | ||
| props = self.nodes[(label, uid)] |
| TESTS_REACHING = "tests_reaching" | ||
| # Graph-driven edit operations (issue #1532). | ||
| RENAME = "rename" | ||
| CHANGE_SIGNATURE = "change_signature" |
| parser = self._parsers.get(language) if language is not None else None | ||
| if parser is None: | ||
| return language, None | ||
| return language, parser.parse(source).root_node |
| for spec in specs: | ||
| if spec.literal is None: | ||
| continue | ||
| annotation = spec.annotation | ||
| if ( | ||
| annotation is None | ||
| and spec.from_index is not None | ||
| and spec.from_index < len(declared) | ||
| ): | ||
| annotation = declared[spec.from_index] or None | ||
| if annotation is None: | ||
| continue | ||
| kind = _literal_kind(spec.literal) | ||
| if kind is not None and not _compatible(kind, annotation): | ||
| raise SignatureRefused( | ||
| cs.SIGNATURE_LITERAL_TYPE.format( | ||
| literal=spec.literal, name=spec.name, annotation=annotation | ||
| ) | ||
| ) |
| if ( | ||
| isinstance(positional, list) | ||
| and positional | ||
| and positional[0] in _RECEIVERS | ||
| ): | ||
| out = out[1:] if len(out) == len(positional) else out |
Graph query support and planner orchestration for hierarchy definitions and callers.
Files changed
codebase_rag/cypher_queries.pycodebase_rag/editing/signature_planner.pyevals/cgr_graph.pyDiff stats
+261 additions, -0 deletions (261 LOC)
Dependencies
This PR depends on:
pr-1,pr-2,pr-3,pr-4Dependency graph
Merge in this order: