Skip to content

fix(edit): re-base indentation when recovering a shifted patch - #190

Open
yurekami wants to merge 2 commits into
SakanaAI:mainfrom
yurekami:fix/indent-rebase-search-replace
Open

fix(edit): re-base indentation when recovering a shifted patch#190
yurekami wants to merge 2 commits into
SakanaAI:mainfrom
yurekami:fix/indent-rebase-search-replace

Conversation

@yurekami

@yurekami yurekami commented Sep 3, 2026

Copy link
Copy Markdown

Summary

  • _find_indented_match measures the signed indentation offset from the SEARCH block's first line to the candidate line in the file and shifts every SEARCH line by it, clamping only the final indentation at column zero.
  • _apply_indentation_to_replace applies that same SEARCH-derived offset to the REPLACE block.
  • Leading whitespace is drawn from the matched line and padded with spaces beyond it, so a tab-indented file keeps its tabs.
  • Eight regression tests in tests/test_edit_base.py.

Why

_find_indented_match recovers a SEARCH block whose indentation does not match the file. It locates the target line, takes that line's indentation, then rebuilds the block:

search_line_indent = len(search_line) - len(search_line.lstrip())
indented_search_lines.append(indent_str + " " * search_line_indent + search_line.strip())

search_line_indent is the line's absolute indent, so the block's own base indentation is added on top of indent_str and counted twice. Recovery therefore only ever worked when the SEARCH block started at column 0.

A block copied out of a nested scope carries its own leading indentation, which is the ordinary case for an evolved method body. For those the rebuilt text never matches anything, and apply_diff_patch (which calls apply_search_replace with its default strict=True) returns num_applied=0 with a SEARCH text not found error. The runner treats that as a failed attempt and retries, so the cost is a rejected patch and another model call, not a silently scored no-op. The first version of this description said otherwise; that came from reading patch_txt instead of error_message when unpacking the return tuple in my reproduction, and is corrected here.

_apply_indentation_to_replace computed the replacement indentation the same way, so fixing only the search side made the patch apply at the wrong depth. Both are changed together.

Design

The first revision of this PR re-based each line on the block's first line and clamped the relative indent at zero. As pointed out in review, that loses legitimate dedentation below the first line: a SEARCH whose later line dedents (the author means "after the if") was flattened onto the first line's indent and matched a same-indent block inside the if, applying with num_applied=1 and rewriting differently scoped code. The same clamp placed a dedented REPLACE line too deep.

This revision instead computes one signed offset, indent(candidate line) - indent(SEARCH first line), and applies it to every SEARCH and REPLACE line, clamping only the result at column zero. A dedenting line keeps dedenting, so it cannot match a same-indent block in another scope, and using the SEARCH-derived offset for REPLACE preserves an intentional difference between the two blocks' first-line indentation.

Indentation characters come from the matched line: the first len(prefix) columns reuse the line's own whitespace and anything beyond is padded with spaces. That keeps test_mixed_indentation_styles (tab-indented file, space-free SEARCH) working the way the previous code did by reusing the matched prefix.

Where this runs: _find_indented_match is called on the main replacement path of apply_search_replace, which every diff-mode edit goes through.

Linked issue or context

No open issue. Found while reading shinka/edit/. The existing test test_find_indented_match_multiline_with_relative_indentation covers this function with a SEARCH block starting at column 0, where absolute and relative indent coincide, which is why the defect was not visible.

Testing

  • Commands run:
    • uv run ruff check tests --exclude tests/file.py -> All checks passed
    • uv run ruff check shinka/edit/apply_diff.py -> All checks passed
    • uv run ruff format --check shinka/edit/apply_diff.py -> already formatted (tests/test_edit_base.py has two pre-existing format hunks at lines 192 and 345 that predate this PR; my additions are clean)
    • uv run mypy --follow-imports=skip --ignore-missing-imports tests/test_edit_base.py tests/conftest.py -> Success
    • uv run pytest -q tests/test_edit_base.py -> 45 passed
    • uv run pytest -q -m "not requires_secrets" -> 899 passed, 1 skipped, 3 failed
  • Results:

The 3 failures are pre-existing on this machine and fail identically on a clean checkout of main (they need wolframscript and a claude binary on PATH, and the third is a subprocess timeout on Windows).

