Skip to content

Fix correctness bugs found by review in the function-level merge engine#22

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
TheValiantOne merged 1 commit into from
Aug 10, 2026
Merged

Fix correctness bugs found by review in the function-level merge engine #22

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 9 additions & 4 deletions WitcherScriptMerger.Core/CLAUDE.md
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -426,10 +426,15 @@ function body only ever gains brace depth from control flow, never another funct
declaration. That structural simplicity is what makes plain brace/paren counting
sufficient, as long as it's string/comment-aware (a single masking pass shared by both
the brace-safe extraction path and the public `StripComments` helper) so a brace or
paren inside a string literal or comment can never be mistaken for real syntax. Reuses
`Tools/FileEncoding.cs` for all file I/O — mod files are inconsistently encoded even
though vanilla is always UTF-16LE+BOM (see "Text-merge input encoding" below), the exact
same hazard this class's own callers already have to account for.
paren inside a string literal or comment can never be mistaken for real syntax.
`ScriptUnitExtractor` itself does no file I/O at all — `Extract`/`StripComments` take
already-read `string` text — encoding normalization is the caller's job:
`DiffPlexMergeEngine.MergeHeadless` reads via `Tools/FileEncoding.cs` before ever
reaching this class, the same `ReadAnyEncoding` call every other text-merge path already
uses (mod files are inconsistently encoded even though vanilla is always UTF-16LE+BOM —
see "Text-merge input encoding" below). Any future caller that reaches `Extract`/
`StripComments` directly with raw file bytes, rather than through that existing
encoding-normalized path, would need to normalize first itself.

**Per-function resolution (`FunctionLevelMergeEngine.TryMerge`)** tries cheap one-sided
shortcuts (unchanged, only-one-side-edited, both-sides-made-the-identical-edit) before
Expand Down
84 changes: 59 additions & 25 deletions WitcherScriptMerger.Core/Inventory/FileMerger.cs
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -128,14 +128,28 @@ public class MergeReportData
bool _bundleChanged;
List<Merge> _pendingBundleMerges = new List<Merge>();

// Drained into HeadlessMergeSummary.FunctionLevelDecisions at the end of
// MergeConflictsHeadless. Appended to (not overwritten) right after every
// Keyed by relativePath (case-insensitive, matching merge.RelativePath's own
// comparison convention elsewhere in this class), drained into
// HeadlessMergeSummary.FunctionLevelDecisions at the end of
// MergeConflictsHeadless - but ONLY for paths that end up in summary.Merged,
// not summary.Skipped. Appended to (not overwritten) right after every
// MergeTextHeadless call, since _mergeEngine.LastFunctionLevelDecisions only
// reflects the single most recent pairwise MergeHeadless call - a multi-mod
// chain can trigger the function-level rescue at more than one step, and each
// one's decisions would otherwise be lost the moment the next chain step's
// MergeHeadless call resets that property back to empty.
List<string> _functionLevelDecisions = new List<string>();
//
// Keyed rather than a flat list (an earlier version of this field was a flat
// List<string>, unconditionally drained in full) because a chain can record
// real decisions for an EARLIER successful step and then fail at a LATER step
// (or, for a bundle, succeed at the text-merge level but fail its later
// blob0.bundle repack) - in either case the file ends up in summary.Skipped,
// and a flat, always-drained list would still report those decisions for a
// file that was never actually merged, contradicting the skipped/merged split
// a caller (e.g. the Vortex extension's merge panel) relies on. Keying by
// relativePath lets the drain step at the end of MergeConflictsHeadless include
// only the entries for paths that actually made it into summary.Merged.
Dictionary<string, List<string>> _functionLevelDecisionsByPath = new Dictionary<string, List<string>>(StringComparer.OrdinalIgnoreCase);

