Fix correctness bugs found by review in the function-level merge engine - #22
Merged
Merged
Conversation
A multi-angle code review of the just-merged function-level merge engine (PR #18) surfaced several real bugs: - The function-level rescue's success notification used DialogIcon. Information, which HeadlessMergeNotifier routes to stdout - corrupting the MCP JSON-RPC stream on every rescue with decisions to report. Now DialogIcon.Warning (stderr), matching every other message in this class. - A file with zero extracted functions/fields (e.g. only top-level consts/enums) crashed with IndexOutOfRangeException in GetSideGapIndex, silently swallowed by a bare catch - the rescue just never fired for exactly this file shape. Fixed the index bug, and separately made TryMerge decline outright when vanilla has no units at all: the entire document is one "gap" in that case, and gap content always reverts to vanilla on reassembly, so "succeeding" here would have silently discarded both mods' real edits instead of the safe decline this now is. - ReconcileInsertions crashed via ToDictionary when one side inserted two same-named units at the same slot - now declines cleanly instead. - NormalizeGap used plain Trim() instead of the NBSP-safe Trim(WhitespaceChars), reintroducing a bug DiffPlexMergeEngine. NormalizeWhitespace was already fixed for. Now reuses that method directly (promoted to internal) instead of a second, drifted copy. - A gap slot made "ineligible" for precise comparison by a nearby deletion got no signal at all when its content actually differed - silently contradicting this engine's own "never empty content silently" contract. Now emits a conservative, location-described caveat note. - MergeBundleConflictHeadless and MergeTextInteractive (both flat and bundle) never threaded the accumulated-mod-names description MergeFlatConflictHeadless already built, so a bundle-packed or interactively-merged multi-mod chain got a misleading "kept MergedBundleContent's edit" instead of naming the real mods - including in a real GUI modal on the interactive path. All four call sites now share one DescribeAccumulated helper. - Function-level decisions recorded during an earlier, successful chain step still got reported even when a later step failed (or, for a bundle, the repack failed) and the file ended up in summary.Skipped, not summary.Merged - plus a casing mismatch between merge.RelativePath and conflict.RelativePath could break exact-match correlation by a downstream consumer. Decisions are now tracked per relative path and only drained for paths that survived to summary.Merged. - The bare catch around the new engine swallowed every exception with zero trace, making a genuine bug indistinguishable from an expected decline. Narrowed to the one anticipated exception type; anything else is now reported via the notifier instead of vanishing silently. - ScriptUnitExtractor's @addField lookup rescanned from the current position to EOF on every extracted unit - for a large vanilla file with zero @addField annotations (a mod-only construct), this meant millions of wasted line scans per rescue attempt. Now a single forward pass computed once, walked with a monotonic pointer. - Fixed a stale CLAUDE.md claim that ScriptUnitExtractor reuses FileEncoding.cs for file I/O - it takes pre-read strings and does none. Re-validated end-to-end against an isolated copy of the same 5 real previously-failing files used to validate PR #18: all 5 still merge successfully after these fixes, with well-formed output and no conflict markers. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GXAuGMLB44T5Zv5o5ZzKah
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
A multi-angle code review of the just-merged function-level merge engine (#18) surfaced several real bugs, all fixed here:
TryFunctionLevelRescue's success notification usedDialogIcon.Information, whichHeadlessMergeNotifierroutes to stdout — corrupting the MCP JSON-RPC stream on every rescue that had decisions to report. NowDialogIcon.Warning(stderr), matching every other message in the class..wsfile with no extracted functions/fields (e.g. only top-level consts/enums) crashed withIndexOutOfRangeExceptioninGetSideGapIndex, silently swallowed by a barecatch— the rescue just never fired for exactly this file shape. Fixed the index bug, and separately madeTryMergedecline outright when vanilla has zero units: the whole document is one "gap" in that case, and gap content always reverts to vanilla on reassembly — so "succeeding" there would have silently discarded both mods' real edits instead of the safe decline this now is.ReconcileInsertionsthrew viaToDictionarywhen one side inserted two same-named units at the same slot — now declines cleanly.NormalizeGapused plainTrim()instead of the NBSP-safeTrim(WhitespaceChars), reintroducing a bugDiffPlexMergeEngine.NormalizeWhitespacewas already fixed for. Now reuses that method directly (promoted tointernal) instead of a drifted second copy.MergeBundleConflictHeadlessandMergeTextInteractive(both flat and bundle) never threaded the accumulated-mod-names descriptionMergeFlatConflictHeadlessalready built, so a bundle-packed or interactively-merged multi-mod chain got a misleading "kept MergedBundleContent's edit" instead of naming the real mods — including in a real GUI modal on the interactive path. All four call sites now share oneDescribeAccumulatedhelper.summary.Skipped, notsummary.Merged— plus a casing mismatch betweenmerge.RelativePathandconflict.RelativePathcould break exact-match correlation downstream. Decisions are now tracked per relative path and only drained for paths that survived tosummary.Merged.catcharound the new engine swallowed every exception with zero trace, making a genuine bug indistinguishable from an expected decline. Narrowed to the one anticipated exception type; anything else is now reported via the notifier instead of vanishing silently.ScriptUnitExtractor's@addFieldlookup rescanned from the current position to EOF on every extracted unit — for a large vanilla file with zero@addFieldannotations (a mod-only construct), that's millions of wasted line scans per rescue attempt. Now a single forward pass computed once, walked with a monotonic pointer.CLAUDE.mdclaim thatScriptUnitExtractorreusesFileEncoding.csfor file I/O — it takes pre-read strings and does none.Not changed (flagged, not acted on): two review angles raised a philosophical concern about the "most distinct from vanilla wins" tiebreak silently resolving genuine edit-vs-edit collisions with no separate review tier. That's the explicitly-requested, approved design from #18, not a bug — left as-is.
AI-assisted development disclosure (per
CONTRIBUTING.md): this PR, including the review that found these issues and the fixes themselves, was developed with Claude Code.Test plan
dotnet build WitcherScriptMerger.sln --configuration Release— cleandotnet format whitespace WitcherScriptMerger.sln --verify-no-changes— cleandotnet test WitcherScriptMerger.sln— 128/128 passing, including 4 new regression tests (zero-vanilla-units decline, duplicate-insertion-name decline, plus updated assertions on 3 existing tests to cover the new ineligible-deletion caveat notes)actor.ws, still merge successfully after these fixes, with well-formed UTF-16LE+BOM output and no conflict markers.🤖 Generated with Claude Code
https://claude.ai/code/session_01GXAuGMLB44T5Zv5o5ZzKah