Skip to content

feat(signature): read the header, bind call arguments, render the rewrite (#1533, 3/9) - #2162

Merged
vitali87 merged 11 commits into
mainfrom
pr-split/split-1912-change-signature/pr-3
Oct 17, 2026
Merged

vitali87 merged 11 commits into
mainfrom
pr-split/split-1912-change-signature/pr-3

Conversation

@vitali87

@vitali87 vitali87 commented Sep 22, 2026 •

Copy link
Copy Markdown
Owner

Summary

Demo

No user-visible change: adds internal helpers for change_signature, which #2164 exposes as cgr change-signature.

Type of Change

  • Bug fix
  • New feature
  • Performance improvement
  • Refactoring (no functional changes)
  • Documentation
  • CI/CD or tooling
  • Dependencies

Related Issues

Part of #1533 (split from #1912; stack: #2163 → #2161 → #2162 → #2166 → #2164 → #2169 → #2168 → #2165 → #2167).

Test Plan

  • Unit tests pass (make test-parallel or uv run pytest -n auto -m "not integration")

  • New tests added

  • Integration tests pass (make test-integration, requires Docker)

  • Manual testing (describe below)

  • uv run python -m pytest codebase_rag/tests/test_signature_bind.py -p no:randomly -q at b798f5720: 44 passed.

  • The full suite was not run locally. The CI Unit Tests run at b798f5720 was cancelled before it finished.

Checklist

  • PR title follows Conventional Commits format
  • All pre-commit checks pass (make pre-commit)
  • No hardcoded strings in non-config/non-constants files
  • No # type: ignore, cast(), Any, or object type hints
  • New comments and docstrings explain why, not what (see Comment Policy)

SKIP=no-commit-to-branch pre-commit run --from-ref 34d367d4f --to-ref b798f5720 (ruff, ruff-format, ty, bandit, generate-readme and the file hygiene hooks): passed. The unit-suite step of make pre-commit was not run locally.

Unchecked items:

  • signature_bind.py builds Python syntax inline in f-strings (f"{spec.name}={value}", the parenthesised argument list, and rf"\b{re.escape(old)}\b"). Its messages come from constants/cli.py.
  • signature_bind.py declares a label: object parameter.
  • Comments were not re-audited against the policy in this pass.

Files changed

  • codebase_rag/editing/signature_bind.py
  • codebase_rag/tests/test_signature_bind.py

Diff stats

+870 additions, -0 deletions (870 LOC)

Dependencies

This PR depends on: pr-2

Dependency graph

Merge in this order:

pr-1: feat(signature): contract expectation, shared call-site helpers and constants for change_signature (#1533, 1/9)
└── pr-2: feat(signature): parameter specs, mapping sources and the change_signature contract check (#1533, 2/9)
    └── pr-3: feat(signature): read the header, bind call arguments, render the rewrite (#1533, 3/9)  <-- this PR
        └── pr-4: feat(signature): the change_signature operation (#1533, 4/9)
            └── pr-5: feat(signature): expose change_signature through the CLI and MCP, with docs (#1533, 5/9)
                └── pr-6: test(signature): acceptance, transaction and MCP tests for change_signature (#1533, 6/9)
                    └── pr-7: test(signature): mapping tests for change_signature (#1533, 7/9)
                        └── pr-8: test(signature): definition and renamed-parameter tests for change_signature (#1533, 8/9)
                            └── pr-9: test(signature): hierarchy, receiver and default guards for change_signature (#1533, 9/9)

Summary by CodeRabbit

  • New Features
    • Added support for updating Python call arguments when parameter definitions change, including calls with nested edits.
    • Parameter renames update applicable references in method bodies while preserving attribute names and keyword labels.
  • Bug Fixes
    • Changes are declined when calls or parameter references are ambiguous or unsafe to rewrite, including unsupported arguments, name conflicts, or incompatible method overrides.
    • Renames are also declined when defaults or nested scopes make the change unsafe.

@coderabbitai

coderabbitai Bot commented Sep 22, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository: vitali87/code-graph-rag/.coderabbit.yaml
  • Review profile: QUIET
  • Plan: Advanced
  • Run ID: 147627ad-29f8-4827-b615-7fbeaa5a19b6
📥 Commits

Reviewing files that changed from the base of the PR and between 0828870 and f53049a.

📒 Files selected for processing (1)
  • codebase_rag/tests/test_signature_bind.py

Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 2 remain after this review.

📝 Walkthrough

The pull request adds helpers to parse Python parameter definitions, check parameter renames, and rewrite call arguments. It also adds tests for supported cases and conditions that cause a rewrite to be refused.

Changes: Python signature rewriting

Layer / File(s) Summary
Parse signatures and identify receivers
codebase_rag/editing/signature_bind.py, codebase_rag/tests/test_signature_bind.py
Extracts supported parameter forms and identifies method receivers, including self and cls receivers and static methods. Tests cover parameter parsing and receiver handling.
Validate and edit parameter renames
codebase_rag/editing/signature_bind.py, codebase_rag/tests/test_signature_bind.py
Finds eligible body references and creates definition edits. Checks defaults, rebinding, nested scopes, comprehensions, and override parameter names. Tests cover rename behavior and refusal cases.
Bind and render call arguments
codebase_rag/editing/signature_bind.py, codebase_rag/tests/test_signature_bind.py
Maps call arguments to parameters, folds contained edits, and renders arguments in the new parameter order when bindings are supported. Tests cover argument binding, rendering, and unsupported patterns.

Priority: ⬇️ Low

Estimated code review effort: 3 (Moderate) | ~25 minutes

Change: Feature

Merge Risk: ⚪ Minimal · up to f5304

The reviewed tests introduce no identified merge-blocking issue; normal checks remain appropriate.

### Architecture Summary

Architecture risk: 🟠 High · up to 7f409

The change affects 1 system.

Changed systems: codebase_rag

Architecture concerns
No architecture-level concerns identified.

Review details

Systems and components

  • observed — codebase_rag (service) was modified; 1 changed file maps to changed impact.

Before / after behavior

  • observed — Modified behavior in codebase_rag/editing/signature_bind.py: Adds the module documentation, imports, and shared signature-spec types and constants used by the binding helpers.
  • observed — Modified behavior in codebase_rag/editing/signature_bind.py: Adds source-text extraction, static-decorator detection, definition lookup by name and line range, and parsing of supported parameter forms; unsupported parameter node shapes return no spec.
  • observed — Modified behavior in codebase_rag/editing/signature_bind.py: Adds body-reference helpers that distinguish identifier references from attribute names and keyword labels, and detect identifiers inside nested syntax.
  • observed — Modified behavior in codebase_rag/editing/signature_bind.py: Adds parameter-rename scanning and refusal rules: nested scopes or rebinding statements mentioning the old name refuse the rename; references to the new name also refuse unless it is being renamed away. Comprehensions that bind the old name refuse, while eligible body references produce edit spans.

Reliability and maintainability

  • inferred — Risk-relevant change factors for codebase_rag: blast_radius_1; blast_radius_2; blast_radius_4; blast_radius_5; direct_dependents_1; direct_dependents_2; direct_dependents_4
Pre-merge checks: Passed 4
Check name Status Explanation
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Title check Passed The title clearly summarizes the main change: reading signatures, binding call arguments, and rendering rewrites. It follows the Conventional Commits format.
Description check Passed The description includes all required sections, explains the implementation and scope, identifies related issues and dependencies, documents targeted tests, and clearly states that the full suite and …
Full details: Description check

Explanation

The description includes all required sections, explains the implementation and scope, identifies related issues and dependencies, documents targeted tests, and clearly states that the full suite and some checklist items were not completed.

✨ Finishing Touches
📝 Generate docstrings
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@vitali87
vitali87 added this pull request to stack #2170 September 22, 2026 23:23
@vitali87
vitali87 force-pushed the pr-split/split-1912-change-signature/pr-3 branch from 6af7113 to 0828870 Compare October 4, 2026 15:32

vitali87 commented Oct 4, 2026 •

Copy link
Copy Markdown
Owner Author

Unit Tests (windows-latest, py3.13) fails on test_stack_quiet_wait.py::test_the_c_library_message_is_kept_at_debug_level[close]. This PR doesn't cause it. The same test fails on main's CI at ff3e0b7, which only bumped the version, and it passed on this PR's own head in the other CI run. The reset case of the same test is already skipped on Windows because mgclient does not always write to stderr there. No fix exists yet. I'll re-run the job once when the run finishes.

Base automatically changed from pr-split/split-1912-change-signature/pr-2 to main October 12, 2026 20:36
@vitali87

Copy link
Copy Markdown
Owner Author

@greptile review

Unit tests for signature_bind on real tree-sitter parses: plain parameter
specs and refusals, receiver handling (static, classmethod, unusual name),
body renames and the scopes that refuse them, binding a call's arguments
to old indexes, rendering the new argument list, folding nested edits,
hierarchy agreement and the definition's own edits.
Each assertion now checks one thing, and the pytest.raises blocks hold
only the call expected to raise.
Comment thread codebase_rag/editing/signature_bind.py Resolved
Reordering parameters used to reorder positional values, so `(a, b)` to
`(b, a)` turned `f(first(), second())` into `f(second(), first())` and
ran `second()` first. Values now stay positional only while they keep
the order the site wrote them; the rest go by keyword in that order.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Y9fLCP6Y2cycV2iRvFM8Ah
vitali87 pushed a commit that referenced this pull request Oct 15, 2026
#2162 now spells reordered values by keyword in their written order, so
the dry run renders `helper(a=3, b='z')` rather than `helper('z', 3)`.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Y9fLCP6Y2cycV2iRvFM8Ah
vitali87 pushed a commit that referenced this pull request Oct 15, 2026
Follows #2162: reordered values are spelled by keyword in the order the
site wrote them, so a reorder never changes evaluation order.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Y9fLCP6Y2cycV2iRvFM8Ah
…uns code

Names and plain literals evaluate the same in any order while nothing
runs between them, so a site passing only those keeps the positional
reorder #1533 asks for: `helper(2, 'x')` becomes `helper('x', 2)`. A site
with a call, attribute or f-string keeps its written order by keyword.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Y9fLCP6Y2cycV2iRvFM8Ah
vitali87 pushed a commit that referenced this pull request Oct 15, 2026
…ents

#2162 reorders a site positionally again when its values are only names
and plain literals, so the dry run renders `helper('z', 3)` once more.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Y9fLCP6Y2cycV2iRvFM8Ah
vitali87 pushed a commit that referenced this pull request Oct 15, 2026
…ally

Follows #2162: only a site with a value that runs code keeps its written
order by keyword.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Y9fLCP6Y2cycV2iRvFM8Ah
Sonar flags `_positional_run` over the cognitive-complexity limit; the
leading run and the cut before keyword values are now separate helpers,
with the same behaviour.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Y9fLCP6Y2cycV2iRvFM8Ah

Copy link
Copy Markdown
Owner Author

scan-pr / osv-scan failed on 43c457f, but the scan itself passed: comparing main with this branch reported No issues found. The job failed afterwards, when uploading the SARIF to code scanning got API rate limit exceeded for installation from GitHub. This PR changes no dependencies and nothing in it can fix a rate limit. I'll re-run the job once after the limit resets.


Generated by Claude Code

@vitali87

Copy link
Copy Markdown
Owner Author

@greptile review

@greptile-apps

This comment has been minimized.

Comment thread codebase_rag/editing/signature_bind.py Resolved
…meter

A class pattern's keyword (`case Point(a=a)`) names an attribute of the
subject, and the trailing parts of a dotted value pattern (`case Color.a:`)
are attribute names too. The body rename treated both as uses of the
parameter, so renaming `a` to `z` produced `case Point(z=z)`, which no
longer matches objects with an `a` attribute. Only the capture and the
head of a dotted name are references now; the same check stops those
labels counting as reads of the new name.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Y9fLCP6Y2cycV2iRvFM8Ah
@sonarqubecloud

Copy link
Copy Markdown

@vitali87

Copy link
Copy Markdown
Owner Author

@greptile review

@vitali87
vitali87 merged commit bb0b21d into main Oct 17, 2026
41 checks passed
@vitali87
vitali87 deleted the pr-split/split-1912-change-signature/pr-3 branch October 17, 2026 22:34
vitali87 pushed a commit that referenced this pull request Oct 17, 2026
#2162 now spells reordered values by keyword in their written order, so
the dry run renders `helper(a=3, b='z')` rather than `helper('z', 3)`.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Y9fLCP6Y2cycV2iRvFM8Ah
vitali87 pushed a commit that referenced this pull request Oct 17, 2026
…ents

#2162 reorders a site positionally again when its values are only names
and plain literals, so the dry run renders `helper('z', 3)` once more.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Y9fLCP6Y2cycV2iRvFM8Ah
vitali87 added a commit that referenced this pull request Oct 17, 2026
#2162 now spells reordered values by keyword in their written order, so
the dry run renders `helper(a=3, b='z')` rather than `helper('z', 3)`.
vitali87 added a commit that referenced this pull request Oct 17, 2026
…ents

#2162 reorders a site positionally again when its values are only names
and plain literals, so the dry run renders `helper('z', 3)` once more.
vitali87 added a commit to xujiantop-crypto/code-graph-rag that referenced this pull request Oct 28, 2026
vitali87#2162 now spells reordered values by keyword in their written order, so
the dry run renders `helper(a=3, b='z')` rather than `helper('z', 3)`.
vitali87 added a commit to xujiantop-crypto/code-graph-rag that referenced this pull request Oct 28, 2026
…ents

vitali87#2162 reorders a site positionally again when its values are only names
and plain literals, so the dry run renders `helper('z', 3)` once more.
deedee pushed a commit to deedee/code-graph-rag that referenced this pull request Oct 29, 2026
Follows vitali87#2162: reordered values are spelled by keyword in the order the
site wrote them, so a reorder never changes evaluation order.
deedee pushed a commit to deedee/code-graph-rag that referenced this pull request Oct 29, 2026
…ally

Follows vitali87#2162: only a site with a value that runs code keeps its written
order by keyword.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

2 participants