// Anchored at BOTH ends ("^...$") - see IsVanillaDlcBundleFolder's own comment
// below for why this matters: it's matched against just the extracted folder-name
Expand DownExpand Up@@ -296,7 +310,8 @@ void MergeFlatFileInteractive(InteractiveMergeRequest file, Merge merge, bool is

var source2 = file.OrderedSources[i];

var mergedFile = MergeTextInteractive(merge, source1, source2);
var oldDescription = DescribeAccumulated(file.OrderedSources.Take(i).Select(s => s.Name));
var mergedFile = MergeTextInteractive(merge, source1, source2, oldDescription, source2.Name);
if (mergedFile != null)
{
source1 = MergeSource.FromFlatFile(mergedFile, null);
Expand DownExpand Up@@ -336,7 +351,8 @@ void MergeBundleFileInteractive(InteractiveMergeRequest file, Merge merge, bool
break;
}

var mergedFile = MergeTextInteractive(merge, source1, source2);
var oldDescription = DescribeAccumulated(file.OrderedSources.Take(i).Select(s => s.Name));
var mergedFile = MergeTextInteractive(merge, source1, source2, oldDescription, file.OrderedSources[i].Name);
if (mergedFile != null)
{
source1 = MergeSource.FromFlatFile(mergedFile, null);
Expand All@@ -352,14 +368,14 @@ void MergeBundleFileInteractive(InteractiveMergeRequest file, Merge merge, bool
}
}

FileInfo MergeTextInteractive(Merge merge, MergeSource source1, MergeSource source2)
FileInfo MergeTextInteractive(Merge merge, MergeSource source1, MergeSource source2, string oldDescription = null, string newDescription = null)
{
// Deliberately engine-neutral wording rather than naming KDiff3 explicitly
// ("waiting for KDiff3 to close") - no external process or window is involved
// at all with DiffPlexMergeEngine.
ProgressInfo.CurrentAction = $"Merging {source1.Name} && {source2.Name}";

var result = _mergeEngine.Merge(source1, source2, _vanillaFile, _outputPath);
var result = _mergeEngine.Merge(source1, source2, _vanillaFile, _outputPath, oldDescription, newDescription);

if (result != MergeEngineResult.AutoSolved)
return null;
Expand DownExpand Up@@ -548,7 +564,22 @@ public HeadlessMergeSummary MergeConflictsHeadless(
}
}

summary.FunctionLevelDecisions.AddRange(_functionLevelDecisions);
// Only for paths that survived to summary.Merged - see
// _functionLevelDecisionsByPath's own comment for why a flat, unconditional
// drain here would misattribute decisions to a file that ultimately failed
// (a later chain step, or a bundle repack, both handled above this point).
// Uses summary.Merged's own casing for the output prefix, not
// merge.RelativePath's - `merge` can be an existing record pulled from
// _inventory.Merges via a case-insensitive match (see the isNew branch
// above), whose stored RelativePath could differ in casing from the
// freshly-scanned conflict.RelativePath that summary.Merged actually holds;
// the dictionary lookup itself is case-insensitive either way.
foreach (var relativePath in summary.Merged)
{
if (_functionLevelDecisionsByPath.TryGetValue(relativePath, out var decisionsForThisPath))
foreach (var decision in decisionsForThisPath)
summary.FunctionLevelDecisions.Add(relativePath + ": " + decision);
}

CleanUpTempFiles();
CleanUpEmptyDirectories();
Expand DownExpand Up@@ -587,32 +618,34 @@ bool MergeFlatConflictHeadless(ModFile conflict, Merge merge, string mergedModNa

_vanillaFile = new FileInfo(conflict.GetVanillaFile());

// Tracked locally, independent of merge.Mods (which can carry stale entries
// from a previous run when merge is a re-merge pulled from _inventory.Merges
// rather than freshly created) - this is only ever the real mod names folded
// into source1 so far within THIS chain, for FunctionLevelMergeEngine's
// Decisions[] audit text (see DiffPlexMergeEngine.TryFunctionLevelRescue's
// own comment on why source1.Name alone is misleading past the first step).
var accumulatedModNames = new List<string> { orderedNames[0] };

for (int i = 1; i < orderedNames.Length; ++i)
{
var hash = conflict.Mods.First(h => h.Name.EqualsIgnoreCase(orderedNames[i]));
var source2 = MergeSource.FromFlatFile(new FileInfo(conflict.GetModFile(orderedNames[i])), hash);

var oldDescription = accumulatedModNames.Count > 1
? "accumulated merge (" + string.Join(", ", accumulatedModNames) + ")"
: accumulatedModNames[0];

var mergedFile = MergeTextHeadless(merge, source1, source2, dryRun, oldDescription, orderedNames[i]);
var mergedFile = MergeTextHeadless(merge, source1, source2, dryRun, DescribeAccumulated(orderedNames.Take(i)), orderedNames[i]);
if (mergedFile == null)
return false;
source1 = MergeSource.FromFlatFile(mergedFile, null);
accumulatedModNames.Add(orderedNames[i]);
}
return true;
}

// The real mod names folded into "source1" so far within a merge chain, for
// FunctionLevelMergeEngine's Decisions[] audit text (see DiffPlexMergeEngine.
// TryFunctionLevelRescue's own comment on why source1.Name alone is misleading
// past a chain's first step - source1 becomes the prior step's accumulated
// output, whose own MergeSource.Name resolves to the merged-mod folder, not a
// real contributing mod). Deliberately takes namesSoFar fresh from the caller's
// own already-authoritative ordered list (orderedNames.Take(i) / OrderedSources.
// Take(i).Select(s => s.Name)) rather than a separately maintained list that
// would just be redundantly re-deriving the same prefix.
static string DescribeAccumulated(IEnumerable<string> namesSoFar)
{
var names = namesSoFar.ToList();
return names.Count > 1 ? "accumulated merge (" + string.Join(", ", names) + ")" : names[0];
}

bool MergeBundleConflictHeadless(ModFile conflict, Merge merge, string[] orderedNames, bool dryRun)
{
merge.BundleName = Path.GetFileName(Paths.RetrieveMergedBundlePath());
Expand DownExpand Up@@ -645,7 +678,7 @@ bool MergeBundleConflictHeadless(ModFile conflict, Merge merge, string[] ordered
if (!GetUnpackedFiles(conflict.RelativePath, ref source1, ref source2))
return false;

var mergedFile = MergeTextHeadless(merge, source1, source2, dryRun);
var mergedFile = MergeTextHeadless(merge, source1, source2, dryRun, DescribeAccumulated(orderedNames.Take(i)), orderedNames[i]);
if (mergedFile == null)
return false;
source1 = MergeSource.FromFlatFile(mergedFile, null);
Expand DownExpand Up@@ -746,8 +779,9 @@ FileInfo MergeTextHeadless(Merge merge, MergeSource source1, MergeSource source2

if (_mergeEngine.LastFunctionLevelDecisions.Count > 0)
{
foreach (var decision in _mergeEngine.LastFunctionLevelDecisions)
_functionLevelDecisions.Add(merge.RelativePath + ": " + decision);
if (!_functionLevelDecisionsByPath.TryGetValue(merge.RelativePath, out var decisionsForThisPath))
_functionLevelDecisionsByPath[merge.RelativePath] = decisionsForThisPath = new List<string>();
decisionsForThisPath.AddRange(_mergeEngine.LastFunctionLevelDecisions);
}

if (result != MergeEngineResult.AutoSolved)
Expand Down
57 changes: 44 additions & 13 deletions WitcherScriptMerger.Core/Tools/DiffPlexMergeEngine.cs
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -8,6 +8,7 @@
using DiffPlex;
using DiffPlex.Chunkers;
using DiffPlex.Model;
using WitcherScriptMerger.FileIndex;
using WitcherScriptMerger.Inventory;

namespace WitcherScriptMerger.Tools
Expand DownExpand Up@@ -232,7 +233,7 @@ public MergeEngineResult MergeHeadless(
// merge even when the whole-file 3-way diff hits this bug. Only
// attempted for .ws files - the extractor is WitcherScript-specific and
// has no notion of XML structure.
if (TryFunctionLevelRescue(baseText, oldText, newText, source1, source2, outputPath, oldDescription, newDescription))
if (TryFunctionLevelRescue(baseText, oldText, newText, source1, source2, outputPath, oldDescription, newDescription, openConflictMarkers))
return MergeEngineResult.AutoSolved;

// DiffPlex's own diff algorithm produced output it isn't safe to trust
Expand DownExpand Up@@ -270,7 +271,7 @@ public MergeEngineResult MergeHeadless(
// FunctionLevelMergeEngine's own comment for why this is a fallback that
// only ever activates where the whole-file merge has already failed, never
// a parallel code path for merges that would have succeeded anyway.
if (TryFunctionLevelRescue(baseText, oldText, newText, source1, source2, outputPath, oldDescription, newDescription))
if (TryFunctionLevelRescue(baseText, oldText, newText, source1, source2, outputPath, oldDescription, newDescription, openConflictMarkers))
return MergeEngineResult.AutoSolved;

// Never write conflict markers to outputPath itself: FileMerger's headless
Expand DownExpand Up@@ -417,9 +418,13 @@ static void DeleteIfExists(string path)
bool TryFunctionLevelRescue(
string baseText, string oldText, string newText,
FileMerger.MergeSource source1, FileMerger.MergeSource source2, string outputPath,
string oldDescription, string newDescription)
string oldDescription, string newDescription, bool openConflictMarkers)
{
if (!Path.GetExtension(outputPath).EqualsIgnoreCase(".ws"))
// ModFile.IsScript, not a locally reinvented extension check - the same
// vocabulary every other file-category dispatch in Core uses for this exact
// question. The extractor is WitcherScript-specific and has no notion of
// XML structure, so .xml conflicts never reach it.
if (!ModFile.IsScript(outputPath))
return false;

FunctionLevelMergeResult result;
Expand All@@ -430,13 +435,28 @@ bool TryFunctionLevelRescue(
source1.Name, source2.Name,
oldDescription ?? source1.Name, newDescription ?? source2.Name);
}
catch
catch (ScriptUnitExtractor.ExtractionException)
{
// A latent bug in the new engine must never regress this method below
// its pre-existing behavior - the caller falls through to whatever it
// was already about to do (write a sidecar, or report the
// DiffAlgorithmException as-is) exactly as if this rescue attempt had
// declined outright.
// The one expected, anticipated decline case (FunctionLevelMergeEngine.
// TryMerge itself already narrows to this same exception type) -
// genuinely just means this input doesn't parse cleanly, not a bug.
return false;
}
catch (Exception ex)
{
// Anything else is a genuine defect in the new engine, not an
// anticipated decline - still can't be allowed to regress this method
// below its pre-existing behavior (the caller falls through to
// whatever it was already about to do), but silently swallowing it
// with zero trace would make such a bug permanently unmeasurable from
// field reports alone. DialogIcon.Warning (not Information) so this
// routes to stderr under HeadlessMergeNotifier, never stdout - stdout
// carries MCP JSON-RPC frames only when running under the mcp verb,
// and writing arbitrary text there would corrupt the protocol stream.
AppState.Notifier.ShowMessage(
$"Function-level merge rescue hit an unexpected error for {source1.Name} + {source2.Name} " +
$"({ex.GetType().Name}: {ex.Message}) - falling back to the whole-file result.",
"Function-level rescue error", NotifyButtons.OK, DialogIcon.Warning);
return false;
}

Expand All@@ -449,10 +469,17 @@ bool TryFunctionLevelRescue(

if (result.Decisions.Count > 0)
{
// DialogIcon.Warning, not Information - see the unexpected-exception
// branch above for why Information (which HeadlessMergeNotifier routes
// to stdout) isn't safe here either; this message fires on every
// successful rescue with decisions to report, including during a dry
// run (openConflictMarkers is false only for the dry-run caller), so
// it's reachable far more often than the exception-logging branch.
var previewSuffix = openConflictMarkers ? "" : " (dry run preview - nothing was actually written)";
AppState.Notifier.ShowMessage(
$"Merged {source1.Name} + {source2.Name} at the function level after the whole-file merge " +
$"couldn't auto-solve it:\n\n" + string.Join("\n", result.Decisions),
"Merged (function-level)", NotifyButtons.OK, DialogIcon.Information);
$"couldn't auto-solve it{previewSuffix}:\n\n" + string.Join("\n", result.Decisions),
"Merged (function-level)", NotifyButtons.OK, DialogIcon.Warning);
}

return true;
Expand DownExpand Up@@ -696,7 +723,11 @@ static bool IsWhitespaceOnlyDifference(IReadOnlyList<string> oldPieces, IReadOnl
return NormalizeWhitespace(oldPieces) == NormalizeWhitespace(newPieces);
}

static string NormalizeWhitespace(IEnumerable<string> pieces)
// Internal, not private: FunctionLevelMergeEngine.NormalizeGap reuses this
// directly (a single-element pieces array) rather than keeping its own second
// copy of the same regex+trim logic - see that method's own comment for the
// real NBSP-related regression duplicating it once already caused.
internal static string NormalizeWhitespace(IEnumerable<string> pieces)
{
// Trim(WhitespaceChars), not the parameterless Trim() - see WhitespaceChars'
// own comment for the real NBSP-related bug this guards against.
Expand Down
Loading
Loading