[vitali87]

Implement inline planning, call-site substitution, import cleanup, and the CRLF cut-span regression fix it needs.

Files changed

• codebase_rag/editing/inline_operation.py
• codebase_rag/editing/move.py

Diff stats

+318 additions, -3 deletions (321 LOC)

Dependencies

This PR depends on: pr-3

Dependency graph

Merge in this order:

pr-1: feat: add extract/inline public types and constants
└── pr-2: feat: analyze extract/inline scopes and transactions
    └── pr-3: feat: implement extract refactoring operation
        ├── pr-4: feat: implement inline refactoring operation  <-- this PR
        │   ├── pr-5: feat: expose extract/inline through CLI
        │   ├── pr-6: feat: expose extract/inline through MCP
        │   │   ├── pr-8: test: cover inline, MCP, and CRLF regression
        │   │   └── pr-10: docs: list extract/inline MCP tools
        │   └── pr-9: docs: document extract/inline architecture
        └── pr-7: test: cover extract refactoring
            └── pr-8: test: cover inline, MCP, and CRLF regression

[coderabbitai]

Warning

Review limit reached

Next included review available in 44 seconds.

Check out review usage here.

View limit details

Limit 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.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Repository: vitali87/code-graph-rag/.coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: b49ec97b-45a9-4eae-a946-2d837f3be67c

📥 Commits

Reviewing files that changed from the base of the PR and between eb49a73 and 42ab71c.

📒 Files selected for processing (2)

• codebase_rag/editing/inline_operation.py
• codebase_rag/editing/move.py

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[greptile-apps]

Confidence Score: 0/5

Unsafe to merge: the inline refactoring can generate invalid source, runtime exceptions, and silently different results.

Findings

1.  Preserve async contracts ▶
2.  Keep callee name scope ▶
3.  Evaluate arguments once ▶
4.  Keep default timing ▶
5.  Reject splatted arguments ▶
6.  Retain live imports ▶

Summary

This change adds inline refactoring and import cleanup, but the generated edits do not consistently preserve program behavior. The inliner can alter coroutine and generator contracts, resolve names in the wrong module, repeat argument side effects, re-evaluate defaults, emit invalid splat/spread syntax, and remove imports that are still required. These failures must be fixed before merging.

_{Reviews (1) · Last reviewed commit: "feat: implement inline refactoring opera..."}

[greptile-apps]

Comments Outside Diff

These 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.

•  Inline refactoring accepts async and generator definitions despite changing their call contracts ▶

  • Bug
    • The single-return eligibility gate accepts async def value(): return 7 and function* values() { return 7; }. The inliner replaces value() / values() with 7, removes the definition, and produces syntactically valid files whose runtime behavior differs: a coroutine becomes an int and an iterator becomes an int.
  • Cause
    • inline_operation.py:56-61 only checks that the body contains one Python or generic return statement; it does not reject async Python definitions or JavaScript generator definitions before substitution. The call-site rewrite at line 107 then substitutes the expression without preserving awaitable/iterator semantics.
  • Fix
    • Reject Python async definitions and JavaScript generator definitions in Inliner.plan before evaluating the single-return eligibility rule. Add regression tests that assert refusal for both forms.

•  Cross-file inline rewrites callee globals as unresolved or caller-bound names ▶

  • Bug
    • Inlining public_value(value) -> helper(value) + OFFSET + floor(0.2) into two external callers emitted helper(5) + OFFSET + floor(0.2) unchanged. The caller with conflicting local/imported bindings returned 116 rather than the original 20; the caller lacking those names raised NameError.
  • Cause
    • Inliner.plan calls _substitute at lines 101-103 with only formal parameter bindings. _substitute replaces identifiers only when they are function parameters/receiver bindings and preserves every other read identifier from the callee expression, although those identifiers resolve in the callee module.
  • Fix
    • Before cross-file inlining, analyze the return expression's free identifiers. Either refuse cross-file inline when any non-parameter identifier is module-scoped/imported, or preserve semantics by safely qualifying/importing required callee bindings in each caller (with collision handling). Add regression coverage for both conflicting caller bindings and absent bindings.

•  Inline substitution duplicates side-effecting arguments for repeated parameters ▶

  • Bug
    • When a returned expression reads a parameter multiple times, lines 247-274 substitute the bound call argument at every read. Inlining def twice(x): return x + x at twice(next_value()) produces ((next_value()) + (next_value())), changing the invocation count from one to two and result from 2 to 3.
  • Cause
    • The implementation gathers every identifier read in reads and performs textual substitution for each one, without rejecting repeated parameters bound to non-simple/side-effecting arguments or materializing the argument once.
  • Fix
    • Refuse inlining when a parameter is read more than once and its bound argument is not provably side-effect-free, or introduce a temporary binding that evaluates each argument once before evaluating the substituted expression.

•  Inlining re-evaluates definition-time Python defaults at every omitted call ▶

  • Bug
    • For def identity(value=default()): return value, two original identity() calls evaluate default() once during definition. The candidate maps the omitted parameter to textual default() and rewrites each call as ((default())), causing two evaluations after removing the definition.
  • Cause
    • _substitute falls through for an omitted argument and directly assigns the default expression source (defaults[name]) at lines 233-234. Python evaluates default expressions once when executing the function definition, not when invoking each call that omits the argument.
  • Fix
    • Refuse to inline Python functions when an omitted argument would require a non-literal/effectful default, or preserve the definition-time value by introducing a one-time temporary initialized at the original definition location and substitute that temporary. Add focused coverage for effectful default expressions and multiple omitted call sites.

•  Inline refactoring accepts splat and spread call arguments and emits invalid code ▶

  • Bug
    • Inlining a one-parameter identity function at identity(*values) produces result = ((*values)), and identity(...values) produces const result = ...values;. Both replacements are syntactically invalid in their respective languages. Python **values is also accepted and rendered as ((**values)).
  • Cause
    • _substitute at codebase_rag/editing/inline_operation.py:226-230 treats all non-keyword named argument nodes returned by _split_call_arguments as ordinary positional values. Tree-sitter represents Python splats and JS spread as list_splat, dictionary_splat, and spread_element, but no rejection occurs before their source text is substituted into an expression position.
  • Fix
    • Reject calls containing Python list_splat/dictionary_splat or JS/TS spread_element before positional binding (return None so the call is not rewritten and the definition is retained). Add focused regression tests for *args, **kwargs, and JS/TS ...args.

•  Inline cleanup removes imports still needed as values or public bindings ▶

  • Bug
    • _drop_imports() deletes every importer of the inlined symbol after call rewriting, even when the local import name remains used as a value, decorator-related binding, base-related binding, or re-export. The executed fixture shows all four import statements removed while helper identifiers remain in every resulting file.
  • Cause
    • At inline_operation.py:132-160, cleanup is driven solely by matching IMPORTS rows for the deleted definition. It does not parse/analyze the post-rewrite importer file to determine whether its bound local name remains referenced outside rewritten call spans, nor does it preserve public re-exports.
  • Fix
    • Before removing an import entry, inspect the transformed importer AST for remaining references to the local bound name (including aliases, decorator/base/value contexts and re-export/public API constructs). Retain the import when any such reference remains; alternatively refuse definition removal when non-call references to the target symbol exist.

[Copilot]

Copilot review overview

🟡 Changes recommended

Address the async inlining behavior and prevent deletion when no call sites are rewritten.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1

Open (1)

• Refuse async function inlining without coroutine preservation · New

What changed in this PR

Implements inline refactoring with call-site substitution, import cleanup, transaction handling, and a CRLF span fix.

Changes:

• Adds inline planning and application logic.
• Handles argument/default substitution and import removal.
• Makes cut-span handling CRLF-aware.

File	Summary	Findings
codebase_rag/​editing/​move.py	CRLF-safe cut-span handling	No blocking findings.
codebase_rag/​editing/​inline_operation.py	Inline planning, rewriting, cleanup, and transaction integration	Async definitions require safe handling; definitions must not be removed when no call sites were rewritten.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[vitali87]

Closing as superseded: this is part of the split of #1550, which was closed without merging, and it targets the stale feat/move-op branch rather than main. The branch is kept, so the extract/inline work can be rebased onto main if it's picked up again.

---

Generated by Claude Code