fix(harness): handle CRLF line endings in the patch fuzziness ladder - #3416

[Warning] Mapping the emitted newline to bodyEnd protects the trailing boundary, but it moves the leading boundary inside a terminator: when old_string itself starts with '\n', originalIndex[idx] for that newline resolves to the '\r', so the replaced span begins on the previous line's CR while the replacement only carries an LF — that line's terminator is silently downgraded CRLF→LF, which is the same class of rewrite that point 2 of the description exists to prevent, and it sits against the class contract "we never touch whitespace the LLM didn't ask to change".

Repro (compiled this PR's head standalone):

existing = "alpha\r\nbeta   \r\ngamma\r\n"
needle   = "\nbeta\ngamma"        // leading newline
// level = TRAILING_WS_STRIPPED, span = "\r\nbeta   \r\ngamma"
// patching with "\nBETA\nGAMMA" yields:
"alpha\nBETA\nGAMMA\r\n"          // first line lost its '\r'

On main this needle matches nothing at all, so it is not a regression — but the new leniency now reaches a path that rewrites a byte outside the requested span.

Suggested direction: give start and end offsets separate map targets (e.g. keep lineEnd for a start that lands on an emitted newline, bodyEnd for an end), or clamp origStart forward past a '\r' that is immediately followed by '\n' in findAllNormalized — the Normalized record would then need the source string (or a parallel offset array). A test pinning a newline-prefixed needle against a CRLF document would lock whichever choice is made.

Thanks — the asymmetry is real, and I've taken your first suggested direction.

Leading boundary. findAllNormalized now resolves the span's start through a startOffset
helper: when the normalised character is a '\n' that a CRLF terminator produced, the start
resolves to the '\n' itself rather than to the '\r' in front of it. The '\r' therefore stays
outside the span, the caller's replacement supplies the line break, and the document's terminator
style survives. The end boundary is unchanged and still resolves to the terminator's start, so
neither boundary can split a terminator — the two are now mirror images. A '\n' that was already a
lone LF resolves exactly as before, so pure-LF input stays byte-for-byte identical.

Your repro now maps to span "\nbeta \r\ngamma" (charAt(start - 1) == '\r'), and patching it
with "\nBETA" yields "alpha\r\nBETA\r\n" — no downgraded terminator.

Tests. Both gaps you named are closed:

• crlfLeadingNewlineKeepsTerminatorOutsideSpan — your exact repro, pinned at
  TRAILING_WS_STRIPPED. It fails on main, and with only startOffset neutered it is the single
  failure in the class, so it pins this fix specifically rather than the CRLF handling in general.
• patchMatchesCrlfSkillFileWithLfNeedle — end-to-end SkillManageTool patch on a CRLF skill file
  with a multi-line LF needle. On main it fails with the user-visible old_string not found in SKILL.md at any fuzziness level (...), and it asserts the frontmatter's terminators survive the
  patch untouched.

Writing the second one turned up something worth recording: create round-trips content through
SkillUtil.createFrom and writes LF, so the tool never produces a CRLF file itself. A CRLF
SKILL.md only arrives from outside the tool — authored on Windows, unpacked from a marketplace
archive, copied in by hand. The test therefore writes the CRLF bytes onto disk directly; from there
readSkillFile → filesystem.read(...) returns them verbatim, which is the no-normalisation path
the description claims.

Info items. The javadoc now states both adjacent behaviours you flagged: a lone '\r' (classic
Mac endings, or a file where only some lines end that way) still matches nothing and is out of
scope, and because the replacement is spliced verbatim, a single patch over a mixed-terminator
document can leave both styles present.

Module run, JDK 17: agentscope-harness — 1091 tests, 0 failures on the last two full runs. One
run out of three additionally hit a background-task quiescence timeout
(HarnessBackgroundTaskQuiescenceExtension, in SandboxFilesystemIsolationScopeExampleTest); it
did not reproduce in the two runs after it, and that extension is unreachable from a pure static
string function, so I am treating it as a timing flake rather than something this change caused.

[Info] Helper reads well and the guards are correct (lineEnd < s.length() keeps a trailing \r with no \n as real content, lineEnd > lineStart keeps "\r\n" a genuine empty line). Two adjacent behaviours are worth a sentence here now that the class javadoc advertises CRLF support:

• Lone-'\r' terminators (classic Mac, or a file where only some lines end in \r) still match nothing: "alpha\rbeta\rgamma" vs "alpha\nbeta" is empty at every level, unchanged from main. Out of scope is fine — saying so keeps the claim honest.
• Mixed-terminator documents now do match ("alpha \nbeta\r\ngamma\n" hits TRAILING_WS_STRIPPED), which is the desired outcome, but combined with the spliced-verbatim limitation it means one patch can leave both styles in the same file.

Done — the class javadoc now states both adjacent behaviours: a lone '\r' (classic Mac endings, or a file where only some lines end that way) is still treated as ordinary content and matches nothing, and because the replacement is spliced verbatim, a single patch over a mixed-terminator document can leave both styles present.

Correcting my own message above: the lone-'\r' sentence was added to the javadoc, but the mixed-terminator consequence was not — I overstated that at the time. It is in now, with the reason the replacement is spliced verbatim, because reverting startOffset makes it reachable again (a newline-leading needle hands that terminator to the caller's new_string).

[Info] crlfBehavesLikeLfTwin comparing level / match count / resolved range against an LF twin is exactly the right guard, and I confirmed the claim independently: pure-LF input produces identical levels, counts and ranges on main and on this branch, so every pre-existing LF test really is untouched.

Two gaps worth closing while this area is open:

1. a needle that starts with '\n' against a CRLF document (the leading-boundary case in my comment on FuzzyTextMatcher.java:208);
2. an end-to-end SkillManageToolTest patch on a CRLF skill file — that test class currently contains no '\r', so the user-visible failure reported here (patch → "old_string not found … at any fuzziness level") is guarded at the matcher level but not at the tool level.

Both gaps closed.

• crlfLeadingNewlineKeepsTerminatorOutsideSpan — the newline-leading needle, using your repro and pinned at TRAILING_WS_STRIPPED.
• patchMatchesCrlfSkillFileWithLfNeedle — the end-to-end SkillManageTool patch.

Writing the second one turned up something worth knowing: create and edit round-trip the content through SkillUtil.createFrom and always write LF, so the tool never produces a CRLF file itself — a CRLF SKILL.md only arrives from outside it. The test writes those bytes onto disk directly, and readSkillFile returns them verbatim.

[Warning] startOffset skips exactly one \r, but the condition it guards on is "the character at the normalised index is a newline" — which is true for every newline in the needle, not just the first. When a needle starts with more than one newline (a copy-pasted old_string spanning a blank line), only the first terminator is stepped over and the second match begins after the leading CR, so that CR is dropped outside the span and stays in the document as an orphan.

Repro against this head (existing = "one\r\n\r\nalpha beta\r\n", old_string = "\n\nalpha beta"):

The LF twin ("one\n\nalpha beta\n") yields "oneREPL\n", so neither 24b6106 nor this head agrees with the parity this PR exists to establish. Reaching TRAILING_WS_STRIPPED rather than WHITESPACE_COLLAPSED is the intended improvement; what regressed is the byte hygiene — main at least left the terminators verbatim.

Stepping over every leading terminator fixes it — advance while the normalised character at the current index is a newline:

while (haystack.text.charAt(idx) == '\n'
        && haystack.source.charAt(orig) == '\r'
        && orig + 1 < haystack.originalLength()
        && haystack.source.charAt(orig + 1) == '\n') {
    orig += 2;   // step past this terminator; the next one may also lead the span
    idx += 1;
}
return orig;

(advance idx alongside orig so multi-newline leading needles terminate correctly, and note idx + 1 must stay in range for haystack.text). Worth a crlfBlankLineLeadingNewlinesPatchedOutputMatchesLfTwin test pinning the patched bytes, not only the span, so all three versions above are frozen.

You're right, and I went further than a while: I removed startOffset entirely. The single-step rule turns out to be the wrong shape rather than an under-sized version of the right one.

Measured, by installing each rule and running the new test (each cell is the patch result folded \r\n→\n, ✅ = equals the LF twin's output):

The while column is the shape the condition actually describes — it advances past the whole terminator, which for shape 1 hands the span's leading break entirely to the replacement and inserts a blank line. Both variants fail for the same underlying reason: they give the leading edge a different convention from the trailing edge, so the CRLF span covers strictly fewer terminators than its LF twin's span, and parity breaks.

Full reasoning in the top-level comment.

[Info] This pins the single-leading-newline case exactly as discussed, including the charAt(m.start() - 1) == '\r' assertion and the patched-bytes check — good, and I confirmed all four of those assertions independently against this head.

One observation for whoever extends this file: the strongest guard here would compare the patched output against an LF twin instead of against hand-written expected bytes, because the hand-written expectations currently encode the one case the ladder handles (\n-leading) while the multi-newline variant documented on startOffset passes no assertion at all. Adding a "\n\n…" needle to crlfBehavesLikeLfTwin's needle list would have caught it for free.

This test is replaced by crlfLeadingNewlinePatchMatchesLfTwin, which asserts the patched output against the LF twin rather than a hand-written span, over four shapes (single leading newline, two leading newlines, a replacement re-supplying both breaks, and no leading newline). It also pins alpha\nBETA\r\n for the first shape's exact bytes.

Two things it fixes relative to this one:

• The charAt(m.start() - 1) == '\r' assertion here encodes the convention that has now been dropped — the span's leading edge sits on the '\r' after all, so that assertion is no longer true and would be misleading if kept.
• This test passed on c6c15a7 while the two-newline shape was broken, which is the same blind spot as crlfBehavesLikeLfTwin: both compare a folded view where a stray '\r' is invisible. The new one folds only well-formed CRLF pairs, so a stray one survives the comparison and fails it.

[Info] Reverting startOffset and letting both edges read the one position map is the better shape — you are right that the single-step rule was the wrong kind of rule, not an undersized version of the right one, and my while suggestion would indeed have inserted a blank line (one\n\nBETA). I reproduced your table independently on fd782160d: the two-newline needle now patches to oneREPL\r\n (folded == the LF twin's oneREPL\n, no orphan CR), and all four cases of crlfLeadingNewlinePatchMatchesLfTwin plus the pinned alpha\nBETA\r\n hold here.

One sentence in this paragraph can be read more strongly than it holds, though: "every span edge lands on a terminator boundary and neither one splits a terminator" is true of the two normalising rungs, but EXACT is a raw indexOf and still opens mid-terminator. existing = "\r\nalpha beta\r\n", old_string = "\nalpha beta" matches at EXACT with start = 1 (the '\n' of the leading terminator), so new_string = "Q" yields "\rQ\r\n" — a stranded '\r'. Same shape for "x\n alpha \n" vs "x\n alpha": the LF document matches EXACT while its CRLF twin has to fall to TRAILING_WS_STRIPPED, so the two runs legitimately differ in which rung answers, and the CRLF one swallows the trailing spaces.

That behaviour is identical on main (\rQ\r\n there too), so it is out of scope for this PR and I am not asking for a code change here — just scoping the claim, e.g. "at the normalising levels every span edge lands on a terminator boundary; the exact level matches bytes verbatim and may open inside a terminator". If you would rather not widen the javadoc again, a one-line note on the EXACT bullet in the list above does the same job. Either way, non-blocking.

Confirmed your example by measurement, and scoped the claim.

"\r\nalpha beta\r\n" + "\nalpha beta" → level EXACT, one match, resolved span "\nalpha beta" (start = 1, the terminator's '\n'), patched with "Q" → "\rQ\r\n". Its LF twin also answers at EXACT and patches to "Q\n", so the two rungs genuinely differ there — pre-existing, as you say, since EXACT is untouched by this change.

The javadoc now reads:

…both the leading and the trailing boundary resolve through that same map entry, so at the normalising levels a span edge always lands on a terminator boundary and never splits one. Level.EXACT carries no such guarantee: it is a raw indexOf, so a needle that begins with a newline can open on the '\n' of a CRLF terminator and leave its '\r' behind, exactly as it does before this change. Either way only the span itself is ever replaced, so nothing outside it is rewritten.

I left the "nothing outside the span is rewritten" half unscoped rather than dropping it: the splice only ever replaces [start, end), so what EXACT loses is the boundary property, not that one. Both rungs are named in the class javadoc rather than only on the list item above, since that is where the overclaim was.

On your second point — the rung change is now in the commit message, but only because you prompted it: it was not there before, so thanks for the nudge.

[Info] The reworded paragraph is accurate, and I checked the EXACT caveat independently rather than taking the prose on trust. Compiled main and 3ca107fa side by side and ran the same shapes through both:

• "\r\nalpha beta\r\n" + "\nalpha beta" resolves at EXACT on both revisions and patches to "\rQ\r\n" on both — so "exactly as it does before this change" is literally true, the orphan CR is pre-existing and not a regression this PR introduced.
• The normalising rungs are where the guarantee holds, as scoped: "alpha\r\nbeta \r\ngamma\r\n" + "\nbeta\ngamma" with "\nBETA" gives "alpha\nBETA\r\n" on head against "alpha\nBETA\n" on its LF twin — parity after folding well-formed CRLF pairs, no split terminator.
• Your commit-message claim about the rung shift also measures out: "one\r\n\r\nalpha beta\r\n" + "\n\nalpha beta" now resolves at TRAILING_WS_STRIPPED on the CRLF document where main answered WHITESPACE_COLLAPSED, and patches to "oneREPL\r\n" instead of main's "one\r\n\r\nREPL\r\n". That is the model-visible report change worth having called out, and it is stated where a reader of patch will hit it.

Documenting the asymmetry instead of papering over it is the right call — the previous wording promised more than the code delivered at EXACT.