Regressions added, all of which fail on main:

  • test_find_indented_match_search_block_carries_its_own_indent, test_find_indented_match_search_deeper_than_target, test_apply_indentation_to_replace_rebases_existing_indent, test_apply_diff_patch_reindents_nested_block: the original defect.
  • test_find_indented_match_dedent_below_first_line_does_not_flatten and test_apply_diff_patch_rejects_flattened_dedent_in_strict_mode: a shifted SEARCH whose later line dedents below its first line does not match a same-indent block, and end to end the patch is rejected with a not-found error rather than applied.
  • test_apply_diff_patch_preserves_dedent_in_replacement: an applied replacement keeps its dedent.
  • test_apply_diff_patch_keeps_replace_relative_to_search: a REPLACE that starts four columns deeper than its SEARCH lands four columns deeper than the matched code.

Risks and compatibility

For a SEARCH block starting at column 0 the offset equals the matched line's indent and every emitted line is byte-identical to before; the 37 pre-existing tests in test_edit_base.py pass unchanged. _apply_indentation_to_replace now takes a signed offset (and optional indent prefix) instead of an indent string; it is private, and its two existing tests were updated to pass 4 in place of four spaces with the same expected output.

Lines that would end up left of column zero after the shift are clamped there rather than raising.

Core evolution pipeline evidence

This changes the patch-application path. The mutable region sits inside a nested class and the model re-emits the block one level shallower than it appears in the file:

original = """# EVOLVE-BLOCK-START
class Solver:
    class Inner:
        def score(self, x):
            total = 0
            return total
# EVOLVE-BLOCK-END
"""

patch = """<<<<<<< SEARCH
    def score(self, x):
        total = 0
        return total
=======
    def score(self, x):
        total = sum(x)
        return total
>>>>>>> REPLACE"""

Before, on main, the patch is rejected and the attempt retried:

num_applied=0  error="SEARCH text not found in editable regions ..."

After, it applies at the file's indentation:

num_applied=1  error=None
class Solver:
    class Inner:
        def score(self, x):
            total = sum(x)
            return total

And the review scenario, a SEARCH whose second line dedents below its first against a file where both lines sit at the same indent, is now rejected (num_applied=0, not-found error) where the first revision applied it and moved z() inside the if.

I have not run a full evolution benchmark, so I cannot say how often a model emits a shifted block in practice; the effect is fewer rejected patches and retries on that path.

Docs and UI

No docs or UI changes.

Prepared with Claude Code (agent-assisted); every command and result above was run locally.

🤖 Generated with Claude Code

https://claude.ai/code/session_01VZA548KwueeKS1BCRhowZq

_find_indented_match recovers a SEARCH block whose indentation does not
match the file. It finds the target line, takes that line's indentation,
and then rebuilds the block by adding each remaining line's *absolute*
indent on top of it. That counts the block's own base indentation twice,
so recovery only ever worked for a block starting at column 0.

Any block copied out of a nested scope carries its own leading
indentation, which is the ordinary case for an evolved method body. For
those, the rebuilt text never matches, _find_indented_match returns -1,
and apply_diff_patch reports num_applied=0 with error=None: the edit is
dropped silently and the evolution step is a no-op.

_apply_indentation_to_replace has the same defect on the replacement
side, so fixing only the search side makes the patch apply at the wrong
depth. Both now re-base on the block's own first (non-empty) line, which
leaves column-0 blocks byte-identical and keeps the existing tests green.

Add four regression tests: search shallower than target, search deeper
than target, replacement re-basing, and an end-to-end patch into a
nested class.
@RobertTLange

Copy link
Copy Markdown
Collaborator

Thanks for isolating this — the underlying bug is real, and the narrower PR is worth pursuing. I reproduced the shifted-indentation failure, and the new tests plus the full non-secret suite pass locally.

I found one correctness issue that I think should be fixed before merge: both new calculations clamp the relative indentation itself:

relative_indent = max(line_indent - base_indent, 0)

That loses legitimate dedentation below the block's first line. In _find_indented_match, it can flatten a dedenting SEARCH into a same-indent candidate and falsely match differently scoped code. I reproduced such a case applying with num_applied=1 and error=None. In _apply_indentation_to_replace, it can similarly place a dedented statement too deep and silently change control flow.

