Repository navigation
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (2)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository: vitali87/code-graph-rag/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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}
| 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) |
There was a problem hiding this comment.
A bare * makes following parameters keyword-only; it does not accept additional positional arguments. For def helper(a, *, b=1), a call with two positional arguments raises TypeError, but this condition classifies the function as variadic and causes the structural arity check to return ok. Real excess-positional contract changes are therefore omitted.
| return node.args.vararg is not None or bool(node.args.kwonlyargs) | |
| return node.args.vararg is not None |
Artifacts
- Authored and executed script loads the target source functions and compares ordinary versus keyword-only positional arity behavior, confirming the targeted path.
- Executed ordinary-signature baseline reports `_header_is_variadic=False`, accepts two positional arguments at runtime, and produces `too_many` from the focused graph check due to the intentionally one-item stored positional fixture.
- Executed keyword-only case shows Python raises a two-positional-argument TypeError while `_header_is_variadic=True` makes `_arity_verdict` return `ok`, confirming the defect.
| if ")" in line: | ||
| break |
There was a problem hiding this comment.
This stops reading at any line containing ), including parentheses inside a default expression. A valid multiline definition such as def f(\n a=g(),\n *rest,\n): is truncated before *rest, so it is treated as non-variadic. Calls that the function accepts can then be reported as arity regressions, causing correct edits to fail their postcondition.
Artifacts
- Authored isolated Python script that extracts and runs the current repository variadic-detection implementation against a temporary realistic source file, confirming the false negative.
- First executed reproduction output shows the current implementation returns false while AST and runtime establish that the function is variadic.
- Repeated executed reproduction against the same structural_delta.py hash produces the same false negative, confirming the result is reproducible.
- Executed git diff and status check shows no modification to codebase_rag/structural_delta.py during validation.
| if not body: | ||
| return False | ||
| try: | ||
| tree = ast.parse(body + "\n pass\n") |
There was a problem hiding this comment.
The helper always appends an indented pass, but a one-line definition already has a complete suite. For def f(a, *rest): return a, the constructed source has an unexpected indent, the parse failure returns False, and the variadic function is treated as having a positional overflow. This can reject correct edits through a false arity delta.
Artifacts
- Authored executable extracts and runs the current helper definitions, then reconstructs a one-line variadic source file; it directly exercises the reported path.
- Python parsed a header-only `def f(a, *rest):` with an indented pass and reported a variadic argument; the control confirms the intended header-only construction.
- The executed current helper code raised the expected parse error for the appended pass and returned false from both direct and reconstructed variadic checks; the finding is confirmed.
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
Unresolved moderate findings affect positional-overflow handling, multiline headers, inline definitions, and the test oracle.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 2
Open (2)
What changed in this PR
This PR replaces regex-based variadic-header detection with AST parsing for structural-delta arity validation and adds regression tests.
Changes:
- Adds AST-based header parsing.
- Covers defaults, nested delimiters, Unicode identifiers, and malformed headers.
- Adds variadic-header regression tests.
| File | Summary and findings |
|---|---|
codebase_rag/structural_delta.py |
Implements AST-based detection. 3 moderate findings: keyword-only arguments incorrectly absorb positional overflow (3 votes); multiline headers can truncate at the first ) (3 votes); complete one-line definitions can fail due to an appended body (1 vote). |
codebase_rag/tests/test_variadic_header_detection.py |
Adds parsing tests. 1 moderate finding: the oracle incorrectly treats keyword-only arguments as variadic (1 vote). |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| 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) |
| 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)) |
|
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 |
Teach structural-delta postcondition checks to parse variadic Python headers robustly for move/signature validation.
Files changed
codebase_rag/structural_delta.pycodebase_rag/tests/test_variadic_header_detection.pyDiff stats
+112 additions, -6 deletions (118 LOC)
Dependencies
This PR depends on:
pr-1Dependency graph
Merge in this order: