Skip to content

Exclude the merged mod from the conflict scan so re-merges stay idempotent - #37

Open
TheValiantOne wants to merge 1 commit into
mainfrom
fix/exclude-merged-mod-from-scan
Open

Exclude the merged mod from the conflict scan so re-merges stay idempotent#37
TheValiantOne wants to merge 1 commit into
mainfrom
fix/exclude-merged-mod-from-scan

Conversation

@TheValiantOne

Copy link
Copy Markdown
Owner

What's wrong

ModFileIndex.BuildAsync enumerates Directory.GetDirectories(ModsDirectory, "mod*") and filters the result through GetIgnoredModNames() — which honored only the IgnoreModNames setting. mod0000_MergedFiles is this tool's own output, but its name starts with mod, so it matched the same glob and was scanned as an ordinary source mod.

Every re-merge therefore fed the previous run's output back in alongside the mods it was built from, re-applying their edits on top of already-merged text. A re-merge was cumulative, not idempotent. Inserted blocks gained a fresh copy per run, and a losing most-distinct-from-vanilla tiebreak could additionally revert an edit a previous run had kept.

Nothing surfaced it as an error. The output stayed syntactically valid, braces balanced, and every run reported Merged N file(s), skipped 0. — which is why it accumulated unnoticed.

How it was found

A routine re-merge on a live 249-mod install, after adding mods. Measured against each mod's own file as ground truth:

live merged outputthe mod's own filevanilla
modBloodAndSteelInCustomDodge() in actor.ws0
modCriSlowMoCRdismemberChance in damageManagerProcessor.ws0

Across the merged set: 37 duplicated mod-added lines in 11 of 42 files, plus one modTTMutagenSwap edit reverted outright (OnSkillMutagenUnequipped(..., false)true).

The fix

  • GetIgnoredModNames() now excludes MergedModName on top of whatever the user configured.
  • Split out ModFileIndex.BuildIgnoredModNames(ignoreSetting, mergedNameSetting) — a pure function over the two raw setting values, so it's unit-testable without touching AppState.Settings (see WitcherScriptMerger.Tests/CLAUDE.md's safety constraints).
  • Added Paths.NormalizeMergedModName(string), a non-interactive counterpart to RetrieveMergedModName(). The scan path must not use the latter — it can prompt via ConfirmInvalidModName and message through AppState.Notifier, neither of which may fire just because mod directories are being enumerated. Both share the new Paths.MergedModNameMaxLength (64) truncation, so the excluded name always equals the directory a merge actually writes. NormalizeMergedModName also trims, deliberately — its result is compared against a DirectoryInfo.Name, which never carries surrounding whitespace.

Second, smaller fix

The WinForms merge verb gated on the combined Paths.ValidateDependencyPaths(), so it refused to start at all without QuickBMS/wcc_lite — even when every conflict was flat-file .ws/.xml. Neither binary is committed to this repo, so a plain clone-and-run hit it every time, with an error pointing at the GUI's dependency setup for tooling the run didn't need. It now uses ValidateTextMergeDependencies(), matching WitcherScriptMerger.Headless. Bundle conflicts still degrade gracefully per-conflict.

This is what blocked the original merge and is why the run that exposed the main bug went through the Headless host instead.

Verification

Unit tests — 15 new in FileIndex/ModFileIndexTests.cs, 173 total, all passing. Covers the bug itself (exclusion with no IgnoreModNames configured), a non-default MergedModName, user entries surviving alongside, case-insensitive de-duplication when the merged mod is already listed by hand (the pre-fix workaround), blank/unconfigured MergedModName adding no phantom entry, and the truncation + trimming behavior.

A/B against origin/main — built both hosts and ran each against a scratch game tree seeded with a stale mod0000_MergedFiles whose two marker strings appear in no mod and not in vanilla:

