From 27252f52b3d7b507e8e4142ea321da28f9b56d87 Mon Sep 17 00:00:00 2001 From: Chris Knight Date: Mon, 10 Aug 2026 21:43:52 -0400 Subject: [PATCH] Add Vortex coexistence & Collections handling (Unit K) Detect, warn, and reconcile when Vortex's built-in game-witcher3 Script Merger integration modifies WSM merge state out from under this companion extension - Collections import (importScriptMerges) and per-profile merge backup/restore (mergeBackup.ts's storeToProfile/restoreFromProfile) can both overwrite merge output with no way for this extension to intercept them (vortex-api has no extension-blocking API). New src/coexistenceGuard.ts snapshots two independent signals (a list_merges-derived signature, and a plain recursive listing of the real merged-mod folder - the latter needed because each companion binary keeps its own separate MergeInventory.xml, so the folder is the one resource genuinely shared with game-witcher3's own discovered tool) and compares them at three trigger points wired from index.ts: gamemode-activated, profile-did-change (not profile-will-change, which Vortex never awaits), and did-deploy - positioned above the isModOrDependencyInstallActive gate so a Collection install is still observed. resolveAction.ts's own merge workflow reconciles the baseline afterward: silently for a real merge (this extension's own write), but through the same compare-and-warn path for a dry-run preview, so a preview-then-cancel can't silently erase evidence of undetected drift. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01GXAuGMLB44T5Zv5o5ZzKah --- vortex-extension/src/coexistenceGuard.test.ts | 461 ++++++++++++++++ vortex-extension/src/coexistenceGuard.ts | 495 ++++++++++++++++++ vortex-extension/src/conflictNotifications.ts | 12 +- vortex-extension/src/index.test.ts | 277 +++++++++- vortex-extension/src/index.ts | 77 +++ vortex-extension/src/resolveAction.test.ts | 114 +++- vortex-extension/src/resolveAction.ts | 66 ++- .../test/coexistenceGuard.integration.test.ts | 203 +++++++ 8 files changed, 1693 insertions(+), 12 deletions(-) create mode 100644 vortex-extension/src/coexistenceGuard.test.ts create mode 100644 vortex-extension/src/coexistenceGuard.ts create mode 100644 vortex-extension/test/coexistenceGuard.integration.test.ts diff --git a/vortex-extension/src/coexistenceGuard.test.ts b/vortex-extension/src/coexistenceGuard.test.ts new file mode 100644 index 0000000..d359abb --- /dev/null +++ b/vortex-extension/src/coexistenceGuard.test.ts @@ -0,0 +1,461 @@ +import * as fs from 'fs'; +import * as os from 'os'; +import * as path from 'path'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +// Isolates refreshCoexistenceState's own orchestration (connect/compute/check/close) from +// conflictScan.ts's real, filesystem-based isWsmToolAcquired/getWsmExePath - the same +// rationale as index.test.ts's own mock of this module. Every other export tested in this +// file (computeMergeStateSnapshot, checkCoexistenceDrift, the signature helpers) takes +// its inputs as plain arguments and never reaches this module, so mocking it here has no +// effect on those tests. +const { isWsmToolAcquiredMock, getWsmExePathMock } = vi.hoisted(() => ({ + isWsmToolAcquiredMock: vi.fn(), + getWsmExePathMock: vi.fn(), +})); + +vi.mock('./conflictScan', () => ({ + isWsmToolAcquired: isWsmToolAcquiredMock, + getWsmExePath: getWsmExePathMock, +})); + +import { WSM_CONFLICTS_NOTIFICATION_ID } from './conflictNotifications'; +import { GetStatusResult, ListMergesResult, WsmMcpClientOptions } from './mcpClient'; +import { + buildFolderListingSignature, + checkCoexistenceDrift, + computeMergeHistorySignature, + computeMergeStateSnapshot, + MergeStateClient, + MergeStateSnapshot, + recordOwnMergeStateSnapshot, + refreshCoexistenceState, + resetCoexistenceGuardState, + WSM_COEXISTENCE_NOTIFICATION_ID, +} from './coexistenceGuard'; + +function merge(relativePath: string, mergedModName = 'mod0000_MergedFiles', mods: Array<{ name: string; hash: string }> = []): ListMergesResult[number] { + return { relativePath, mergedModName, mods }; +} + +function status(overrides: Partial = {}): GetStatusResult { + return { + gameDirectory: 'C:\\Games\\Witcher3', + modsDirectory: 'C:\\Games\\Witcher3\\Mods', + dependenciesValid: true, + textMergeDependenciesValid: true, + bundleDependenciesValid: true, + modsDirectoryExists: true, + mergedModName: 'mod0000_MergedFiles', + conflictCount: 0, + ...overrides, + }; +} + +function fakeApi() { + const notifications: unknown[] = []; + return { + getState: () => ({}), + sendNotification: vi.fn((notification: unknown) => { + notifications.push(notification); + return WSM_COEXISTENCE_NOTIFICATION_ID; + }), + dismissNotification: vi.fn(), + showDialog: vi.fn(async () => ({ action: 'Close', input: {} })), + notifications, + }; +} + +describe('computeMergeHistorySignature', () => { + it('is order-independent (sorted before joining)', () => { + const a = computeMergeHistorySignature([merge('b.ws'), merge('a.ws')]); + const b = computeMergeHistorySignature([merge('a.ws'), merge('b.ws')]); + expect(a).toBe(b); + }); + + it('is empty for no recorded merges', () => { + expect(computeMergeHistorySignature([])).toBe(''); + }); + + it('differs when a recorded merge is added, removed, or its mods/hashes change', () => { + const base = computeMergeHistorySignature([merge('a.ws', 'mod0000_MergedFiles', [{ name: 'modA', hash: 'h1' }])]); + const added = computeMergeHistorySignature([ + merge('a.ws', 'mod0000_MergedFiles', [{ name: 'modA', hash: 'h1' }]), + merge('b.ws'), + ]); + const changedHash = computeMergeHistorySignature([merge('a.ws', 'mod0000_MergedFiles', [{ name: 'modA', hash: 'h2' }])]); + + expect(base).not.toBe(added); + expect(base).not.toBe(changedHash); + }); +}); + +describe('buildFolderListingSignature', () => { + let tmpDir: string; + + beforeEach(() => { + tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'wsm-coexistence-')); + }); + + afterEach(() => { + fs.rmSync(tmpDir, { recursive: true, force: true }); + }); + + it('is empty for a folder that does not exist (ENOENT tolerated, not an error)', async () => { + const signature = await buildFolderListingSignature(path.join(tmpDir, 'does-not-exist')); + expect(signature).toBe(''); + }); + + it('is empty for an existing, empty folder', async () => { + const signature = await buildFolderListingSignature(tmpDir); + expect(signature).toBe(''); + }); + + it('lists nested files with relative paths, order-independent of directory read order', async () => { + fs.mkdirSync(path.join(tmpDir, 'sub')); + fs.writeFileSync(path.join(tmpDir, 'b.ws'), 'content-b'); + fs.writeFileSync(path.join(tmpDir, 'sub', 'a.ws'), 'content-a'); + + const signature = await buildFolderListingSignature(tmpDir); + + expect(signature).toContain('b.ws:'); + expect(signature).toContain(path.join('sub', 'a.ws') + ':'); + // Sorted: "b.ws" sorts after "sub/a.ws" is not guaranteed either way by content, but + // the signature itself must be deterministic regardless of fs.readdir's own order - + // verified by the "order-independent" test below instead of asserting exact order + // here. + }); + + it('changes when a file\'s size changes', async () => { + fs.writeFileSync(path.join(tmpDir, 'a.ws'), 'short'); + const before = await buildFolderListingSignature(tmpDir); + + fs.writeFileSync(path.join(tmpDir, 'a.ws'), 'a much longer piece of content than before'); + const after = await buildFolderListingSignature(tmpDir); + + expect(before).not.toBe(after); + }); + + it('is stable across two reads of the same unchanged content', async () => { + fs.writeFileSync(path.join(tmpDir, 'a.ws'), 'unchanged'); + fs.writeFileSync(path.join(tmpDir, 'b.ws'), 'also unchanged'); + + const first = await buildFolderListingSignature(tmpDir); + const second = await buildFolderListingSignature(tmpDir); + + expect(first).toBe(second); + }); + + // Regression test for a real gap caught in code review: an earlier version of + // walkFilesRecursive checked only isFile()/isDirectory(), silently skipping a symlink + // entry entirely (isSymbolicLink() true, the other two both false) - neither recording + // nor recursing into it, so a change reachable only via a symlink inside the merged-mod + // folder would never show up in this signature at all. + // + // Symlink creation can fail with EPERM on Windows without Developer Mode or an + // elevated process (confirmed: this is a real, common CI/dev-machine restriction, not a + // hypothetical) - this test skips itself gracefully rather than failing the whole suite + // on a machine where symlinks simply aren't creatable, since that's an environment + // limitation, not a signal this module's own logic is broken. + it('records a symlinked file (its own link metadata, not silently skipped)', async () => { + const targetPath = path.join(tmpDir, 'target.ws'); + const linkPath = path.join(tmpDir, 'link.ws'); + fs.writeFileSync(targetPath, 'target content'); + + try { + fs.symlinkSync(targetPath, linkPath, 'file'); + } catch (err) { + if ((err as NodeJS.ErrnoException).code === 'EPERM') { + return; // no symlink privilege on this machine - nothing more this test can check + } + throw err; + } + + const signature = await buildFolderListingSignature(tmpDir); + + expect(signature).toContain('link.ws:'); + expect(signature).toContain('target.ws:'); + }); +}); + +describe('computeMergeStateSnapshot', () => { + let tmpDir: string; + + beforeEach(() => { + tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'wsm-coexistence-snapshot-')); + }); + + afterEach(() => { + fs.rmSync(tmpDir, { recursive: true, force: true }); + }); + + function fakeClient(mergedFolder: string, merges: ListMergesResult): MergeStateClient { + return { + getStatus: async () => status({ modsDirectory: tmpDir, mergedModName: mergedFolder }), + listMerges: async () => merges, + }; + } + + it('joins get_status\'s modsDirectory + mergedModName to locate the folder listing, and reflects list_merges too', async () => { + fs.mkdirSync(path.join(tmpDir, 'mod0000_MergedFiles')); + fs.writeFileSync(path.join(tmpDir, 'mod0000_MergedFiles', 'a.ws'), 'merged content'); + + const snapshot = await computeMergeStateSnapshot(fakeClient('mod0000_MergedFiles', [merge('a.ws')])); + + expect(snapshot.mergedModName).toBe('mod0000_MergedFiles'); + expect(snapshot.folderListingSignature).toContain('a.ws:'); + expect(snapshot.mergeHistorySignature).toBe(computeMergeHistorySignature([merge('a.ws')])); + }); + + it('does not require the merged-mod folder to already exist (get_status is not gated on modsDirectoryExists per WsmMcpTools.GetStatus)', async () => { + // No directory created under tmpDir at all - simulates a fresh install where + // get_status still reports a real modsDirectory/mergedModName (both plain config + // reads) even though nothing has ever been merged yet. + const snapshot = await computeMergeStateSnapshot(fakeClient('mod0000_MergedFiles', [])); + + expect(snapshot.folderListingSignature).toBe(''); + expect(snapshot.mergeHistorySignature).toBe(''); + }); +}); + +describe('checkCoexistenceDrift', () => { + beforeEach(() => { + resetCoexistenceGuardState(); + }); + + function snapshot(overrides: Partial = {}): MergeStateSnapshot { + return { + folderListingSignature: 'a.ws:10:1000', + mergeHistorySignature: 'a.ws:mod0000_MergedFiles:modA=hash1', + mergedModName: 'mod0000_MergedFiles', + ...overrides, + }; + } + + it('seeds the baseline on the first observation without notifying - nothing to compare against yet', () => { + const api = fakeApi(); + + checkCoexistenceDrift(api as never, snapshot()); + + expect(api.sendNotification).not.toHaveBeenCalled(); + }); + + it('does not notify when the snapshot is unchanged from the baseline', () => { + const api = fakeApi(); + + checkCoexistenceDrift(api as never, snapshot()); + checkCoexistenceDrift(api as never, snapshot()); + + expect(api.sendNotification).not.toHaveBeenCalled(); + }); + + it('sends a distinctly-branded, non-conflicts notification when the folder listing changes', () => { + const api = fakeApi(); + + checkCoexistenceDrift(api as never, snapshot()); + checkCoexistenceDrift(api as never, snapshot({ folderListingSignature: 'a.ws:99:9999' })); + + expect(api.sendNotification).toHaveBeenCalledTimes(1); + const notification = api.notifications[0] as { id: string; type: string; allowSuppress?: boolean }; + expect(notification.id).toBe(WSM_COEXISTENCE_NOTIFICATION_ID); + expect(notification.type).toBe('warning'); + // Deliberately not suppressible by default, unlike the ordinary conflicts + // notification - see coexistenceGuard.ts's own doc comment. + expect(notification.allowSuppress).not.toBe(true); + }); + + it('dismisses the ordinary conflicts notification on a genuine drift, so a stale count never lingers', () => { + const api = fakeApi(); + + checkCoexistenceDrift(api as never, snapshot()); + checkCoexistenceDrift(api as never, snapshot({ folderListingSignature: 'changed' })); + + expect(api.dismissNotification).toHaveBeenCalledWith(WSM_CONFLICTS_NOTIFICATION_ID); + }); + + it('does not dismiss anything on the first observation or when nothing changed', () => { + const api = fakeApi(); + + checkCoexistenceDrift(api as never, snapshot()); + checkCoexistenceDrift(api as never, snapshot()); + + expect(api.dismissNotification).not.toHaveBeenCalled(); + }); + + it('sends a notification when the merge-history signature changes, even if the folder listing does not', () => { + const api = fakeApi(); + + checkCoexistenceDrift(api as never, snapshot()); + checkCoexistenceDrift(api as never, snapshot({ mergeHistorySignature: 'a.ws:mod0000_MergedFiles:modA=hash2' })); + + expect(api.sendNotification).toHaveBeenCalledTimes(1); + }); + + it('only notifies once for a single change, not again on every subsequent identical re-check', () => { + const api = fakeApi(); + const changed = snapshot({ folderListingSignature: 'changed' }); + + checkCoexistenceDrift(api as never, snapshot()); + checkCoexistenceDrift(api as never, changed); + checkCoexistenceDrift(api as never, changed); + checkCoexistenceDrift(api as never, changed); + + expect(api.sendNotification).toHaveBeenCalledTimes(1); + }); + + it('does not throw when sendNotification itself throws', () => { + const api = fakeApi(); + api.sendNotification.mockImplementationOnce(() => { + throw new Error('notification system unavailable'); + }); + + checkCoexistenceDrift(api as never, snapshot()); + expect(() => checkCoexistenceDrift(api as never, snapshot({ folderListingSignature: 'changed' }))).not.toThrow(); + }); + + // Regression test for the ordering fix mirroring conflictNotifications.ts's own + // notifyConflictsIfChanged: the baseline must only advance after sendNotification + // actually succeeds, or a failed attempt would be silently treated as "already + // reported" for the rest of the session - the exact bug notifyConflictsIfChanged's own + // doc comment explains avoiding. + it('retries the notification on the next check when sendNotification failed - does not silently drop a real drift', () => { + const api = fakeApi(); + checkCoexistenceDrift(api as never, snapshot()); + + api.sendNotification.mockImplementationOnce(() => { + throw new Error('notification system unavailable'); + }); + const changed = snapshot({ folderListingSignature: 'changed' }); + checkCoexistenceDrift(api as never, changed); + expect(api.sendNotification).toHaveBeenCalledTimes(1); // the failed attempt + + // Same still-changed snapshot re-checked at the next checkpoint - since the failed + // attempt never advanced the baseline, this must be treated as a fresh, real drift + // and retried, not silently matched against a baseline that was never actually + // reported to the user. + checkCoexistenceDrift(api as never, changed); + expect(api.sendNotification).toHaveBeenCalledTimes(2); + }); + + it('recordOwnMergeStateSnapshot updates the baseline without notifying, and a later external change is still caught against the new baseline', () => { + const api = fakeApi(); + + checkCoexistenceDrift(api as never, snapshot()); + // This extension's own merge changes state - re-baseline, no notification. + recordOwnMergeStateSnapshot(snapshot({ folderListingSignature: 'own-merge-result' })); + expect(api.sendNotification).not.toHaveBeenCalled(); + + // A later external change, compared against the *new* (own-merge) baseline, not the + // original one - still detected. + checkCoexistenceDrift(api as never, snapshot({ folderListingSignature: 'externally-changed' })); + expect(api.sendNotification).toHaveBeenCalledTimes(1); + }); +}); + +describe('refreshCoexistenceState', () => { + beforeEach(() => { + resetCoexistenceGuardState(); + isWsmToolAcquiredMock.mockReset(); + getWsmExePathMock.mockReset().mockReturnValue('C:\\wsm\\WitcherScriptMerger.Headless.exe'); + }); + + function fakeStatus(overrides: Partial = {}): GetStatusResult { + return { + gameDirectory: 'C:\\Games\\Witcher3', + modsDirectory: 'C:\\Games\\Witcher3\\Mods', + dependenciesValid: true, + textMergeDependenciesValid: true, + bundleDependenciesValid: true, + modsDirectoryExists: true, + mergedModName: 'mod0000_MergedFiles', + conflictCount: 0, + ...overrides, + }; + } + + function fakeConnectApi() { + return { getState: () => ({}), sendNotification: vi.fn(), dismissNotification: vi.fn(), showDialog: vi.fn() }; + } + + it('does nothing - never connects - when no WSM tool has been acquired', async () => { + isWsmToolAcquiredMock.mockResolvedValue(false); + const connect = vi.fn(); + + await refreshCoexistenceState(fakeConnectApi() as never, { connect }); + + expect(connect).not.toHaveBeenCalled(); + }); + + it('connects with a bounded per-request timeout at the exe path conflictScan.ts resolves, computes a snapshot, and closes the client', async () => { + isWsmToolAcquiredMock.mockResolvedValue(true); + const closeSpy = vi.fn(async () => undefined); + const fakeClient = { + getStatus: vi.fn(async () => fakeStatus()), + listMerges: vi.fn(async () => []), + close: closeSpy, + }; + const connect = vi.fn(async (_options: WsmMcpClientOptions) => fakeClient); + + await refreshCoexistenceState(fakeConnectApi() as never, { connect: connect as never }); + + expect(connect).toHaveBeenCalledTimes(1); + const options = connect.mock.calls[0][0] as WsmMcpClientOptions; + expect(options.exePath).toBe('C:\\wsm\\WitcherScriptMerger.Headless.exe'); + // Bounded, not mcpClient.ts's own 30s default - this call sits inside Vortex's + // did-deploy emitAndAwait window on one of its trigger points (index.ts's + // checkForConflictsAfterDeploy) - see coexistenceGuard.ts's own + // COEXISTENCE_CHECK_TIMEOUT_MS doc comment. + expect(options.requestTimeoutMs).toBe(15_000); + expect(fakeClient.getStatus).toHaveBeenCalledTimes(1); + expect(fakeClient.listMerges).toHaveBeenCalledTimes(1); + expect(closeSpy).toHaveBeenCalledTimes(1); + }); + + it('resolves (never rejects/throws) when connect itself rejects', async () => { + isWsmToolAcquiredMock.mockResolvedValue(true); + const connect = vi.fn(async () => { + throw new Error('spawn failed'); + }); + + await expect(refreshCoexistenceState(fakeConnectApi() as never, { connect: connect as never })).resolves.toBeUndefined(); + }); + + it('still closes the client, and still resolves without throwing, when getStatus itself rejects', async () => { + isWsmToolAcquiredMock.mockResolvedValue(true); + const closeSpy = vi.fn(async () => undefined); + const fakeClient = { + getStatus: vi.fn(async () => { + throw new Error('get_status failed'); + }), + listMerges: vi.fn(async () => []), + close: closeSpy, + }; + const connect = vi.fn(async () => fakeClient); + + await expect(refreshCoexistenceState(fakeConnectApi() as never, { connect: connect as never })).resolves.toBeUndefined(); + expect(closeSpy).toHaveBeenCalledTimes(1); + }); + + it('sends the distinct coexistence notification end-to-end when two real checks observe a genuine change', async () => { + isWsmToolAcquiredMock.mockResolvedValue(true); + let listMergesCallCount = 0; + const connect = vi.fn(async () => ({ + getStatus: vi.fn(async () => fakeStatus()), + listMerges: vi.fn(async () => { + listMergesCallCount += 1; + // First call (seeds the baseline): no recorded merges. Second call: one - + // a real, observable change in mergeHistorySignature between the two checks. + return listMergesCallCount === 1 ? [] : [{ relativePath: 'a.ws', mergedModName: 'mod0000_MergedFiles', mods: [] }]; + }), + close: vi.fn(async () => undefined), + })); + const api = fakeConnectApi(); + + await refreshCoexistenceState(api as never, { connect: connect as never }); + expect(api.sendNotification).not.toHaveBeenCalled(); // first observation only seeds the baseline + + await refreshCoexistenceState(api as never, { connect: connect as never }); + expect(api.sendNotification).toHaveBeenCalledTimes(1); + expect((api.sendNotification.mock.calls[0][0] as { id: string }).id).toBe(WSM_COEXISTENCE_NOTIFICATION_ID); + }); +}); diff --git a/vortex-extension/src/coexistenceGuard.ts b/vortex-extension/src/coexistenceGuard.ts new file mode 100644 index 0000000..45b70ec --- /dev/null +++ b/vortex-extension/src/coexistenceGuard.ts @@ -0,0 +1,495 @@ +import * as fs from 'fs'; +import * as path from 'path'; +import { log, selectors, types } from 'vortex-api'; +import { resetConflictNotificationState, WSM_CONFLICTS_NOTIFICATION_ID } from './conflictNotifications'; +import { getWsmExePath, isWsmToolAcquired } from './conflictScan'; +import { WITCHER3_GAME_ID } from './gating'; +import { GetStatusResult, ListMergesResult, WsmMcpClient } from './mcpClient'; +import { buildWsmEnv, mergeWithProcessEnv } from './wsmEnv'; + +/** + * Unit K: coexistence & Collections handling. + * + * `docs/vortex-extension-design.md` section 0 and Open Question 1 name two concrete + * correctness hazards from Vortex's own built-in `game-witcher3` extension, which this + * extension coexists with rather than replaces (see `gating.ts`'s own doc comment): + * Collections import (`importScriptMerges()`) overwriting merge output on confirmation, + * and per-profile merge backup/restore (`mergeBackup.ts`'s `storeToProfile`/ + * `restoreFromProfile`, wired from `eventHandlers.ts`'s `onProfileWillChange` off the + * `profile-will-change` event). **There is no `vortex-api` mechanism to intercept, + * disable, or block another extension's own registrations or event handlers** (re- + * confirmed against the published `lib/api.d.ts` - no such method exists on + * `IExtensionContext`/`IExtensionApi`), so this module cannot prevent either hazard. Its + * job is **detect, warn, reconcile** - notice that WSM's own merge state changed without + * going through this extension's own workflow, tell the user distinctly from an ordinary + * "you have conflicts" notification, and point them at the existing remediation surfaces + * (the merge-history dashlet, `mergeHistoryDashlet.ts`; the "Resolve Script Conflicts" + * action, `resolveAction.ts`) rather than trying to auto-fix anything itself. + * + * **Two mechanistic corrections to the design doc's own framing, found by fetching and + * reading the real `Nexus-Mods/Vortex` monorepo source directly (`gh api + * repos/Nexus-Mods/Vortex/contents/extensions/games/game-witcher3/src/...`), not carried + * over from the design doc's own summary** - both change how "detect" has to work here, + * so they're recorded with their exact citations rather than only in this unit's PR + * description: + * + * 1. **`storeToProfile`/`restoreFromProfile` operate on a *different* `MergeInventory.xml` + * than this extension's own, by default.** `mergeBackup.ts`'s `handleMergedScripts` + * resolves the file it moves (`MERGE_INV_MANIFEST`, `"MergeInventory.xml"` - + * `common.ts`) against `path.dirname(scriptMergerTool.path)`, where `scriptMergerTool` + * is the discovery entry for `SCRIPT_MERGER_ID = "W3ScriptMerger"` (`common.ts`) - + * `game-witcher3`'s *own* discovered tool, a separately-acquired binary (typically the + * `IDCs/WitcherScriptMerger` fork it auto-downloads - design doc section 0). This + * extension registers a **distinct** tool id, `WSM_TOOL_ID = 'WitcherScriptMergerEnhanced'` + * (`discoveredTool.ts`), at its own acquired path (`storage.ts`'s `getWsmToolDir`). + * Per WSM's own `Paths.Inventory` (`WitcherScriptMerger.Core/Paths.cs`), resolved + * against `Environment.CurrentDirectory` - which both hosts pin to + * `AppContext.BaseDirectory` before dispatching to `merge`/`mcp` + * (`WitcherScriptMerger.Core/Mcp/CLAUDE.md`) - each binary's `MergeInventory.xml` + * lives next to *that* binary, not in some shared location. So in the default, + * two-separate-binaries configuration, `storeToProfile`/`restoreFromProfile` (and + * `importScriptMerges`, which calls the same `handleMergedScripts`) never touch this + * extension's own `MergeInventory.xml` file directly. What genuinely *is* shared, + * confirmed from the same source: `handleMergedScripts`'s `mergedScriptsPath = + * path.join(gamePath, "Mods", mergedModName)` - the real, physical merged-mod-content + * folder inside the actual game `Mods` directory Vortex manages, which every WSM + * instance (whichever binary produced it) reads/writes via the identical + * `GameDirectory`/`ModsDirectory` resolution (design doc section 4.1). That shared + * folder, not `MergeInventory.xml` itself, is the resource both hazards actually + * contend over - which is why this module snapshots the folder's own contents + * (`buildFolderListingSignature` below), not only `MergeInventory.xml`-derived state, + * despite `list_merges`/`MergeInventory.xml` being the only *schema* knowledge this + * unit is meant to lean on (see `computeMergeHistorySignature`'s own doc comment for + * why that half still matters: it's the one signal that *does* catch the case where a + * user has pointed `game-witcher3`'s own `W3ScriptMerger` discovery at the exact same + * binary this extension acquired). + * 2. **The per-profile backup/restore hazard is opt-in per profile, not automatic.** + * `mergeBackup.ts`'s `genBaseProps` returns `undefined` (a no-op) unless + * `state.persistent.profiles[profileId].features.local_merges` is `true` - and + * `game-witcher3`'s own `index.ts` registers that feature + * (`context.registerProfileFeature("local_merges", "boolean", "settings", "Profile + * Data", "This profile will store and restore profile specific data (merged scripts, + * loadorder, etc) when switching profiles", ...)`) as an ordinary Vortex profile + * toggle, **defaulting to unset/false**. So hazard 2 does not fire "on every profile + * switch" as a blanket statement - only for a profile the user has explicitly opted + * into that feature. This doesn't change what this module does (it still can't tell + * *why* merge state changed, only *that* it did), but it does mean hazard 2 is rarer + * in practice than the design doc's own phrasing suggests. + * + * **Detection mechanism.** A snapshot-and-compare approach, per this unit's own task + * description, combining two independent signals so a change is caught regardless of + * which of the two directory configurations above applies: + * + * - `mergeHistorySignature`: an order-independent signature over `list_merges()`'s + * result (`RecordedMerge[]`) - reuses `mcpClient.ts`'s existing `WsmMcpClient.listMerges` + * (the same MCP tool `mergeHistoryDashlet.ts`'s `fetchMergeHistory` already calls, per + * this unit's own instructions to build on that path rather than re-parsing + * `MergeInventory.xml` independently), mirroring `conflictNotifications.ts`'s + * `computeConflictSignature` shape (sorted, delimited, newline-joined - the same + * Windows-path-safe delimiter reasoning applies here unchanged). + * - `folderListingSignature`: a plain, no-XML-parsing recursive directory listing (path + * relative to the merged-mod folder, size, mtime) of the actual merged-mod-content + * folder on disk, per the citation above - the one signal that also catches the shared- + * folder-only hazard. Located via `get_status`'s own `modsDirectory`/`mergedModName` + * fields (`WsmMcpTools.GetStatus()`, `WitcherScriptMerger.Core/Mcp/WsmMcpTools.cs`) - + * confirmed by reading that method directly that both are plain `AppState.Settings`/ + * `Paths` reads, populated unconditionally, *not* gated on `modsDirectoryExists` (a + * separate field on the same response) - so they're reliable even when nothing has + * been merged yet or the mods directory doesn't exist. `buildFolderListingSignature` + * itself tolerates a missing folder (treats it as an empty listing) for exactly that + * reason, rather than requiring the caller to pre-check `modsDirectoryExists`. + * + * **Trigger points** (wired from `index.ts`): `gamemode-activated` into witcher3 (already + * listened to for tool re-registration), `profile-did-change` (fires *after* a profile + * switch completes, per `@nexusmods/vortex-api`'s `docs/EVENTS.md` - `(profileId: string)` + * - a new listener this unit adds), and `did-deploy`, **positioned above (before) the + * existing `isModOrDependencyInstallActive` early-return** in `checkForConflictsAfterDeploy` + * - installing a Collection is precisely the window `isModOrDependencyInstallActive` + * exists to detect and skip *conflict scanning* during, but it's also precisely the + * window `importScriptMerges()` (hazard 1) actually runs in, so a coexistence check gated + * behind that same early-return would never see the deployment where the overwrite + * happened. Deliberately **not** wired off `profile-will-change` itself: that event is a + * plain, synchronous `events.on` emit (`EVENTS.md` marks `will-deploy`/`did-deploy`. + * "Async.", not this one), so `game-witcher3`'s own `onProfileWillChange` handler (also + * registered via `events.on`, confirmed in its `index.ts`) is never awaited by Vortex + * before continuing - there is no reliable way to snapshot "before" its file moves and + * compare "after" from a second listener on the same event. Every trigger point above is + * instead a *later* checkpoint that re-observes current, settled state, which is what + * makes an idempotent snapshot-and-compare workable here at all: `profile-will-change` + * itself is skipped, `profile-did-change` (after the switch, and after `game-witcher3`'s + * own async handler has had time to run, though not provably so) is used instead. + * + * **Re-baselining after this extension's own writes.** `resolveAction.ts`'s + * `runMergeConflictsWorkflow` calls `recordOwnMergeStateSnapshot` (not + * `checkCoexistenceDrift`) immediately after a successful `mergeConflicts` call, using the + * same already-open client (no extra spawn). This is necessary, not just tidy: without + * it, this extension's *own* successful merges would themselves look like external + * interference the next time a trigger point re-checks, since a real merge genuinely does + * change both signals above. Re-snapshotting immediately (rather than tracking a "we just + * merged" flag/timestamp) is deliberate too - idempotent, no timing window to race against + * a later checkpoint, and it can't accidentally swallow a genuine external change that + * happens to land inside some suppression window. + */ + +export const WSM_COEXISTENCE_NOTIFICATION_ID = 'witcherscriptmerger-vortex-coexistence-drift'; + +/** Combined, comparable snapshot of "everything about WSM's merge state this extension + * can observe without re-implementing `MergeInventory.xml`'s own XML schema or WSM's + * own hashing (`Tools/Hasher.cs`)" - see this module's own header doc comment for why + * both halves are needed. */ +export interface MergeStateSnapshot { + folderListingSignature: string; + mergeHistorySignature: string; + /** Carried along for notification copy only - not itself part of the comparison + * (`mergeHistorySignature`/`folderListingSignature` already reflect anything that + * matters about it). */ + mergedModName: string; +} + +/** The subset of `WsmMcpClient` this module needs - lets callers (and this module's own + * tests) pass an already-connected client without depending on the full class, matching + * `resolveAction.ts`'s own `WsmMergeClient`/`wsmStatusSummary.ts`'s own narrowed-surface + * seams. */ +export interface MergeStateClient { + getStatus(): Promise; + listMerges(): Promise; +} + +function isEnoent(err: unknown): boolean { + return typeof err === 'object' && err !== null && (err as NodeJS.ErrnoException).code === 'ENOENT'; +} + +/** + * Order-independent signature over `list_merges()`'s result. Deliberately mirrors + * `conflictNotifications.ts`'s `computeConflictSignature` shape (per-entry field join, + * sorted, newline-joined) rather than inventing a different convention - same + * Windows-reserved-character delimiter reasoning applies unchanged (`:`/`|`/`\n` cannot + * appear in a relative path, a mod name, or a hex hash string). + */ +export function computeMergeHistorySignature(merges: ListMergesResult): string { + return merges + .map( + (m) => + `${m.relativePath}:${m.mergedModName}:${[...m.mods.map((mod) => `${mod.name}=${mod.hash}`)].sort().join('|')}`, + ) + .sort() + .join('\n'); +} + +/** Recursively walks `dir`, appending one `"::"` entry per + * file (relative to `baseDir`) to `out`. A missing directory (the merged-mod folder + * doesn't exist yet - nothing has ever been merged, or the mods directory itself is + * absent) is treated as "no entries", not an error - this module's own doc comment + * explains why `computeMergeStateSnapshot` doesn't pre-check `modsDirectoryExists` + * before calling this. Any other error (a permissions problem, a locked file mid-scan) + * propagates - silently treating that as "empty" would risk a false "everything was + * deleted" drift signal instead of surfacing the real problem. */ +async function walkFilesRecursive(dir: string, baseDir: string, out: string[]): Promise { + let entries: fs.Dirent[]; + try { + entries = await fs.promises.readdir(dir, { withFileTypes: true }); + } catch (err) { + if (isEnoent(err)) { + return; + } + throw err; + } + + for (const entry of entries) { + const fullPath = path.join(dir, entry.name); + if (entry.isDirectory()) { + await walkFilesRecursive(fullPath, baseDir, out); + } else if (entry.isFile() || entry.isSymbolicLink()) { + // A symlink (isSymbolicLink() true, isFile()/isDirectory() both false - Dirent's + // type reflects the link entry itself, not whatever it points at) is recorded via + // lstat, not followed. Deliberately not resolved into a directory recursion (a + // cyclic symlink would recurse forever) and not dereferenced to the *target* + // file's own stat (would misattribute a change at some unrelated location as a + // change *in this folder*, and could reach outside it entirely). Recording it as + // an opaque entry - its own link metadata, not the target's - is enough for this + // signature's purpose: notice that *something* here changed, not describe exactly + // what. lstat (not stat) is used uniformly for the plain-file case too - the two + // are identical for an actual file (they only differ when the entry itself is a + // symlink), so one call site covers both without a branch. Caught in code review: + // an earlier version used isFile() only, silently skipping - neither recording nor + // recursing into - any symlink entirely, a real gap in the one signal this module + // has that's supposed to observe the real folder contents unconditionally. + const stat = await fs.promises.lstat(fullPath); + out.push(`${path.relative(baseDir, fullPath)}:${stat.size}:${Math.floor(stat.mtimeMs)}`); + } + } +} + +/** + * Builds a sorted, newline-joined listing signature for every file recursively under + * `folderPath`. Uses size + mtime, not a content hash - cheap (no file reads) and, per + * this module's own header comment, both hazards this unit targets replace file content + * wholesale (`mergeBackup.ts`'s `moveFiles` deletes-then-copies every file; + * `importScriptMerges`'s `handleMergedScripts` does the same), so a genuine hazard always + * changes both fields for every affected file. Accepted, documented tradeoff: an + * incidental mtime-only touch with unchanged content and size (e.g. some external tool + * touching the file without editing it) could produce a false-positive "drift" signal - + * not dangerous (worst case, one extra distinctly-worded notification pointing the user + * at the merge-history dashlet), just possible noise. Reimplementing WSM's own xxHash32 + * (`Tools/Hasher.cs`) in TypeScript to compare byte-for-byte instead would risk a subtle + * mismatch with the .NET implementation and duplicate load-bearing hashing logic outside + * this repo's own source of truth for it - not worth it for a best-effort secondary + * signal. + */ +export async function buildFolderListingSignature(folderPath: string): Promise { + const entries: string[] = []; + await walkFilesRecursive(folderPath, folderPath, entries); + return entries.sort().join('\n'); +} + +/** + * Combines both signals (see this module's own header doc comment) into one comparable + * snapshot. Takes an already-connected client rather than connecting its own - the two + * production call sites (`refreshCoexistenceState` below, and `resolveAction.ts`'s + * `runMergeConflictsWorkflow`) each already have one open for their own purposes, and + * amortizing this module's two extra tool calls (`get_status`, `list_merges`) onto an + * existing connection is free; a caller with no client open yet should use + * `refreshCoexistenceState`, which owns its own short-lived connect/close cycle per + * `mcpClient.ts`'s documented lifecycle policy. + */ +export async function computeMergeStateSnapshot(client: MergeStateClient): Promise { + // Run concurrently, not sequentially - get_status and list_merges are independent + // reads with no data dependency between them (confirmed against WsmMcpTools.cs: each + // re-scans/re-loads its own state fresh, with nothing shared or cached server-side + // between calls), and WsmMcpClient's own JSON-RPC id-based request/response matching + // already supports arbitrary concurrent in-flight requests correctly - this halves + // this function's own latency contribution on the did-deploy path (itself inside + // Vortex's awaited did-deploy window) for no correctness cost. Caught in code review - + // an earlier version awaited these one after the other for no reason. + const [status, merges] = await Promise.all([client.getStatus(), client.listMerges()]); + + const folderListingSignature = await buildFolderListingSignature(path.join(status.modsDirectory, status.mergedModName)); + + return { + folderListingSignature, + mergeHistorySignature: computeMergeHistorySignature(merges), + mergedModName: status.mergedModName, + }; +} + +function snapshotsEqual(a: MergeStateSnapshot, b: MergeStateSnapshot): boolean { + return a.folderListingSignature === b.folderListingSignature && a.mergeHistorySignature === b.mergeHistorySignature; +} + +/** Module-level "last known" baseline - in-memory only, scoped to this extension's own + * process lifetime, matching `conflictNotifications.ts`'s own `lastNotifiedSignature` + * precedent (and its "no persisted cross-session state needed" rationale: a fresh + * Vortex session has no baseline to compare the first observation against, which is + * exactly the desired behavior - see `checkCoexistenceDrift` below). `undefined` (not a + * neutral empty-signature sentinel like that module uses) specifically because "no + * baseline yet" and "baseline is a snapshot of empty state" are genuinely different + * conditions here: the former must never warn (nothing to compare against), the latter + * must compare normally like any other snapshot. */ +let lastKnownSnapshot: MergeStateSnapshot | undefined; + +/** Test-only reset hook, mirroring `conflictNotifications.ts`'s + * `resetConflictNotificationState` - no production caller should ever need this. */ +export function resetCoexistenceGuardState(): void { + lastKnownSnapshot = undefined; +} + +/** + * Records `snapshot` as the new baseline without comparing or notifying - the + * "re-baseline after our own writes" half of this module's own header doc comment. + * `resolveAction.ts`'s `runMergeConflictsWorkflow` is the only production caller, for + * both the dry-run preview and the real merge (harmless either way: a dry run doesn't + * write anything, so re-recording it is an idempotent no-op against whatever the + * baseline already was). + */ +export function recordOwnMergeStateSnapshot(snapshot: MergeStateSnapshot): void { + lastKnownSnapshot = snapshot; +} + +/** + * Compares `snapshot` against the last known baseline and warns distinctly (never + * throws - safe to call from any of `index.ts`'s event handlers, matching every other + * module's "must never escape an event handler" convention) when it's genuinely + * different from a *previously observed* baseline. The very first observation in a + * session only seeds the baseline - there is nothing to have "drifted" from yet, and + * treating a fresh session's first snapshot as drift would falsely accuse Vortex's own + * extension of interference that may have happened in a previous session (or never at + * all). + * + * On a genuine change: resets `conflictNotifications.ts`'s own suppression state + * (`resetConflictNotificationState`) - a stale `alreadyResolved`-based conflict signature + * is no longer trustworthy once WSM's own merge-state has changed underneath it (a merge + * record `scan_conflicts` treated as "already resolved" may no longer reflect what's + * actually in the merged-mod folder), so the next `did-deploy` should get a fair chance + * to notify against reality rather than silently matching a now-stale "already seen" + * signature. Also proactively dismisses the *ordinary* conflicts notification + * (`WSM_CONFLICTS_NOTIFICATION_ID`) if one happens to be showing - it was computed + * against merge state that's now known to be stale in *either* direction (conflicts may + * have been silently resolved, in which case leaving a "N unresolved conflicts" warning + * on screen is simply wrong; or the real count may now be different, in which case the + * old number is inaccurate) - and this new, distinctly-worded notification below already + * tells the user something needs a fresh look, so nothing is lost by clearing the old + * one rather than leaving a now-unverifiable number next to it. Safe even when nothing + * was actually showing (`dismissNotification` no-ops on an unknown id, per + * `conflictNotifications.ts`'s own established assumption). Without this dismissal, the + * *particular* case of an external change that fully resolved every conflict would never + * get cleared at all: `resetConflictNotificationState` alone sets + * `lastNotifiedSignature` back to `''`, which is the exact same value + * `computeConflictSignature([])` produces for "nothing unresolved" - so the very next + * did-deploy scan finding zero conflicts would see `signature === lastNotifiedSignature` + * and return early *before* ever reaching the `dismissNotification` branch. + * + * Then sends a notification with an id/wording distinct from `conflictNotifications.ts`'s + * own `WSM_CONFLICTS_NOTIFICATION_ID` - this is "something changed your WSM merge state + * outside this extension", not "you have new conflicts", per this unit's own task + * description. `allowSuppress` is deliberately omitted/`false` here (unlike the ordinary + * conflicts notification's `allowSuppress: true`) - permanently suppressing "your merge + * output may have been overwritten" is a worse default than for routine conflict nagging. + * + * **`lastKnownSnapshot` is only advanced to `snapshot` after a successful notification + * (or immediately, for the "nothing changed"/first-observation cases, where there's + * nothing to fail)** - deliberately not committed unconditionally up front. Mirrors + * `conflictNotifications.ts`'s own `notifyConflictsIfChanged`, which documents the exact + * same reasoning: if the baseline were advanced before `sendNotification` completes and + * that call then throws, the user would never have actually seen the notification, yet + * every later checkpoint would silently treat this exact drift as "already reported" for + * the rest of the session - a failed attempt must be retried at the next checkpoint, not + * recorded as handled. + */ +export function checkCoexistenceDrift(api: types.IExtensionApi, snapshot: MergeStateSnapshot): void { + const previous = lastKnownSnapshot; + + if (previous === undefined || snapshotsEqual(previous, snapshot)) { + lastKnownSnapshot = snapshot; + return; + } + + resetConflictNotificationState(); + try { + api.dismissNotification?.(WSM_CONFLICTS_NOTIFICATION_ID); + } catch (err) { + // Best-effort only - a failure here must not prevent the coexistence notification + // itself (the more important of the two) from still being attempted below. + log('warn', 'witcherscriptmerger-vortex: failed to dismiss the stale conflicts notification during a coexistence-drift check', { + error: err instanceof Error ? err.message : String(err), + }); + } + + try { + api.sendNotification?.({ + id: WSM_COEXISTENCE_NOTIFICATION_ID, + type: 'warning', + title: 'WitcherScriptMerger merge state changed outside this extension', + message: + 'Something other than this extension\'s own "Resolve Script Conflicts" action changed your WitcherScriptMerger ' + + 'merge results - most likely Vortex\'s own built-in Witcher 3 Script Merger support (installing a Collection ' + + 'that bundles script merges, or a per-profile merge restore on switching profiles). Check the ' + + '"WitcherScriptMerger History" dashlet and re-run "Resolve Script Conflicts" to review current state.', + actions: [ + { + title: 'More', + action: () => { + api.showDialog?.( + 'info', + 'WitcherScriptMerger Coexistence', + { + text: + 'This companion extension detected that WitcherScriptMerger\'s recorded merges and/or merged-file ' + + 'output changed since it last checked, without going through this extension\'s own "Resolve Script ' + + 'Conflicts" action.\n\n' + + 'Vortex has a separate, built-in Witcher 3 Script Merger integration that this extension coexists ' + + 'with rather than replaces. That built-in integration can overwrite merge results when you install a ' + + 'Collection containing script merges (it shows its own warning dialog first), and can back up/restore ' + + 'merged scripts per Vortex profile if a profile has the "local_merges" profile feature enabled.\n\n' + + 'This extension cannot prevent either of those - open the "WitcherScriptMerger History" dashlet to ' + + 'see the current recorded merges, and use "Resolve Script Conflicts" (Mods page toolbar) to review ' + + 'and re-merge anything that needs it.', + }, + [{ label: 'Close', default: true }], + ); + }, + }, + ], + }); + } catch (err) { + // Deliberately does NOT advance lastKnownSnapshot below in this branch - see this + // function's own doc comment on why a failed notification attempt must be retried at + // the next checkpoint rather than recorded as "already handled." + log('warn', 'witcherscriptmerger-vortex: failed to show the coexistence-drift notification', { + error: err instanceof Error ? err.message : String(err), + }); + return; + } + + lastKnownSnapshot = snapshot; +} + +/** + * Bounds each individual `get_status`/`list_merges` request (and the `initialize` + * handshake) - matches `conflictScan.ts`'s own `POST_DEPLOY_SCAN_TIMEOUT_MS` exactly, for + * the identical reason: this function is called from `index.ts`'s `checkForConflictsAfterDeploy`, + * which runs *inside* Vortex's own `emitAndAwait('did-deploy', ...)` await window (see that + * file's own comment, and `conflictScan.ts`'s) - `mcpClient.ts`'s default + * `DEFAULT_REQUEST_TIMEOUT_MS` (30s) applied per-request would let this specific call site + * extend Vortex's own reported deployment-completion time by up to a full minute across + * the handshake + two tool calls. Applied uniformly (not only on the did-deploy path) for + * simplicity - a bounded wait is also just better UX on the `profile-did-change`/ + * `gamemode-activated` trigger points, even though those aren't part of an awaited chain. + */ +const COEXISTENCE_CHECK_TIMEOUT_MS = 15_000; + +export interface RefreshCoexistenceStateDeps { + /** Test-only seam - defaults to the real `WsmMcpClient.connect`. */ + connect?: typeof WsmMcpClient.connect; +} + +/** + * Owns its own short-lived connect/compute/close cycle (unlike `computeMergeStateSnapshot` + * above, which expects an already-open client) - the entry point every `index.ts` trigger + * point (`gamemode-activated`, `profile-did-change`, `did-deploy`) calls directly. Never + * throws: every failure (no tool acquired yet, connect failure, a tool-call error) is + * logged and swallowed, exactly like `fetchMergeHistory`/`scanWsmConflicts`'s own + * "must never break the caller's own primary flow" discipline - this is a secondary, + * best-effort signal, and `index.ts`'s `checkForConflictsAfterDeploy` in particular must + * still reach its own `scanWsmConflicts`/`notifyConflictsIfChanged` call afterward even if + * this fails. + * + * Deliberately a **separate** WSM process spawn from `conflictScan.ts`'s own + * `scanWsmConflicts` on the `did-deploy` path, not amortized onto the same client/ + * connection - a conscious tradeoff, not an oversight. Amortizing would mean either + * widening `scanWsmConflicts`'s own return shape (an established, separately-tested + * function with its own overlapping-call coalescing via `inFlightScan`) to also carry + * `get_status`/`list_merges` results, or bypassing that coalescing with a second, + * independent connect anyway - both add real coupling between this unit and Unit G's + * conflict-scanning module for a feature that only needs to catch a *rare* event + * (someone else's extension interfering with merge state), not a per-deployment hot + * path. The extra process costs a bounded, sub-second-to-low-seconds WSM startup+ + * `get_status`+`list_merges` round trip once per Witcher3 deployment - acceptable given + * deployments are user/install-triggered, not a polling loop. + */ +export async function refreshCoexistenceState(api: types.IExtensionApi, deps: RefreshCoexistenceStateDeps = {}): Promise { + try { + if (!(await isWsmToolAcquired(api))) { + log('debug', 'witcherscriptmerger-vortex: no acquired WSM tool - skipping coexistence-state check'); + return; + } + + const connect = deps.connect ?? WsmMcpClient.connect; + const gameDirectory = selectors.discoveryByGame(api.getState(), WITCHER3_GAME_ID)?.path; + const env = mergeWithProcessEnv(buildWsmEnv({ gameDirectory })); + + let client: WsmMcpClient | undefined; + try { + client = await connect({ exePath: getWsmExePath(api), env, requestTimeoutMs: COEXISTENCE_CHECK_TIMEOUT_MS }); + const snapshot = await computeMergeStateSnapshot(client); + checkCoexistenceDrift(api, snapshot); + } finally { + if (client) { + await client.close(); + } + } + } catch (err) { + log('warn', 'witcherscriptmerger-vortex: coexistence-state check failed', { + error: err instanceof Error ? err.message : String(err), + }); + } +} diff --git a/vortex-extension/src/conflictNotifications.ts b/vortex-extension/src/conflictNotifications.ts index 79d0164..5ca38ef 100644 --- a/vortex-extension/src/conflictNotifications.ts +++ b/vortex-extension/src/conflictNotifications.ts @@ -142,8 +142,16 @@ export function isModOrDependencyInstallActive(api: types.IExtensionApi): boolea * session with zero conflicts matches the "nothing changed" early-return below and * never calls `dismissNotification` for an id that was never sent (harmless against * the real Vortex API, which no-ops on an unknown id, but pointless noise otherwise). - * Exported reset hook is for test isolation only - no production caller should ever - * need it. */ + * + * **Exported reset hook now has a real production caller, not just test isolation** + * (correcting this comment's own earlier claim otherwise): `coexistenceGuard.ts`'s + * `checkCoexistenceDrift` calls this when it detects WSM's merge state changed outside + * this extension (Unit K) - a stale `alreadyResolved`-based signature is no longer + * trustworthy once the underlying merge state it was computed against has moved, so the + * next `did-deploy` needs a fair chance to re-notify rather than matching a signature + * that predates the external change. Test suites still call it for isolation between + * cases too (see this file's own tests' `beforeEach`), which is the *original* reason + * this was exported - just no longer the only one. */ let lastNotifiedSignature = ''; export function resetConflictNotificationState(): void { diff --git a/vortex-extension/src/index.test.ts b/vortex-extension/src/index.test.ts index 9fb4ed2..e5e3548 100644 --- a/vortex-extension/src/index.test.ts +++ b/vortex-extension/src/index.test.ts @@ -1,4 +1,4 @@ -import { describe, expect, it, vi } from 'vitest'; +import { beforeEach, describe, expect, it, vi } from 'vitest'; // `vi.mock` factories are hoisted above imports, so anything they reference has to come // from `vi.hoisted` rather than an ordinary outer-scope `const` - this isolates index.ts's @@ -12,6 +12,7 @@ const { scanWsmConflictsMock, notifyConflictsIfChangedMock, isModOrDependencyInstallActiveMock, + refreshCoexistenceStateMock, } = vi.hoisted(() => ({ ensureWsmToolRegisteredMock: vi.fn(), registerWsmStatusDashletMock: vi.fn(), @@ -19,6 +20,7 @@ const { scanWsmConflictsMock: vi.fn(), notifyConflictsIfChangedMock: vi.fn(), isModOrDependencyInstallActiveMock: vi.fn(), + refreshCoexistenceStateMock: vi.fn(), })); vi.mock('./toolAcquisition', () => ({ @@ -42,6 +44,17 @@ vi.mock('./conflictNotifications', () => ({ isModOrDependencyInstallActive: isModOrDependencyInstallActiveMock, })); +// Unit K: isolates index.ts's own wiring from coexistenceGuard.ts's real behavior (its +// own snapshot/compare/notify logic is covered directly by coexistenceGuard.test.ts +// instead) - matches every mock above's own rationale. Without this, refreshCoexistenceState +// would run for real against the *mocked* './conflictScan' module above (which doesn't +// export getWsmExePath), silently failing inside its own try/catch on every did-deploy +// test where isWsmToolAcquiredMock resolves true - technically harmless (it never throws +// out to the caller) but untested and liable to mask a real regression. +vi.mock('./coexistenceGuard', () => ({ + refreshCoexistenceState: refreshCoexistenceStateMock, +})); + import main from './index'; import { WITCHER3_GAME_ID } from './gating'; @@ -68,7 +81,11 @@ import { WITCHER3_GAME_ID } from './gating'; function fakeContext(initialActiveGameId: string | undefined, profiles: Record = {}) { const state = { activeGameId: initialActiveGameId, profiles }; let onceCallback: (() => void) | undefined; - const eventListeners = new Map void>>(); + // Args-capable (not just `() => void`) since Unit K's 'profile-did-change' listener + // takes a `profileId: string` argument - `did-deploy`'s own async equivalent already + // needed args support (see `fireAsyncEvent` below), this just extends the same + // capability to plain `events.on` listeners. + const eventListeners = new Map void>>(); const asyncListeners = new Map Promise>(); const registerActionMock = vi.fn(); @@ -84,7 +101,7 @@ function fakeContext(initialActiveGameId: string | undefined, profiles: Record state, events: { - on: (eventName: string, listener: () => void) => { + on: (eventName: string, listener: (...args: unknown[]) => void) => { const listeners = eventListeners.get(eventName) ?? []; listeners.push(listener); eventListeners.set(eventName, listeners); @@ -99,7 +116,7 @@ function fakeContext(initialActiveGameId: string | undefined, profiles: Record[0], fireOnce: () => onceCallback?.(), - fireEvent: (eventName: string) => eventListeners.get(eventName)?.forEach((listener) => listener()), + fireEvent: (eventName: string, ...args: unknown[]) => eventListeners.get(eventName)?.forEach((listener) => listener(...args)), fireAsyncEvent: (eventName: string, ...args: unknown[]) => asyncListeners.get(eventName)?.(...args), setActiveGame: (gameId: string | undefined) => { state.activeGameId = gameId; @@ -109,6 +126,23 @@ function fakeContext(initialActiveGameId: string | undefined, profiles: Record { + // Unit K: refreshCoexistenceState is now called unconditionally, from THREE different + // call sites, whenever witcher3 is the relevant game (tryRegisterWsmTool - both at + // fireOnce() time and on every 'gamemode-activated' - plus checkForConflictsAfterDeploy + // and checkCoexistenceOnProfileChange). A safe resolved-value default here (rather than + // requiring every single test that happens to exercise a witcher3-active path to + // remember to configure it) avoids a whole class of "Cannot read properties of + // undefined (reading 'catch')" crashes an unconfigured vi.fn() would otherwise cause the + // moment this file's own .catch(...) call sites run against it - caught in code review + // when the gamemode-activated wiring was added and several previously-passing tests + // started crashing. Individual tests below still override this via their own + // .mockClear()/.mockReset() calls where they need to assert something more specific + // (an exact call count, a rejection, etc.) - this is only the baseline every other test + // can rely on without thinking about it. + beforeEach(() => { + refreshCoexistenceStateMock.mockReset().mockResolvedValue(undefined); + }); + it('returns true (Vortex extension init contract)', () => { const { context } = fakeContext(undefined); expect(main(context)).toBe(true); @@ -151,6 +185,37 @@ describe('main (index.ts)', () => { expect(ensureWsmToolRegisteredMock).toHaveBeenCalledTimes(1); }); + // Unit K: this test exists specifically because a prior version of this file's own + // header comment (and coexistenceGuard.ts's own) documented 'gamemode-activated' as a + // wired coexistence-check trigger point without tryRegisterWsmTool actually calling + // refreshCoexistenceState at all - caught in code review, not by any test, since + // nothing had asserted on this call site before. This test is what would have caught + // that regression, and is what should catch it again if this wiring is ever dropped. + it('calls refreshCoexistenceState (the third trigger point) whenever tryRegisterWsmTool runs with witcher3 active - both at context.once time and on a live gamemode-activated switch', () => { + const { context, fireOnce, fireEvent, setActiveGame } = fakeContext(WITCHER3_GAME_ID); + + main(context); + fireOnce(); + expect(refreshCoexistenceStateMock).toHaveBeenCalledTimes(1); + expect(refreshCoexistenceStateMock).toHaveBeenCalledWith(context.api); + + setActiveGame('skyrimse'); + fireEvent('gamemode-activated'); + expect(refreshCoexistenceStateMock).toHaveBeenCalledTimes(1); // still not witcher3 - no new call + + setActiveGame(WITCHER3_GAME_ID); + fireEvent('gamemode-activated'); + expect(refreshCoexistenceStateMock).toHaveBeenCalledTimes(2); + }); + + it('does not throw when refreshCoexistenceState rejects from tryRegisterWsmTool', () => { + refreshCoexistenceStateMock.mockReset().mockRejectedValue(new Error('coexistence check failed')); + const { context, fireOnce } = fakeContext(WITCHER3_GAME_ID); + + main(context); + expect(() => fireOnce()).not.toThrow(); + }); + it('does nothing on "gamemode-activated" when the newly-active game still is not witcher3', () => { ensureWsmToolRegisteredMock.mockClear().mockResolvedValue(false); const { context, fireOnce, fireEvent, setActiveGame } = fakeContext('skyrimse'); @@ -278,6 +343,7 @@ describe('main (index.ts)', () => { ensureWsmToolRegisteredMock.mockClear().mockResolvedValue(false); isWsmToolAcquiredMock.mockClear().mockResolvedValue(true); isModOrDependencyInstallActiveMock.mockClear().mockReturnValue(false); + refreshCoexistenceStateMock.mockClear().mockResolvedValue(undefined); const conflicts = [{ relativePath: 'a.ws' }]; scanWsmConflictsMock.mockClear().mockResolvedValue(conflicts); notifyConflictsIfChangedMock.mockClear(); @@ -297,6 +363,7 @@ describe('main (index.ts)', () => { it('skips scanning (without throwing) when no WSM tool has been acquired yet', async () => { ensureWsmToolRegisteredMock.mockClear().mockResolvedValue(false); isWsmToolAcquiredMock.mockClear().mockResolvedValue(false); + refreshCoexistenceStateMock.mockClear(); scanWsmConflictsMock.mockClear(); notifyConflictsIfChangedMock.mockClear(); const { context, fireOnce, fireAsyncEvent } = fakeContext(WITCHER3_GAME_ID, { @@ -316,6 +383,7 @@ describe('main (index.ts)', () => { ensureWsmToolRegisteredMock.mockClear().mockResolvedValue(false); isWsmToolAcquiredMock.mockClear().mockResolvedValue(true); isModOrDependencyInstallActiveMock.mockClear().mockReturnValue(false); + refreshCoexistenceStateMock.mockClear().mockResolvedValue(undefined); const conflicts = [{ relativePath: 'a.ws' }]; scanWsmConflictsMock.mockClear().mockResolvedValue(conflicts); notifyConflictsIfChangedMock.mockClear(); @@ -354,10 +422,123 @@ describe('main (index.ts)', () => { expect(notifyConflictsIfChangedMock).not.toHaveBeenCalled(); }); + // Unit K's own headline regression: refreshCoexistenceState must run even while a + // mod/dependency install is in progress (e.g. installing a Collection), because + // that's precisely the window in which game-witcher3's own importScriptMerges() can + // overwrite this extension's merge output (coexistenceGuard.ts's own doc comment, + // hazard 1). A coexistence check gated behind the same isModOrDependencyInstallActive + // early-return that (correctly) skips *conflict scanning* here would never see the + // one did-deploy where that overwrite actually happened. + it('still calls refreshCoexistenceState even while a mod/dependency install is in progress', async () => { + ensureWsmToolRegisteredMock.mockClear().mockResolvedValue(false); + isWsmToolAcquiredMock.mockClear().mockResolvedValue(true); + isModOrDependencyInstallActiveMock.mockClear().mockReturnValue(true); + scanWsmConflictsMock.mockClear(); + const { context, fireOnce, fireAsyncEvent } = fakeContext(WITCHER3_GAME_ID, { + profile1: { gameId: WITCHER3_GAME_ID }, + }); + + main(context); + fireOnce(); + // fireOnce() itself already triggered one refreshCoexistenceState call via + // tryRegisterWsmTool (witcher3 is this test's own initial active game) - cleared + // here so the assertion below isolates did-deploy's own, separate call to it, + // which is what this test actually exists to prove. + refreshCoexistenceStateMock.mockClear(); + await fireAsyncEvent('did-deploy', 'profile1', undefined); + + expect(refreshCoexistenceStateMock).toHaveBeenCalledTimes(1); + expect(refreshCoexistenceStateMock).toHaveBeenCalledWith(context.api); + // Still correctly skips the unrelated conflict scan itself. + expect(scanWsmConflictsMock).not.toHaveBeenCalled(); + }); + + it('calls refreshCoexistenceState for an ordinary witcher3 deployment too', async () => { + ensureWsmToolRegisteredMock.mockClear().mockResolvedValue(false); + isWsmToolAcquiredMock.mockClear().mockResolvedValue(true); + isModOrDependencyInstallActiveMock.mockClear().mockReturnValue(false); + scanWsmConflictsMock.mockClear().mockResolvedValue([]); + notifyConflictsIfChangedMock.mockClear(); + const { context, fireOnce, fireAsyncEvent } = fakeContext(WITCHER3_GAME_ID, { + profile1: { gameId: WITCHER3_GAME_ID }, + }); + + main(context); + fireOnce(); + refreshCoexistenceStateMock.mockClear(); // see comment in the test just above + await fireAsyncEvent('did-deploy', 'profile1', undefined); + + expect(refreshCoexistenceStateMock).toHaveBeenCalledTimes(1); + }); + + it('does not call refreshCoexistenceState when no WSM tool has been acquired yet', async () => { + ensureWsmToolRegisteredMock.mockClear().mockResolvedValue(false); + isWsmToolAcquiredMock.mockClear().mockResolvedValue(false); + const { context, fireOnce, fireAsyncEvent } = fakeContext(WITCHER3_GAME_ID, { + profile1: { gameId: WITCHER3_GAME_ID }, + }); + + main(context); + fireOnce(); + refreshCoexistenceStateMock.mockClear(); // see comment two tests above + await fireAsyncEvent('did-deploy', 'profile1', undefined); + + expect(refreshCoexistenceStateMock).not.toHaveBeenCalled(); + }); + + it('does not call refreshCoexistenceState for a non-witcher3 deployment', async () => { + ensureWsmToolRegisteredMock.mockClear().mockResolvedValue(false); + isWsmToolAcquiredMock.mockClear(); + const { context, fireOnce, fireAsyncEvent } = fakeContext(WITCHER3_GAME_ID, { + profile1: { gameId: 'skyrimse' }, + }); + + main(context); + fireOnce(); + refreshCoexistenceStateMock.mockClear(); // see comment three tests above + await fireAsyncEvent('did-deploy', 'profile1', undefined); + + expect(refreshCoexistenceStateMock).not.toHaveBeenCalled(); + }); + + // Corrected in code review: an earlier version of this test's own title claimed the + // conflict scan still runs when refreshCoexistenceState rejects, while its own + // neighboring comment said the opposite - and neither was actually verified by an + // assertion, so the mismatch went unnoticed. The real, correct behavior (per + // checkForConflictsAfterDeploy's single shared try/catch wrapping the whole handler + // body) is that a refreshCoexistenceState rejection aborts the rest of *this specific* + // handler invocation, matching the existing, established precedent for every other + // unexpected throw in this same handler (see the isWsmToolAcquired-throws-EBUSY test + // below) - not a special case for this one call. refreshCoexistenceState is + // documented (coexistenceGuard.ts's own doc comment) to never actually reject in + // production; this test exists to prove the handler stays robust (never rejects + // onAsync's own promise) even if that contract were ever violated, not to claim the + // scan proceeds regardless. + it('resolves (never rejects) when refreshCoexistenceState itself rejects, and correctly skips the rest of this handler invocation (matching every other unexpected-throw case in this same handler)', async () => { + ensureWsmToolRegisteredMock.mockClear().mockResolvedValue(false); + isWsmToolAcquiredMock.mockClear().mockResolvedValue(true); + isModOrDependencyInstallActiveMock.mockClear().mockReturnValue(false); + refreshCoexistenceStateMock.mockClear().mockRejectedValue(new Error('coexistence check failed')); + const conflicts = [{ relativePath: 'a.ws' }]; + scanWsmConflictsMock.mockClear().mockResolvedValue(conflicts); + notifyConflictsIfChangedMock.mockClear(); + const { context, fireOnce, fireAsyncEvent } = fakeContext(WITCHER3_GAME_ID, { + profile1: { gameId: WITCHER3_GAME_ID }, + }); + + main(context); + fireOnce(); + + await expect(fireAsyncEvent('did-deploy', 'profile1', undefined)).resolves.toBeUndefined(); + expect(scanWsmConflictsMock).not.toHaveBeenCalled(); + expect(notifyConflictsIfChangedMock).not.toHaveBeenCalled(); + }); + it('resolves (never rejects) when scanWsmConflicts throws - onAsync listeners must report their own errors', async () => { ensureWsmToolRegisteredMock.mockClear().mockResolvedValue(false); isWsmToolAcquiredMock.mockClear().mockResolvedValue(true); isModOrDependencyInstallActiveMock.mockClear().mockReturnValue(false); + refreshCoexistenceStateMock.mockClear().mockResolvedValue(undefined); scanWsmConflictsMock.mockClear().mockRejectedValue(new Error('spawn failed')); notifyConflictsIfChangedMock.mockClear(); const { context, fireOnce, fireAsyncEvent } = fakeContext(WITCHER3_GAME_ID, { @@ -425,4 +606,92 @@ describe('main (index.ts)', () => { expect(scanWsmConflictsMock).not.toHaveBeenCalled(); }); }); + + // Unit K: the second coexistence-check trigger point, alongside did-deploy above - + // see coexistenceGuard.ts's own header comment for why profile-will-change itself is + // deliberately not used and profile-did-change is used instead. + describe('profile-did-change coexistence check', () => { + it('registers a profile-did-change handler via events.on (not onAsync) at context.once time', () => { + const { context, fireOnce, fireEvent } = fakeContext(undefined); + + main(context); + fireOnce(); + + // Must not throw - proves a listener really is registered for this event name, + // not silently falling through to a no-op (fireEvent no-ops on an unknown event + // name too, so this alone wouldn't distinguish "registered" from "not registered" + // without the refreshCoexistenceStateMock assertions in the tests below). + expect(() => fireEvent('profile-did-change', 'profile1')).not.toThrow(); + }); + + it('calls refreshCoexistenceState when the switched-to profile is witcher3', () => { + refreshCoexistenceStateMock.mockClear().mockResolvedValue(undefined); + const { context, fireOnce, fireEvent } = fakeContext(undefined, { + profile1: { gameId: WITCHER3_GAME_ID }, + }); + + main(context); + fireOnce(); + fireEvent('profile-did-change', 'profile1'); + + expect(refreshCoexistenceStateMock).toHaveBeenCalledTimes(1); + expect(refreshCoexistenceStateMock).toHaveBeenCalledWith(context.api); + }); + + it('does not call refreshCoexistenceState when the switched-to profile is a different game', () => { + refreshCoexistenceStateMock.mockClear(); + const { context, fireOnce, fireEvent } = fakeContext(undefined, { + profile1: { gameId: 'skyrimse' }, + }); + + main(context); + fireOnce(); + fireEvent('profile-did-change', 'profile1'); + + expect(refreshCoexistenceStateMock).not.toHaveBeenCalled(); + }); + + it('does not call refreshCoexistenceState when the profileId is unknown (no matching profile at all)', () => { + refreshCoexistenceStateMock.mockClear(); + const { context, fireOnce, fireEvent } = fakeContext(undefined, {}); + + main(context); + fireOnce(); + fireEvent('profile-did-change', 'unknown-profile'); + + expect(refreshCoexistenceStateMock).not.toHaveBeenCalled(); + }); + + it('does not throw (a plain events.on listener rejecting is an unhandled rejection in Vortex\'s own process) when refreshCoexistenceState rejects', async () => { + refreshCoexistenceStateMock.mockClear().mockRejectedValue(new Error('coexistence check failed')); + const { context, fireOnce, fireEvent } = fakeContext(undefined, { + profile1: { gameId: WITCHER3_GAME_ID }, + }); + + main(context); + fireOnce(); + + expect(() => fireEvent('profile-did-change', 'profile1')).not.toThrow(); + // Let the rejected promise's own .catch() handler actually run before the test + // ends, matching the existing "does not throw when ensureWsmToolRegistered + // rejects" test's own pattern above. + await new Promise((resolve) => setTimeout(resolve, 0)); + }); + + it('does not throw when reading state for the profile gate throws synchronously', () => { + refreshCoexistenceStateMock.mockClear(); + const { context, fireOnce, fireEvent } = fakeContext(undefined, { + profile1: { gameId: WITCHER3_GAME_ID }, + }); + + main(context); + fireOnce(); + context.api.getState = () => { + throw new Error('state store unavailable'); + }; + + expect(() => fireEvent('profile-did-change', 'profile1')).not.toThrow(); + expect(refreshCoexistenceStateMock).not.toHaveBeenCalled(); + }); + }); }); diff --git a/vortex-extension/src/index.ts b/vortex-extension/src/index.ts index 5b0e5be..57dc794 100644 --- a/vortex-extension/src/index.ts +++ b/vortex-extension/src/index.ts @@ -1,4 +1,5 @@ import { log, selectors, types } from 'vortex-api'; +import { refreshCoexistenceState } from './coexistenceGuard'; import { isWsmToolAcquired, scanWsmConflicts } from './conflictScan'; import { isModOrDependencyInstallActive, notifyConflictsIfChanged } from './conflictNotifications'; import { isWitcher3Active, WITCHER3_GAME_ID } from './gating'; @@ -85,6 +86,18 @@ import { ensureWsmToolRegistered } from './toolAcquisition'; * This extension must never call `context.registerGame('witcher3', ...)` - Vortex's own * built-in `game-witcher3` extension already owns that registration; this extension is a * companion to it, not a replacement. + * + * Unit K (coexistence & Collections handling) adds no new `context.register*` call - see + * `coexistenceGuard.ts`'s own header doc comment for the full detect/warn/reconcile + * design and its citations. It adds two things to what's registered above: a new + * `context.api.events.on('profile-did-change', ...)` listener (plain `events.on`, not + * `onAsync` - matches `EVENTS.md`'s own "Profile has been switched" entry, which is not + * marked "Async." the way `did-deploy` is), registered inside `context.once` alongside + * `tryRegisterWsmTool`'s own `gamemode-activated` listener; and a call to + * `refreshCoexistenceState` from inside `checkForConflictsAfterDeploy`, deliberately + * placed *above* that function's own `isModOrDependencyInstallActive` early-return - see + * that call site's own comment for why installing a Collection is exactly the case this + * ordering exists to still catch. */ function main(context: types.IExtensionContext): boolean { // Unit J: the dependency/status dashlet. Called here, synchronously and @@ -120,6 +133,21 @@ function main(context: types.IExtensionContext): boolean { error: err instanceof Error ? err.message : String(err), }); }); + + // Unit K: the third coexistence-check trigger point (alongside profile-did-change and + // did-deploy, both further below) - catches drift that accumulated while a different + // game was active, or while Vortex itself was closed, neither of which either of the + // other two triggers would ever see. Fixed in code review: an earlier version of this + // file's own header comment (and coexistenceGuard.ts's own) documented + // 'gamemode-activated' as a wired trigger point without this call actually existing - + // a real doc/code mismatch, not just stale prose. refreshCoexistenceState never throws + // (its own internal try/catch) - this .catch is belt-and-suspenders only, same + // reasoning as the .catch immediately above. + refreshCoexistenceState(context.api).catch((err: unknown) => { + log('warn', 'witcherscriptmerger-vortex: gamemode-activated coexistence check failed', { + error: err instanceof Error ? err.message : String(err), + }); + }); } // onAsync's own contract (@nexusmods/vortex-api's lib/api.d.ts doc comment on @@ -172,6 +200,18 @@ function main(context: types.IExtensionContext): boolean { return; } + // Unit K (coexistence & Collections handling): deliberately called *before* the + // isModOrDependencyInstallActive gate just below, not after it. That gate exists to + // skip *conflict scanning* during a mod/dependency-install burst (see its own doc + // comment) - but installing a Vortex Collection that bundles script merges is + // exactly the window in which game-witcher3's own importScriptMerges() can silently + // overwrite this extension's merge output (coexistenceGuard.ts's own header comment, + // hazard 1). A coexistence check gated behind the same early-return would never see + // the one did-deploy where that overwrite actually happened. refreshCoexistenceState + // never throws (own try/catch) and is a purely best-effort secondary signal, so it + // can't affect what follows either way. + await refreshCoexistenceState(context.api); + if (isModOrDependencyInstallActive(context.api)) { // Purely an optimization, not a correctness requirement - notifyConflictsIfChanged // (conflictNotifications.ts) checks this same condition again on whatever result @@ -195,6 +235,42 @@ function main(context: types.IExtensionContext): boolean { } } + // Unit K (coexistence & Collections handling): the `profile-did-change` trigger point - + // see coexistenceGuard.ts's own header comment for why `profile-will-change` itself is + // deliberately not used instead (a plain, synchronous events.on emit whose listeners + // Vortex never awaits, so there's no reliable way to bracket the built-in extension's + // own file moves around it). Gated like checkForConflictsAfterDeploy gates did-deploy: + // resolves the *switched-to* profile's own gameId via selectors.profileById rather than + // isWitcher3Active(context.api) - by the time profile-did-change fires the active game + // already is the new profile's game, so the two would normally agree, but resolving it + // from the event's own profileId keeps this consistent with that existing reasoning + // rather than assuming it. + function checkCoexistenceOnProfileChange(profileId: string): void { + let gameId: string | undefined; + try { + gameId = selectors.profileById(context.api.getState(), profileId)?.gameId; + } catch (err) { + log('warn', 'witcherscriptmerger-vortex: profile-did-change coexistence check failed to read state', { + error: err instanceof Error ? err.message : String(err), + }); + return; + } + + if (gameId !== WITCHER3_GAME_ID) { + return; + } + + // refreshCoexistenceState never throws (its own internal try/catch) - this .catch is + // belt-and-suspenders only, matching tryRegisterWsmTool's own reasoning just above: + // a plain events.on listener that returns a rejected promise becomes an unhandled + // rejection in Vortex's own process rather than a contained extension failure. + refreshCoexistenceState(context.api).catch((err: unknown) => { + log('warn', 'witcherscriptmerger-vortex: profile-did-change coexistence check failed', { + error: err instanceof Error ? err.message : String(err), + }); + }); + } + // Registered synchronously here, not inside context.once below - see // mergeHistoryDashlet.ts's own registerMergeHistoryDashlet doc comment for why a // register call specifically must not be deferred into once, unlike @@ -204,6 +280,7 @@ function main(context: types.IExtensionContext): boolean { context.once(() => { tryRegisterWsmTool(); context.api.events.on('gamemode-activated', tryRegisterWsmTool); + context.api.events.on('profile-did-change', checkCoexistenceOnProfileChange); context.api.onAsync('did-deploy', checkForConflictsAfterDeploy); }); diff --git a/vortex-extension/src/resolveAction.test.ts b/vortex-extension/src/resolveAction.test.ts index e0358a4..f796cf6 100644 --- a/vortex-extension/src/resolveAction.test.ts +++ b/vortex-extension/src/resolveAction.test.ts @@ -1,9 +1,29 @@ -import { describe, expect, it, vi } from 'vitest'; +import { beforeEach, describe, expect, it, vi } from 'vitest'; import { WITCHER3_GAME_ID } from './gating'; import { WSM_TOOL_ID } from './discoveredTool'; -import { MergeConflictsResult, WsmMcpClientOptions } from './mcpClient'; +import { GetStatusResult, MergeConflictsResult, WsmMcpClientOptions } from './mcpClient'; import { resolveScriptConflicts, WsmMergeClient } from './resolveAction'; +// Unit K: isolates resolveAction.ts's own logic from coexistenceGuard.ts's real +// behavior (its own snapshot/compare logic is covered directly by +// coexistenceGuard.test.ts instead) - same rationale as every other sibling-module mock +// in this codebase (e.g. index.test.ts's own mocks). Without this, the real +// computeMergeStateSnapshot would call the fake client's own getStatus/listMerges below +// and attempt a real recursive fs walk against whatever `status.modsDirectory` those +// stubs return - harmless in practice (an ENOENT-tolerant walk against a nonsense path) +// but untested and not this file's concern. +const { computeMergeStateSnapshotMock, recordOwnMergeStateSnapshotMock, checkCoexistenceDriftMock } = vi.hoisted(() => ({ + computeMergeStateSnapshotMock: vi.fn(), + recordOwnMergeStateSnapshotMock: vi.fn(), + checkCoexistenceDriftMock: vi.fn(), +})); + +vi.mock('./coexistenceGuard', () => ({ + computeMergeStateSnapshot: computeMergeStateSnapshotMock, + recordOwnMergeStateSnapshot: recordOwnMergeStateSnapshotMock, + checkCoexistenceDrift: checkCoexistenceDriftMock, +})); + function mergeResult(overrides: Partial = {}): MergeConflictsResult { return { merged: [], @@ -73,10 +93,27 @@ function fakeApi(options: { }; } +function fakeStatus(overrides: Partial = {}): GetStatusResult { + return { + gameDirectory: 'C:\\Games\\Witcher3', + modsDirectory: 'C:\\Games\\Witcher3\\Mods', + dependenciesValid: true, + textMergeDependenciesValid: true, + bundleDependenciesValid: true, + modsDirectoryExists: true, + mergedModName: 'mod0000_MergedFiles', + conflictCount: 0, + ...overrides, + }; +} + /** A fake `connect` - returns queued results/errors in call order and records every * `exePath`/`env` it was called with, plus how many of the clients it produced were * closed - proves `resolveScriptConflicts` closes every client it opens, even on the - * error path (via `finally`). */ + * error path (via `finally`). `getStatus`/`listMerges` are trivial stubs, never + * meaningfully exercised here since `./coexistenceGuard` (the only thing that would + * call them, via `computeMergeStateSnapshot`) is mocked above - they exist only to + * satisfy `WsmMergeClient`'s type. */ function fakeConnect(outcomes: Array<{ result?: MergeConflictsResult; error?: Error }>) { const calls: WsmMcpClientOptions[] = []; let closedCount = 0; @@ -92,6 +129,8 @@ function fakeConnect(outcomes: Array<{ result?: MergeConflictsResult; error?: Er } return outcome.result!; }, + getStatus: async () => fakeStatus(), + listMerges: async () => [], close: async () => { closedCount += 1; }, @@ -102,6 +141,16 @@ function fakeConnect(outcomes: Array<{ result?: MergeConflictsResult; error?: Er } describe('resolveScriptConflicts', () => { + beforeEach(() => { + computeMergeStateSnapshotMock.mockReset().mockResolvedValue({ + folderListingSignature: '', + mergeHistorySignature: '', + mergedModName: 'mod0000_MergedFiles', + }); + recordOwnMergeStateSnapshotMock.mockReset(); + checkCoexistenceDriftMock.mockReset(); + }); + it('shows an error notification and never connects when no WSM tool has been registered', async () => { const { api, notifications, showDialogCalls } = fakeApi({}); const { connect } = fakeConnect([]); @@ -171,6 +220,65 @@ describe('resolveScriptConflicts', () => { expect(showDialogCalls[1].type).toBe('success'); }); + // Unit K: reconciles coexistenceGuard.ts's own "last known merge state" against this + // extension's own just-completed workflow, using the same still-open client each time - + // see resolveAction.ts's own comment in runMergeConflictsWorkflow for why the preview + // and the real merge deliberately go through *different* coexistenceGuard.ts functions + // (checkCoexistenceDrift for the no-write preview vs. the silent + // recordOwnMergeStateSnapshot for the real, writing merge), not the same one for both. + it('checks for coexistence drift (does not silently re-baseline) after the dry-run preview, and silently re-baselines only after the real merge', async () => { + const { api } = fakeApi({ + toolPath: 'C:\\wsm\\WitcherScriptMerger.Headless.exe', + dialogResponses: [{ action: 'Merge Now' }], + }); + const preview = mergeResult({ merged: ['a.ws'], skipped: ['b.xml'] }); + const final = mergeResult({ merged: ['a.ws'], skipped: [], dryRun: false }); + const { connect } = fakeConnect([{ result: preview }, { result: final }]); + const previewSnapshot = { folderListingSignature: 'sig1', mergeHistorySignature: 'sig2', mergedModName: 'mod0000_MergedFiles' }; + const finalSnapshot = { folderListingSignature: 'sig3', mergeHistorySignature: 'sig4', mergedModName: 'mod0000_MergedFiles' }; + computeMergeStateSnapshotMock.mockReset().mockResolvedValueOnce(previewSnapshot).mockResolvedValueOnce(finalSnapshot); + + await resolveScriptConflicts(api, { connect }); + + // Once per connected client (preview, then the real merge) - each call receives + // whichever client instance was open at that point, per computeMergeStateSnapshot's + // own "already-open client" contract. + expect(computeMergeStateSnapshotMock).toHaveBeenCalledTimes(2); + + // Preview (dryRun: true, no write performed) - compared against the existing + // baseline, never silently adopted as the new one. A regression here (an earlier + // version of this code unconditionally called recordOwnMergeStateSnapshot for both + // calls) would let a preview-then-cancel workflow silently erase evidence of real, + // undetected drift with no notification ever shown - caught in code review. + expect(checkCoexistenceDriftMock).toHaveBeenCalledTimes(1); + expect(checkCoexistenceDriftMock).toHaveBeenCalledWith(api, previewSnapshot); + expect(recordOwnMergeStateSnapshotMock).not.toHaveBeenCalledWith(previewSnapshot); + + // Real merge (dryRun: false) - this extension's own write, silently adopted as the + // new known-good baseline, no comparison/notification. + expect(recordOwnMergeStateSnapshotMock).toHaveBeenCalledTimes(1); + expect(recordOwnMergeStateSnapshotMock).toHaveBeenCalledWith(finalSnapshot); + expect(checkCoexistenceDriftMock).not.toHaveBeenCalledWith(api, finalSnapshot); + }); + + it('does not let a coexistence-guard snapshot failure affect the merge result the user sees', async () => { + const { api, showDialogCalls } = fakeApi({ + toolPath: 'C:\\wsm\\WitcherScriptMerger.Headless.exe', + dialogResponses: [{ action: 'Merge Now' }], + }); + const preview = mergeResult({ merged: ['a.ws'], skipped: [] }); + const final = mergeResult({ merged: ['a.ws'], skipped: [], dryRun: false }); + const { connect } = fakeConnect([{ result: preview }, { result: final }]); + computeMergeStateSnapshotMock.mockReset().mockRejectedValue(new Error('snapshot failed')); + + await resolveScriptConflicts(api, { connect }); + + expect(showDialogCalls).toHaveLength(2); + expect(showDialogCalls[1].type).toBe('success'); + expect(recordOwnMergeStateSnapshotMock).not.toHaveBeenCalled(); + expect(checkCoexistenceDriftMock).not.toHaveBeenCalled(); + }); + it('shows an "info" (not "success") result dialog when the real merge still leaves skipped files', async () => { const { api, showDialogCalls } = fakeApi({ toolPath: 'C:\\wsm\\WitcherScriptMerger.Headless.exe', diff --git a/vortex-extension/src/resolveAction.ts b/vortex-extension/src/resolveAction.ts index fdb2452..ceb9499 100644 --- a/vortex-extension/src/resolveAction.ts +++ b/vortex-extension/src/resolveAction.ts @@ -1,7 +1,8 @@ import { log, selectors, types } from 'vortex-api'; +import { checkCoexistenceDrift, computeMergeStateSnapshot, recordOwnMergeStateSnapshot } from './coexistenceGuard'; import { WSM_TOOL_ID } from './discoveredTool'; import { isWitcher3Active, WITCHER3_GAME_ID } from './gating'; -import { MergeConflictsArgs, MergeConflictsResult, WsmMcpClient, WsmMcpClientOptions } from './mcpClient'; +import { GetStatusResult, ListMergesResult, MergeConflictsArgs, MergeConflictsResult, WsmMcpClient, WsmMcpClientOptions } from './mcpClient'; import { buildMergeSummaryDialogContent } from './mergePanel'; import { mergeWithProcessEnv } from './wsmEnv'; @@ -44,9 +45,19 @@ const ACTIVITY_NOTIFICATION_ID = 'witcherscriptmerger-vortex-resolve-conflicts-a /** The subset of `WsmMcpClient` this file actually needs - lets unit tests inject a * fake without spawning a real WSM process (mirrors `toolAcquisition.ts`'s own - * `client`/`extractor` test seams). */ + * `client`/`extractor` test seams). + * + * `getStatus`/`listMerges` were added alongside Unit K (coexistence & Collections + * handling) - not because this file calls the underlying MCP tools directly, but + * because `runMergeConflictsWorkflow` passes the already-open client straight into + * `coexistenceGuard.ts`'s `computeMergeStateSnapshot(client: MergeStateClient)`, which + * needs both. Reusing the same already-connected client (rather than opening a third + * one just for this) is the entire point - see that function's own doc comment on why + * it takes a client instead of connecting its own. */ export interface WsmMergeClient { mergeConflicts(args?: MergeConflictsArgs): Promise; + getStatus(): Promise; + listMerges(): Promise; close(): Promise; } @@ -194,7 +205,56 @@ async function runMergeConflictsWorkflow( try { const client = await connect({ exePath, env }); try { - return await client.mergeConflicts(args); + const result = await client.mergeConflicts(args); + + // Unit K: reconciles coexistenceGuard.ts's own "last known merge state" against + // this extension's own just-completed workflow, using the same still-open client + // (no extra spawn). Deliberately non-fatal (never lets a failure here affect the + // result the user is about to see) either way, but the *two* calls below are not + // interchangeable - which one runs depends on whether this was the dry-run preview + // or the real, confirmed merge: + // + // - Real merge (`args.dryRun !== true`): `recordOwnMergeStateSnapshot` - silently + // adopts the post-merge state as the new baseline, no comparison, no notification. + // This extension itself just wrote that state, so it's known-good by definition; + // without this, this extension's own successful merge would look identical to + // external interference the next time a trigger point (did-deploy, + // profile-did-change, gamemode-activated) re-checks, since a genuine merge does + // change both signals `computeMergeStateSnapshot` compares. + // - Dry-run preview (`args.dryRun === true`): `checkCoexistenceDrift` instead - + // compares against the existing baseline and warns if it's already diverged + // *before* this workflow ever wrote anything. **Not** a silent re-baseline here: + // a dry run performs no write (`WsmMcpTools.MergeConflicts`'s own `dryRun` + // contract), so if the two differ, that's evidence of drift that happened + // *before* the user even opened this dialog - silently adopting the preview's + // read as the new "known good" baseline (as an earlier version of this code did) + // would erase that evidence permanently, including for a user who previews and + // then cancels without merging anything at all. Caught in code review: a + // preview-then-cancel workflow was silently absorbing real drift into the + // baseline with no notification ever shown, defeating this unit's entire purpose + // for exactly that sequence. + try { + const snapshot = await computeMergeStateSnapshot(client); + // `!== true`, not `=== false`: `MergeConflictsArgs.dryRun` is optional, and the + // server-side tool's own default when omitted is a *real* merge + // (`WsmMcpTools.MergeConflicts(..., bool dryRun = false)`) - so an omitted value + // must land on the "real merge" branch below, matching that server default, + // rather than being misread as a preview merely because it isn't literally + // `=== false`. Both of this file's own call sites always pass an explicit + // `dryRun: true`/`dryRun: false` today, so this only matters for a hypothetical + // future caller that omits it. + if (args.dryRun !== true) { + recordOwnMergeStateSnapshot(snapshot); + } else { + checkCoexistenceDrift(api, snapshot); + } + } catch (err) { + log('debug', 'witcherscriptmerger-vortex: failed to reconcile the coexistence-guard baseline after a resolve-script-conflicts workflow', { + error: err instanceof Error ? err.message : String(err), + }); + } + + return result; } finally { await client.close(); } diff --git a/vortex-extension/test/coexistenceGuard.integration.test.ts b/vortex-extension/test/coexistenceGuard.integration.test.ts new file mode 100644 index 0000000..7a4b9f6 --- /dev/null +++ b/vortex-extension/test/coexistenceGuard.integration.test.ts @@ -0,0 +1,203 @@ +import { afterAll, beforeAll, describe, expect, it } from 'vitest'; +import { spawnSync } from 'child_process'; +import * as fs from 'fs'; +import * as os from 'os'; +import * as path from 'path'; +import { fileURLToPath } from 'url'; +import { checkCoexistenceDrift, computeMergeStateSnapshot, resetCoexistenceGuardState, WSM_COEXISTENCE_NOTIFICATION_ID } from '../src/coexistenceGuard'; +import { WsmMcpClient } from '../src/mcpClient'; + +// Real, end-to-end integration test for coexistenceGuard.ts (Unit K): spawns the actual, +// compiled WitcherScriptMerger Headless host's `mcp` verb and drives a real +// get_status -> list_merges -> (recursive fs listing of the real merged-mod folder) +// round trip via computeMergeStateSnapshot, both before and after a genuine +// `merge_conflicts` call - the same "spawn a real process, prove the higher-level logic +// reacts correctly to its real output" pattern conflictScan.integration.test.ts already +// establishes for scanConflicts()/notifyConflictsIfChanged, applied here to +// computeMergeStateSnapshot()/checkCoexistenceDrift(). +// +// Deliberately uses only an auto-solving conflict (two mods editing the same .ws file on +// disjoint lines), not a genuinely-conflicting one - unlike +// test/mcpClient.integration.test.ts's own real-merge describe block, which stages a +// second, genuinely-conflicting .xml file specifically to prove the skipped/sidecar path. +// This test only needs the merged-mod folder and MergeInventory.xml to end up +// non-trivially populated by a real merge; a genuine conflict would additionally trigger +// FileOpener.Open on the written conflict-marker sidecar (a real, documented side effect - +// see that other file's own comment), which this test has no reason to also incur. + +const __dirname = path.dirname(fileURLToPath(import.meta.url)); +const REPO_ROOT = path.resolve(__dirname, '..', '..'); +const HEADLESS_CSPROJ = path.join( + REPO_ROOT, + 'WitcherScriptMerger.Headless', + 'WitcherScriptMerger.Headless.csproj', +); +const HEADLESS_BUILD_DIR = path.join( + REPO_ROOT, + 'WitcherScriptMerger.Headless', + 'bin', + 'Debug', + 'net10.0', +); +const HEADLESS_EXE = path.join(HEADLESS_BUILD_DIR, 'WitcherScriptMerger.Headless.exe'); + +// Same escaping rationale as every other integration test's own identically-named helper +// (e.g. mcpClient.integration.test.ts) - kept local since it's test-fixture plumbing. +function escapeXmlAttribute(value: string): string { + return value + .replace(/&/g, '&') + .replace(//g, '>') + .replace(/"/g, '"') + .replace(/'/g, '''); +} + +function buildScratchConfig(gameDirectory: string, modsDirectory: string): string { + return ` + + + + + + + + + + + + + + + + + + +`; +} + +const VANILLA_WS_CONTENT = + 'function FuncA() {\r\n' + + ' var a : int;\r\n' + + ' a = 1;\r\n' + + '}\r\n' + + '\r\n' + + 'function FuncB() {\r\n' + + ' var b : int;\r\n' + + ' b = 1;\r\n' + + '}\r\n'; +const MOD1_WS_CONTENT = VANILLA_WS_CONTENT.replace('a = 1;', 'a = 100;'); +const MOD2_WS_CONTENT = VANILLA_WS_CONTENT.replace('b = 1;', 'b = 200;'); + +let scratchDir: string; +let exePath: string; + +beforeAll(() => { + if (!fs.existsSync(HEADLESS_EXE)) { + const result = spawnSync('dotnet', ['build', HEADLESS_CSPROJ, '-c', 'Debug'], { + cwd: REPO_ROOT, + stdio: 'inherit', + }); + if (result.status !== 0) { + throw new Error( + `dotnet build of WitcherScriptMerger.Headless failed (required to run the coexistenceGuard ` + + `integration test) - exit code ${result.status}`, + ); + } + } + + if (!fs.existsSync(HEADLESS_EXE)) { + throw new Error(`Expected built exe not found at ${HEADLESS_EXE} even after building.`); + } + + scratchDir = fs.mkdtempSync(path.join(os.tmpdir(), 'wsm-coexistence-test-')); + fs.cpSync(HEADLESS_BUILD_DIR, scratchDir, { recursive: true }); + + const gameDir = path.join(scratchDir, 'Game'); + const modsDir = path.join(scratchDir, 'Mods'); + const vanillaScriptsDir = path.join(gameDir, 'content', 'content0', 'scripts', 'game'); + const mod1ScriptDir = path.join(modsDir, 'mod0001_First', 'content', 'scripts', 'game'); + const mod2ScriptDir = path.join(modsDir, 'mod0002_Second', 'content', 'scripts', 'game'); + + for (const dir of [vanillaScriptsDir, mod1ScriptDir, mod2ScriptDir]) { + fs.mkdirSync(dir, { recursive: true }); + } + + fs.writeFileSync(path.join(vanillaScriptsDir, 'itemA.ws'), VANILLA_WS_CONTENT, 'utf8'); + fs.writeFileSync(path.join(mod1ScriptDir, 'itemA.ws'), MOD1_WS_CONTENT, 'utf8'); + fs.writeFileSync(path.join(mod2ScriptDir, 'itemA.ws'), MOD2_WS_CONTENT, 'utf8'); + + fs.writeFileSync( + path.join(scratchDir, 'WitcherScriptMerger.Headless.dll.config'), + buildScratchConfig(gameDir, modsDir), + 'utf8', + ); + + exePath = path.join(scratchDir, 'WitcherScriptMerger.Headless.exe'); +}, 300_000); + +afterAll(() => { + if (scratchDir) { + fs.rmSync(scratchDir, { recursive: true, force: true }); + } +}); + +describe('coexistenceGuard integration (real WSM Headless process, real merge round trip)', () => { + it('computeMergeStateSnapshot reflects the real merged-mod folder + MergeInventory.xml before and after a real merge, and checkCoexistenceDrift reacts to the difference', async () => { + resetCoexistenceGuardState(); + + const client = await WsmMcpClient.connect({ exePath }); + try { + // Before any merge: get_status reports a real modsDirectory/mergedModName (plain + // config reads - WsmMcpTools.GetStatus, confirmed not gated on modsDirectoryExists) + // even though nothing has been merged yet, and list_merges is empty. Both signals + // this module compares should therefore be empty strings. + const before = await computeMergeStateSnapshot(client); + expect(before.mergedModName).toBe('mod0000_MergedFiles'); + expect(before.folderListingSignature).toBe(''); + expect(before.mergeHistorySignature).toBe(''); + + const mergeResult = await client.mergeConflicts({ dryRun: false }); + expect(mergeResult.merged).toEqual([path.join('game', 'itemA.ws')]); + + // After a real, non-dry-run merge, using the same still-open client (the same + // amortization resolveAction.ts's own runMergeConflictsWorkflow relies on). + const after = await computeMergeStateSnapshot(client); + expect(after.folderListingSignature).not.toBe(''); + expect(after.mergeHistorySignature).not.toBe(''); + expect(after.folderListingSignature).not.toBe(before.folderListingSignature); + expect(after.mergeHistorySignature).not.toBe(before.mergeHistorySignature); + + // Ties the real round trip above to this module's own detect/warn logic, the same + // way conflictScan.integration.test.ts feeds a real scanConflicts() result into + // notifyConflictsIfChanged. + const sendNotification = createNotificationSpy(); + const fakeApi = { getState: () => ({}), sendNotification, showDialog: async () => ({ action: 'Close', input: {} }) }; + + checkCoexistenceDrift(fakeApi as never, before); + expect(sendNotification.calls).toHaveLength(0); // first observation - only seeds the baseline + + checkCoexistenceDrift(fakeApi as never, after); + expect(sendNotification.calls).toHaveLength(1); + expect(sendNotification.calls[0].id).toBe(WSM_COEXISTENCE_NOTIFICATION_ID); + expect(sendNotification.calls[0].type).toBe('warning'); + + // Re-checking the same (already-warned-about) state must not re-notify. + checkCoexistenceDrift(fakeApi as never, after); + expect(sendNotification.calls).toHaveLength(1); + } finally { + await client.close(); + } + }, 30_000); +}); + +// Tiny hand-rolled spy (no vi.fn() - this file intentionally exercises the real +// mcpClient/coexistenceGuard modules with no mocking whatsoever), mirroring +// conflictScan.integration.test.ts's own createNotificationSpy. +function createNotificationSpy() { + const calls: Array<{ id?: string; type: string; message: string; actions?: unknown[] }> = []; + const fn = (notification: { id?: string; type: string; message: string; actions?: unknown[] }) => { + calls.push(notification); + return notification.id ?? 'generated-id'; + }; + return Object.assign(fn, { calls }); +}