fix(edit): preserve unrelated lines during fuzzy text matching#90060
fix(edit): preserve unrelated lines during fuzzy text matching#90060SebTardif wants to merge 7 commits into
Conversation
|
Codex review: needs maintainer review before merge. Reviewed July 1, 2026, 3:00 PM ET / 19:00 UTC. Summary PR surface: Source +123, Tests +233. Total +356 across 2 files. Reproducibility: yes. Source-level reproduction is high-confidence: current main replaces the write/diff base with whole-file normalizeForFuzzyMatch output whenever any edit uses fuzzy matching, and edit.ts writes that returned newContent; I did not execute tests in this read-only pass. Review metrics: none identified. Stored data model Root-cause cluster Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Risk before merge
Maintainer options:
Next step before merge
Security Review detailsBest possible solution: Land this PR, or an equivalent focused helper fix, after current CI and maintainer review confirm the Unicode offset mapper and regression coverage. Do we have a high-confidence way to reproduce the issue? Yes. Source-level reproduction is high-confidence: current main replaces the write/diff base with whole-file normalizeForFuzzyMatch output whenever any edit uses fuzzy matching, and edit.ts writes that returned newContent; I did not execute tests in this read-only pass. Is this the best way to solve the issue? Yes. The PR fixes the shared helper used by both execution and preview, keeping the original base and mapping only fuzzy match offsets; fixing only edit.ts would leave preview/caller drift. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 5db7c3797de3. Label changesLabel justifications:
Evidence reviewedPR surface: Source +123, Tests +233. Total +356 across 2 files. View PR surface stats
What I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
|
dfe9550 to
7bd1c13
Compare
mapNormalizedOffsetToOriginal assumed 1:1 within-line character positions, but NFKC normalization can expand compatibility characters (e.g., U+FB03 to "ffi"). This caused the fuzzy match replacement to splice at the wrong position when a NFKC-expanding character appeared before the match target on the same line. Add mapLineOffsetThroughNfkc that walks original code points and tracks cumulative NFKC expansion to compute the correct original offset. Includes a fast path when NFKC does not change the line length. Addresses ClawSweeper P1 finding on PR openclaw#90060.
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
mapNormalizedOffsetToOriginal assumed 1:1 within-line character positions, but NFKC normalization can expand compatibility characters (e.g., U+FB03 to "ffi"). This caused the fuzzy match replacement to splice at the wrong position when a NFKC-expanding character appeared before the match target on the same line. Add mapLineOffsetThroughNfkc that walks original code points and tracks cumulative NFKC expansion to compute the correct original offset. Includes a fast path when NFKC does not change the line length. Addresses ClawSweeper P1 finding on PR openclaw#90060.
dad5205 to
06626c4
Compare
06626c4 to
fd227bb
Compare
mapNormalizedOffsetToOriginal assumed 1:1 within-line character positions, but NFKC normalization can expand compatibility characters (e.g., U+FB03 to "ffi"). This caused the fuzzy match replacement to splice at the wrong position when a NFKC-expanding character appeared before the match target on the same line. Add mapLineOffsetThroughNfkc that walks original code points and tracks cumulative NFKC expansion to compute the correct original offset. Includes a fast path when NFKC does not change the line length. Addresses ClawSweeper P1 finding on PR openclaw#90060.
Adopted from PR openclaw#90060 (by @SebTardif) with ClawSweeper P1 fix: The original PR's NFKC fast-path checked length equality (nfkcLine.length === origLine.length) to skip offset mapping. This is incorrect — NFKC can preserve total code-unit length while shifting internal positions. For example, "fi X é" (U+FB01 + X + e+combining-acute) normalizes to "fi X é" — both are 6 code units, but X shifts from codepoint 2 to 3. Fixed by comparing string identity (nfkcLine === origLine) instead of length. Added regression test for this exact length-neutral mixed case. Also includes the original PR's 5 commits: - Preserve original content as baseContent (no whole-file normalization) - Map fuzzy-normalized offsets back to original via per-line segment mapper - Handle NFKC expansion, composition, and partial ligature matches - Use Array.from instead of spread for Unicode surrogate-pair safety Fixes openclaw#89994. Upstream PR: openclaw#90060
Adopted from PR openclaw#90060 (by @SebTardif) with ClawSweeper P1 fix: The original PR's NFKC fast-path checked length equality (nfkcLine.length === origLine.length) to skip offset mapping. This is incorrect — NFKC can preserve total code-unit length while shifting internal positions. For example, "fi X é" (U+FB01 + X + e+combining-acute) normalizes to "fi X é" — both are 6 code units, but X shifts from codepoint 2 to 3. Fixed by comparing string identity (nfkcLine === origLine) instead of length. Added regression test for this exact length-neutral mixed case. Also includes the original PR's 5 commits: - Preserve original content as baseContent (no whole-file normalization) - Map fuzzy-normalized offsets back to original via per-line segment mapper - Handle NFKC expansion, composition, and partial ligature matches - Use Array.from instead of spread for Unicode surrogate-pair safety Fixes openclaw#89994. Upstream PR: openclaw#90060
Adopted from PR openclaw#90060 (by @SebTardif) with ClawSweeper P1 fix: The original PR's NFKC fast-path checked length equality (nfkcLine.length === origLine.length) to skip offset mapping. This is incorrect — NFKC can preserve total code-unit length while shifting internal positions. For example, "fi X é" (U+FB01 + X + e+combining-acute) normalizes to "fi X é" — both are 6 code units, but X shifts from codepoint 2 to 3. Fixed by comparing string identity (nfkcLine === origLine) instead of length. Added regression test for this exact length-neutral mixed case. Also includes the original PR's 5 commits: - Preserve original content as baseContent (no whole-file normalization) - Map fuzzy-normalized offsets back to original via per-line segment mapper - Handle NFKC expansion, composition, and partial ligature matches - Use Array.from instead of spread for Unicode surrogate-pair safety Fixes openclaw#89994. Upstream PR: openclaw#90060
|
@SebTardif can you fix the rank up moves for this PR? Should get this PR ready for maintainer view. |
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. |
When a fuzzy match was needed, applyEditsToNormalizedContent normalized the entire file content (stripping trailing whitespace, replacing smart quotes with ASCII, converting em dashes to hyphens, applying NFKC) and used that as the base for splicing. This silently rewrote every line in the file, not just the matched region. Fix: keep the original content as the base and map fuzzy match positions back to the original using a line-based offset table. normalizeForFuzzyMatch preserves line count and performs 1:1 character replacements within lines, so per-line column mapping is correct. Closes openclaw#89994 Signed-off-by: Sebastien Tardif <[email protected]>
mapNormalizedOffsetToOriginal assumed 1:1 within-line character positions, but NFKC normalization can expand compatibility characters (e.g., U+FB03 to "ffi"). This caused the fuzzy match replacement to splice at the wrong position when a NFKC-expanding character appeared before the match target on the same line. Add mapLineOffsetThroughNfkc that walks original code points and tracks cumulative NFKC expansion to compute the correct original offset. Includes a fast path when NFKC does not change the line length. Addresses ClawSweeper P1 finding on PR openclaw#90060.
Group each base character with its following combining marks before normalizing with NFKC, so cross-code-point composition (e.g., e + U+0301 combining acute accent composing to é) is mapped correctly. The previous per-code-point normalization missed these compositions and could splice the wrong original range. Add regression test with a decomposed combining character before a fuzzy match to cover the composition case. Signed-off-by: Sebastien Tardif <[email protected]>
Signed-off-by: Sebastien Tardif <[email protected]>
The fast path in mapLineOffsetThroughNfkc returned the normalized offset directly when origLine.length === nfkcLine.length. This is wrong because NFKC can shift character positions while preserving total length (e.g., U+FB01 expands 1->2 chars while e+U+0301 composes 2->1 chars, netting to zero). Changed the fast path to compare the full strings (nfkcLine === origLine) so length-neutral NFKC changes always go through the segment walker. Added a regression test using a line with both expansion (U+FB01) and composition (e+U+0301) that is length-neutral under NFKC.
ee15401 to
17ee761
Compare
…uping
The mapLineOffsetThroughNfkc function previously only grouped base
characters with following combining marks (\p{M}). NFKC can also compose
adjacent non-mark code points (e.g., Hangul Compatibility Jamo
U+3131 + U+314F compose to U+AC00). This caused offset miscalculation
when such sequences appeared before a fuzzy-matched region.
Replace the combining-mark-only grouping with a greedy approach that
extends each segment as long as the next code point participates in NFKC
composition (i.e., normalizing the extended segment differs from
normalizing each part separately).
Add a Hangul Jamo regression test that verifies correct offset mapping
when Compatibility Jamo compose under NFKC.
Signed-off-by: Sebastien Tardif <[email protected]>
|
@clawsweeper review |
|
🦞🧹 I asked ClawSweeper to review this item again. |
|
This pull request has been automatically marked as stale due to inactivity. |
|
Closing as completed. The fuzzy-edit bug this PR targeted is fixed on main. Maintainer note on #89994: This branch is stale and conflicts with that path, so there is nothing useful left to land here. Thanks to everyone who reviewed and tested along the way. |
Fixes #89994
Summary
When
applyEditsToNormalizedContentneeded fuzzy matching for any edit, it normalized the entire file vianormalizeForFuzzyMatchand used that as the base for all splicing. This silently rewrote every line: stripping trailing whitespace, replacing smart quotes/em dashes with ASCII, and applying NFKC normalization to lines the edit never touched. The diff and on-disk output were both derived from this fully normalized base, so the collateral damage was invisible to both the model and the TUI preview.Fix
Instead of normalizing the entire file when a fuzzy match is needed, the fix:
baseContent(always)mapNormalizedOffsetToOriginalhelper that maps character positions from fuzzy-normalized space back to original space using a line-based offset tableThis works because
normalizeForFuzzyMatchpreserves line count and performs 1:1 character replacements within each line (smart quotes, dashes, special spaces, NFKC). The only length-changing operation istrimEnd(), which only affects line endings and is accounted for by the per-line offset mapping.Real behavior proof
Behavior addressed: A single fuzzy edit silently normalizes and rewrites every line in the file, corrupting smart quotes, em dashes, and trailing whitespace on lines the edit never touched.
Real environment tested: macOS 15.5, Node v26.0.0, OpenClaw built from patched source at
/tmp/wt-89994.Exact steps or command run after this patch:
Step 1. Build from branch and verify fix in compiled dist:
Step 2. Run tsx proof script that exercises the production code path:
Evidence after fix: terminal output from the patched build:
The fix is active in the production bundle:
Terminal confirms fuzzy edits now preserve unrelated lines:
Observed result after fix: Terminal output confirms that after the fix, fuzzy-matched edits only modify the targeted region. Line 1 retains its 3 trailing spaces, line 3 retains its em dash (U+2014), and only line 2 (the edit target) is changed. Before this fix, all 3 lines would have been silently normalized.
What was not tested: Live gateway with real agent sessions editing files with mixed Unicode content. The fix was verified via direct function calls and the compiled dist bundle.