PRE-FIX (origin/main)
merged mod named as a merge participant: 1 decision line(s)
function-level: game\foo.ws: function Alpha: kept mod0000_MergedFiles's version ...
stale-marker leakage: {'StaleAccumulatedMarker': 1, 'staleAccumulatedField': 1}
VERDICT: LEAKED - merged mod was scanned as a source mod
POST-FIX (this branch)
merged mod named as a merge participant: 0 decision line(s)
stale-marker leakage: {'StaleAccumulatedMarker': 0, 'staleAccumulatedField': 0}
VERDICT: clean - merged mod ignored, rebuilt from vanilla + real mods only

A simpler two-mod fixture does not reproduce it — with a clean prior merge the extra input converges to the same result. The stale-output fixture is what isolates the mechanism.

Live install — a clean rebuild of all 42 merged files, verified against every contributing mod's own source: 0 duplicated mod-added lines, where the previous output had 37. Also re-checked encoding (UTF-16LE + BOM on all 42), brace balance against vanilla, enum integrity, and no duplicate function definitions beyond vanilla's own 31 legitimate same-name cases.

Second fix — the WinForms merge verb now completes with no QuickBMS/wcc_lite present, printing [Bundle Checking Unavailable] ... skipping bundle-content checking for this scan. then Merged 1 file(s), skipped 0. (exit 0). Previously exit 1 with no merge performed.

dotnet format whitespace --verify-no-changes clean.

Note for users on an existing install

Any mod0000_MergedFiles produced before this fix may already carry accumulated duplicates. The fix prevents further accumulation but does not retroactively clean existing output — delete the merged mod folder once and re-merge to get a correct rebuild.

AI-assisted development

This PR was produced by Claude Code (Opus 5), including the investigation that found the bug on a live install, the fix, the tests, and the A/B verification above. Per CONTRIBUTING.md: everything reported here was actually run, not inferred — the marker counts, the A/B output, and the live-install rebuild figures are from real runs against real data.

…otent
The mod-directory filter honored only IgnoreModNames, so mod0000_MergedFiles -
this tool's own output - matched BuildAsync's "mod*" glob and got scanned as an
ordinary source mod. Every re-merge then fed the previous run's output back in
alongside the mods it was built from, re-applying their edits on top of
already-merged text: inserted blocks gained a fresh copy per run, and a losing
most-distinct-from-vanilla tiebreak could revert an edit a previous run had kept.
Found on a live 249-mod install after a routine re-merge: a single
modBloodAndSteel insertion present 6x in actor.ws and a modCriSlowMoCR one 6x in
damageManagerProcessor.ws (each appears exactly once in the mod's own file), 37
duplicated mod-added lines across 11 of 42 merged files, and one modTTMutagenSwap
edit reverted outright. Nothing surfaced this as an error - the output stayed
syntactically valid and merged "successfully" every time, which is why it went
unnoticed across repeated merges.
GetIgnoredModNames now also excludes MergedModName, via a new pure
BuildIgnoredModNames(ignoreSetting, mergedNameSetting) that's unit-testable
without touching AppState.Settings, and Paths.NormalizeMergedModName - a
non-interactive counterpart to RetrieveMergedModName (which can prompt via
ConfirmInvalidModName, and must not on a scan path) sharing its new
MergedModNameMaxLength truncation so the excluded name always equals the
directory a merge actually writes.
Also drops the WinForms merge verb's bundle-tool gate: it used the combined
ValidateDependencyPaths(), so `merge` refused to start at all without
QuickBMS/wcc_lite even when every conflict was flat-file. Neither binary is
committed to this repo, so a plain clone-and-run hit that refusal every time.
Now matches Headless's text-merge-only gate; bundle conflicts still degrade
gracefully per-conflict.
15 new tests (173 total). Verified A/B against origin/main on a scratch tree
carrying a stale mod0000_MergedFiles whose marker strings appear in no mod and
not in vanilla: pre-fix those markers leaked into the new output and the decision
log named mod0000_MergedFiles as a merge participant; post-fix, zero leakage and
no such decisions. Re-verified on the live install - a clean rebuild has 0
duplicated mod lines where the previous output had 37.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FP8H6rBLCGPBFRSVsF3Kgw
Sign up for freeto 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.

1 participant

@TheValiantOne