[vitali87]

Implement extract planning and rewriting for Python and JS/TS spans using the shared scope and contract helpers.

Files changed

• codebase_rag/editing/extract_operation.py

Diff stats

+296 additions, -0 deletions (296 LOC)

Dependencies

This PR depends on: pr-2

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  <-- this PR
        ├── pr-4: feat: implement inline refactoring operation
        │   ├── 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: bef2d707-15d1-4e15-bbcf-ef1d0b2bd88a

📥 Commits

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

📒 Files selected for processing (1)

• codebase_rag/editing/extract_operation.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: 1/5

Not safe to merge: reproduced extraction paths either generate invalid source, fail at runtime, or change JavaScript behavior; the explicit naming requirement also remains unmet.

Findings

1.  Invalid class helper placement ▶
2.  Mixed outputs lose declarations ▶
3.  Async extraction commits invalid code ▶
4.  JavaScript context changes ▶
5.  CRLF files gain LF lines ▶
6.  Use descriptive helper names ▶

Summary

This change adds extract-function planning and transactional application for Python and JavaScript/TypeScript. It is not safe to merge yet: class-method extraction can generate invalid JavaScript, mixed JavaScript outputs can fail at runtime, async extractions can commit files rejected by language parsers, and JavaScript context-sensitive references change behavior. CRLF formatting is also not preserved. The repository naming requirement must be satisfied before merging.

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

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

•  JS class-method extraction inserts an illegal function declaration into the class body ▶

  • Bug
    • For Calculator.compute, extracting lines 3-4 produces const doubled = extractDoubled(value); in the method and then inserts function extractDoubled(value) { ... } before the class closing brace. JavaScript class bodies cannot contain standalone function declarations, and Node rejects the generated file with SyntaxError: Unexpected identifier 'extractDoubled'.
  • Cause
    • Extractor.plan unconditionally inserts generated JS helper text immediately after the method definition (lines 144-156), while _js_pieces always emits a standalone function declaration and bare call (lines 242-249). That form and placement are incompatible for a class method.
  • Fix
    • Special-case JS/TS method extraction: either insert the helper outside the enclosing class at module scope (and preserve a valid callable scope), or emit a valid class method and call it through an appropriate receiver (this.extractDoubled(...) or a static receiver). Add JS and TS class-method extraction tests that syntax-check generated output.

•  Mixed JavaScript extract outputs omit declaration for a fresh result ▶

  • Bug
    • For const newValue = existing = seed + 1, extraction identifies outputs ('newValue', 'existing') and rewrites the selected statement to ({ newValue, existing } = extractStep(existing, seed));. Because existing predates the selected span, the call-site rewrite does not declare either target. In strict mode newValue is unresolved, so the syntactically valid generated file throws ReferenceError: newValue is not defined.
  • Cause
    • _js_pieces determines fresh with all(...) across all outputs and emits a bare destructuring assignment whenever any output was already bound. This cannot represent a mixed fresh/existing output set while retaining the fresh binding.
  • Fix
    • Emit a declaration for fresh outputs and assignment for existing outputs, preserving evaluation order-for example, assign the helper result to a temporary and then declare/assign outputs separately. Alternatively, refuse extraction whenever outputs mix names declared in the span and names that were already bound.

•  Extractor commits invalid code when extracting await-containing Python or JavaScript spans ▶

  • Bug
    • A focused temporary-fixture reproduction invoked Extractor.plan and Extractor.apply with a graph-query stub for an await-containing assignment. Both original async functions parsed and returned 7. Python planning emitted def extracted() and value = extracted() around await asyncio.sleep(...); JavaScript planning emitted function extracted() and const value = extracted() around await Promise.resolve(...). Extractor.apply reported applied: true for both cases, but subsequent external checks rejected the resulting files: Python py_compile raised SyntaxError: 'await' outside async function, and node --check raised SyntaxError: await is only valid in async functions and the top level bodies of modules.
  • Cause
    • _py_pieces and _js_pieces unconditionally emit synchronous helper declarations and bare calls. No extraction-span analysis detects an await_expression, so the generated helper is not async and the caller does not await it. The internal tree-sitter has_error parse verdict is true for the invalid generated forms, allowing _commit to persist the change.
  • Fix
    • Detect await_expression in the selected span. For Python, emit async def and replace the call with await helper(...); for JavaScript/TypeScript, emit async function and replace the call with await helper(...). Alternatively, refuse extraction when async context cannot be preserved. Add plan/apply tests using external Python and Node syntax checks so code is not committed solely on the permissive tree-sitter parse verdict.

•  JavaScript extraction commits behavior-changing helpers for this and arguments ▶

  • Bug
    • The extractor accepts spans containing this or arguments, moves their statements into a standalone helper, and invokes that helper bare. The focused fixture demonstrated this changing output from 42 to NaN and arguments changing output from 10 to NaN; both transformed files passed node --check and were committed by Extractor.apply.
  • Cause
    • _analyse identifies only ordinary lexical identifiers as inputs and _js_pieces emits a standalone function extracted(...) plus extracted(...). It neither detects context-sensitive this/arguments nor preserves the original receiver or argument object.
  • Fix
    • Reject extract spans containing JavaScript this, arguments, or super unless context-preserving extraction is explicitly implemented. At minimum, add an AST scan before _js_pieces and raise ExtractRefused for these node types; add plan and apply regression tests.

•  Extractor emits LF into CRLF source files ▶

  • Bug
    • Applying extraction to a CRLF Python source creates five bare-LF line endings while retaining three original CRLF line endings. This causes line-ending churn in the edited file.
  • Cause
    • Extractor.plan builds generated function and call text with literal LF separators instead of deriving the source file's newline convention. Decoding/stripping the extracted body also leaves CRLF handling inconsistent.
  • Fix
    • Detect the source newline convention once in plan (for example, use CRLF when present) and pass/use it for generated call and function text, including Python and JS/TS pieces; normalize extracted body line separators consistently before re-emitting it.

[Copilot]

Copilot review overview

🟡 Changes recommended

Callable-target validation and TypeScript generic preservation must be addressed before approval.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1

Open (1)

• Non-callable definitions can be extracted as functions · New

What changed in this PR

Implements extract-function planning and transactional rewriting for Python and JavaScript/TypeScript.

Changes:

• Analyzes definitions, spans, inputs, outputs, and early exits.
• Generates helper functions and replacement calls.
• Applies validated patches with rollback and re-ingestion support.

File	Review summary
codebase_rag/​editing/​extract_operation.py	Implements extraction planning and rewriting. Critical: restrict targets to callable definitions. Moderate: preserve TypeScript generic parameters in generated helpers.

---

💡 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