Harden MCP tools for minimal-rights operation

[TheValiantOne]

Summary

Unit 7 of the ongoing re-architecture batch: MCP hardening for minimal-rights operation. Covers all four items from the unit's scope.

1. Directory allow-listing. merge_conflicts's relativePaths and orderOverrides keys are now validated (WsmMcpTools.EnsureInScope/IsWithinModsDirectory) to resolve inside Paths.ModsDirectory before any scan or merge runs — using Path.GetFullPath plus a proper prefix check against the fully-qualified root (with a trailing separator, so ModsDirectory can't be spoofed by a sibling directory like ModsDirectoryEvil), not a naive StartsWith. Path.IsPathRooted rejects absolute paths and UNC paths outright (both would otherwise silently discard the mods-directory prefix via Path.Combine's own "second arg rooted → first arg ignored" behavior). Out-of-scope entries are rejected with a clear ArgumentException before any scan/merge, listing every offending entry, rather than silently matching nothing.

Important finding while implementing this: relativePaths was never actually joined into a filesystem path anywhere in this codebase — it's only ever compared for equality against already-scanned ModFile.RelativePath values. So this closes no live traversal; it's defense-in-depth plus fixing the "silently ignored" half of the requirement (a malicious-looking entry used to just fail to match anything, with no signal to the caller that something suspicious was rejected vs. simply absent).

2. Audit of all 4 tools. orderOverrides values reach Path.Combine (via FileMerger.GetModFile) but only after being validated against ModFile.ContainsMod — a whitelist of real, currently-scanned mod folder names — so that path was already safe. scan_conflicts, get_status, and list_merges take no path-shaped parameters at all.

Two real (non-path-traversal) bugs found and fixed in orderOverrides validation, both "silently succeeds having done less than the caller intended":

• A partial or duplicate mod list (["modA"] for a 2-mod conflict, or ["modA","modA"]) previously passed every existing check and would merge an incomplete or self-paired chain while still reporting the file as fully merged.
• Once a file's been merged once, its own merged-mod folder re-enters conflict.Mods as a pseudo-source (scan_conflicts's own description already warns clients about this). An override naming only the one remaining real source mod after that (["modA"] when conflict.Mods is [mergedMod, modA]) would satisfy a naive "must cover every real source" check while still merging nothing, since a merge chain needs at least a pair.

FileMerger.ResolveMergeOrder now requires: no unknown mod names, no duplicates, at least two entries, and every one of the file's real source mods (explicitly excluding the configured merged-mod name) present at least once. This method is shared with the merge CLI verb's --order-file (Program.cs → MergeOperations.RunMerge → MergeConflictsHeadless → ResolveMergeOrder), so this is a real behavior change there too: an existing --order-file that lists a subset of a file's mods — previously silently accepted — now gets rejected for that file (Skipped, CLI exit code flips 0→2 for an unchanged config in that scenario). Documented in root CLAUDE.md's CLI mode section.

3. dryRun mode added to merge_conflicts. scan_conflicts's alreadyResolved only re-checks existing merge records' hashes — it can't tell you whether a currently unresolved conflict would actually auto-solve, since that requires exercising the merge engine. dryRun: true does exactly that, without: writing merged output, repacking a bundle, or modifying MergeInventory.xml.

Two things I found and fixed while making that promise actually hold:

• MergeInventory.Load() can call Save() on its own (AddMissingHashes backfilling an older-schema record's missing hash) before dryRun is ever otherwise consulted. MergeInventory.Load gained an allowSave parameter (default true, unchanged for every other caller — the CLI verb, scan_conflicts, list_merges, the GUI); merge_conflicts passes allowSave: !dryRun.
• The dry-run output redirect (writing to a scratch path under TempBundleContent\DryRun\... instead of the real destination) initially skipped the existing "output already exists, decline to overwrite" check entirely (HeadlessMergeNotifier always declines). That made a dry run disagree with what a real run of the identical conflict would actually do whenever the real output already existed. Both MergeFlatConflictHeadless and MergeBundleConflictHeadless now check existence against the real would-be output path regardless of dryRun, and only redirect the write target for a dry run.

merge is always a throwaway object during a dry run rather than one pulled from _inventory.Merges — a structural guarantee (not just an if (!dryRun) at each mutation site) that a dry-run pass can't mutate a live inventory record even indirectly.

4. WitcherScriptMerger.Core/Mcp/CLAUDE.md (new — no parallel unit had created one) documents minimal required permissions: standard user file I/O only, three roots (configured mods/game dirs, and the app's own install directory for MergeInventory.xml/scratch dirs — Program.RunCli pins Environment.CurrentDirectory = AppContext.BaseDirectory, verified empirically that a caller-supplied working directory has no effect), no admin rights, no network beyond the stdio transport itself.

Response shape change

merge_conflicts now returns {merged, skipped, unmatched, dryRun} (was {merged, skipped}). unmatched lists any relativePaths entry that's in-scope but doesn't match a currently-detected conflict (resolved/removed between scan and call) — distinct from an out-of-scope entry, which is a hard error for the whole call, not a per-entry unmatched.

Known limitations / deliberate scope boundaries (disclosed, not fixed)

• Categories.BundleText scope-check semantics are unexercised. For that category, conflict.RelativePath is a path internal to a bundle archive (from QuickBms.GetBundleContentPaths), not one rooted at Paths.ModsDirectory — an ordinary internal path validates fine, but a bundle whose internal listing itself contained a rooted/..-bearing entry would make that conflict unreachable via relativePaths. Every scratch config used for verification has CheckBundleContents=false (no real QuickBMS/wcc_lite binaries were available in the verification environment), consistent with the bundle path's pre-existing "code-reviewed but not round-tripped" status in CLAUDE.md.
• Bundle repack failures are inherently invisible to a dry run — dryRun never calls PackNewBundle/wcc_lite at all, by design, so it can't detect a repack-time failure a real run might hit.
• A relativePaths entry matching a real, in-scope, non-mergeable-category conflict (Categories.FlatNotMergeable/BundleNotMergeable) currently appears in none of merged/skipped/unmatched — it's excluded from unmatched (it did match a real conflict) but then falls out of MergeConflictsHeadless's category filter before reaching merged/skipped. Pre-existing filter behavior; unmatched is new and implies more completeness than this edge case has.
• A 3+-mod dryRun chain's intermediate merge-source label is cosmetic-only wrong (ModFile.GetModNameFromPath's fallback returns the constant "Merged Bundle Content" for the dry-run scratch path instead of a meaningful name) — affects only progress-text/diagnostic labeling, not correctness. Not exercised by the 2-mod smoke tests below.
• Directory allow-listing is deliberately MCP-only, not applied to the CLI's --order-file path — the CLI is invoked directly by a trusted local operator with full filesystem access already; there's no untrusted-remote-caller threat model there the way there explicitly is for an MCP client. orderOverrides values are safe on both paths regardless, via ModFile.ContainsMod's whitelist.
• EnsureInScope aborts the entire merge_conflicts call on any single out-of-scope entry, rather than degrading per-conflict like other validation failures in the same method (ResolveMergeOrder's errors). Deliberate: this is the one validation that's actually about a caller attempting to escape the intended scope, and fail-closed on attack-shaped input is the more conservative posture — a batch with one poisoned entry processes nothing rather than quietly tolerating the attempt as "just another skip reason."
• Rebuilding orderOverrides into a case-insensitive, separator-normalized dictionary (WsmMcpTools.MergeConflicts) uses Dictionary.ToDictionary, which throws a plain ArgumentException if two original keys collide after normalization (e.g. differing only by casing or / vs \) — surfaces as the same generic isError: true tool response as every other validation failure here, just without saying which keys collided.

AI assistance disclosure

Implemented by Claude Code per CONTRIBUTING.md. Code-review skill invoked against the diff, returned 13 findings; the ones above under "fixed" were addressed, the ones under "known limitations" were deliberately left as documented, disclosed tradeoffs rather than expanding this unit's scope further.

Verification

• dotnet build WitcherScriptMerger.sln — succeeds, same 5 pre-existing CA1823 warnings as main, no new warnings.
• dotnet format whitespace WitcherScriptMerger.sln --verify-no-changes — clean.
• Real host exe, hand-rolled MCP stdio client (initialize → tools/list → tools/call), against a scratch game/mods tree, dependency paths pointed at real-but-inert stand-ins (where.exe as a launchable KDiff3 stand-in, 0-byte placeholders for QuickBMS/wcc_lite, CheckBundleContents=false) so Paths.ValidateDependencyPaths() passes without needing real KDiff3/QuickBMS/wcc_lite (unavailable in this environment):
  • tools/list schema reflects the new dryRun parameter and updated descriptions.
  • get_status/list_merges/scan_conflicts unchanged and correct against a real synthetic 2-mod conflict.
  • merge_conflicts with a ..\..\..\Windows\System32\... entry, an absolute C:\Windows\... entry, a UNC \\server\share\... entry, a null array element, and a malicious orderOverrides key — all rejected cleanly (isError: true, detailed reason logged server-side to stderr, generic message to the client — itself appropriate hardening, not leaking internal path info back to a potentially untrusted caller) with zero filesystem changes (verified via before/after recursive snapshots of the scratch tree, not just the response text).
  • merge_conflicts with a legitimate but non-existent relativePaths entry correctly returns it in unmatched rather than being silently dropped or erroring.
• In-process harness (disposable scratch console app referencing WitcherScriptMerger.Core directly, per CLAUDE.md's Tests section precedent) with a fake IMergeEngine that always reports AutoSolved and physically writes its output — since the stand-in KDiff3 above can only ever prove the "would not auto-solve" path, this was necessary to prove the accurate-preview path (a real KDiff3/QuickBMS/wcc_lite install still wasn't available in this environment, so this stub is what makes the following meaningful, and no claim about real KDiff3 behavior rests on it):
  • dryRun: true on a genuinely auto-solvable 2-mod conflict: reports merged, but the real output file, tempbundlecontent, and MergeInventory.xml all remain absent throughout.
  • The identical call with dryRun: false immediately after: reports merged, and now the real output file exists with the expected content, MergeInventory.xml exists with 1 correctly-hashed record.
  • orderOverrides validation: partial list, duplicate entry, and unknown mod name each rejected with the expected error message and skipped; a valid full permutation (reversed order) succeeds.
  • The documented "already merged, re-merge with just the real source mods" scenario (conflict.Mods = [mergedMod, modA, modB], override = [modA, modB]) succeeds — this is exactly the case a naive "must cover every one of conflict.Mods" check would have wrongly rejected.
  • The single-remaining-real-source regression case (conflict.Mods = [mergedMod, modA], override = [modA]) is correctly rejected ("must name at least two mods to merge"), and the real output file is confirmed byte-for-byte unchanged — not silently "merged" with no work done.
  • The dry-run/real-run agreement fix: with a real output file already on disk from an earlier real merge, dryRun: true on the same conflict now correctly reports skipped (predicting the "already exists, declined" outcome) instead of falsely reporting merged.
• Grepped the full diff for machine-specific absolute paths (C:\Users\..., AppData\Local\Temp\...) before committing — clean.