Could you instead compute the signed indentation offset from the SEARCH block's base to the matched target, then apply that same offset to every SEARCH and REPLACE line, clamping only the final indentation at column zero? Using the SEARCH-derived offset for REPLACE also preserves intentional baseline differences between the SEARCH and REPLACE blocks.

Please add regressions for:

  • a shifted SEARCH whose later line dedents below its first line, ensuring it does not falsely match a same-indent source block;
  • an end-to-end replacement that preserves such a dedent;
  • differing SEARCH/REPLACE first-line indentation, ensuring the replacement retains the intended relative shift.

One description correction: apply_diff_patch calls apply_search_replace with its default strict=True. On current main, the PR's core example returns num_applied=0 with a non-None SEARCH-not-found error, and the runner treats that as a failed attempt and retries. So the practical benefit is avoiding rejected patches/retries, rather than preventing a no-op child from being silently scored.

Local validation on the PR head: Ruff passed; tests/test_edit_base.py had 41 passed; the non-secret suite had 892 passed, 7 skipped, 1 deselected.

…espace

The first version of this fix re-based each line on the block's first
line and clamped the relative indent at zero. That loses legitimate
dedentation below the first line: a SEARCH block whose later line
dedents (the author means "after the if") was flattened onto the first
line's indent and matched a same-indent block inside the if, applying
with num_applied=1 and rewriting differently scoped code. The same
clamp placed a dedented REPLACE line too deep.

Measure the signed offset from the SEARCH block's first-line indent to
the candidate line in the file, shift every SEARCH and REPLACE line by
that offset, and clamp only the final indentation at column zero.
Using the SEARCH-derived offset for REPLACE also keeps an intentional
difference between the two blocks' first-line indentation.

Leading whitespace is drawn from the matched line and padded with
spaces beyond it, so a tab-indented file keeps its tabs, which the
previous code achieved by reusing the matched line's prefix.

Four regressions: a dedenting SEARCH must not match a same-indent
block, end to end that false match is rejected, an applied replacement
keeps its dedent, and a REPLACE that starts deeper than its SEARCH
stays deeper.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VZA548KwueeKS1BCRhowZq
@yurekami

yurekami commented Sep 3, 2026

Copy link
Copy Markdown
Author

Thank you for the review, and for the reproduction. I confirmed the false match on the previous head: a SEARCH whose second line dedents below its first, against a file where both lines sit at the same indent inside the if, applied with num_applied=1 and moved the replacement line inside the if.

Pushed a follow-up commit rather than rewriting the reviewed one. What changed:

  • _find_indented_match now measures one signed offset, indent(candidate line) - indent(SEARCH first line), and shifts every SEARCH line by it. Only the final indentation is clamped at column zero, so a dedenting line keeps dedenting and cannot be flattened onto a same-indent block in another scope.
  • _apply_indentation_to_replace applies that same SEARCH-derived offset to the REPLACE block, which keeps an intentional difference between the two blocks' first-line indentation. Its signature is now (replace_text, indent_offset, indent_chars=""); it is private, and its two existing tests pass 4 in place of four spaces with the same expected output.
  • Leading whitespace is drawn from the matched line and padded with spaces beyond it. My first pass of the signed-offset version emitted spaces only and broke test_mixed_indentation_styles (tab-indented file, space-free SEARCH), which the original code handled by reusing the matched line's prefix; this keeps that behaviour.

Regressions added for the three cases you listed, plus the end-to-end rejection:

  • test_find_indented_match_dedent_below_first_line_does_not_flatten
  • test_apply_diff_patch_rejects_flattened_dedent_in_strict_mode (your scenario: num_applied == 0, not-found error, content unchanged)
  • test_apply_diff_patch_preserves_dedent_in_replacement
  • test_apply_diff_patch_keeps_replace_relative_to_search (REPLACE starts four columns deeper than SEARCH and lands four deeper than the matched code)

On the description: you are right about strict=True. On main the core example returns num_applied=0 with a SEARCH text not found error and the runner retries, so the benefit is avoiding rejected patches and retries rather than preventing a silently scored no-op. The earlier claim came from my reproduction script unpacking patch_txt in the position of error_message. The PR description is corrected, and the original commit message overstated it in the same way; if you squash on merge the description is the accurate account.

Locally on the new head: tests/test_edit_base.py 45 passed; the non-secret suite 899 passed with the same 3 environment failures as before (wolframscript, claude binary, Windows subprocess timeout); Ruff and mypy clean.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants