diff --git a/vortex-extension/src/conflictNotifications.test.ts b/vortex-extension/src/conflictNotifications.test.ts new file mode 100644 index 0000000..685b299 --- /dev/null +++ b/vortex-extension/src/conflictNotifications.test.ts @@ -0,0 +1,276 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest'; +import { ScanConflictsResult } from './mcpClient'; +import { + computeConflictSignature, + isModOrDependencyInstallActive, + notifyConflictsIfChanged, + resetConflictNotificationState, + WSM_CONFLICTS_NOTIFICATION_ID, +} from './conflictNotifications'; + +function conflict(relativePath: string, alreadyResolved = false, modNames: string[] = []): ScanConflictsResult[number] { + return { + relativePath, + category: 'Script', + mods: modNames.map((name) => ({ name, hash: '', isOutdated: false })), + defaultOrder: [], + alreadyResolved, + }; +} + +// `activity` intentionally accepts either shape: `string[]` is the real, confirmed +// runtime shape (Vortex's `session` reducer's `startActivity`/`stopActivity` push/filter +// a plain array per group - see conflictNotifications.ts's own `activityEntries` doc +// comment for the exact citation), while `@nexusmods/vortex-api`'s published `lib/api.d.ts` +// types it as a single `string` - stale, but tolerated too so this doesn't break again if +// a future SDK version actually matches its own types. +function fakeApi(activity: Record = {}) { + return { + getState: () => ({ session: { base: { activity } } }), + sendNotification: vi.fn().mockReturnValue(WSM_CONFLICTS_NOTIFICATION_ID), + dismissNotification: vi.fn(), + }; +} + +describe('computeConflictSignature', () => { + it('is order-independent (sorted before joining)', () => { + expect(computeConflictSignature([conflict('b.ws'), conflict('a.ws')])).toBe( + computeConflictSignature([conflict('a.ws'), conflict('b.ws')]), + ); + }); + + it('is empty for no conflicts', () => { + expect(computeConflictSignature([])).toBe(''); + }); + + it('differs when the conflict set differs', () => { + expect(computeConflictSignature([conflict('a.ws')])).not.toBe(computeConflictSignature([conflict('a.ws'), conflict('b.ws')])); + }); + + it('differs when the same relativePath gains a new contributing mod, even though the path set is unchanged', () => { + // A relativePath-only signature would miss this: WSM's own scan_conflicts + // de-duplicates by relativePath, so the same file staying in the unresolved set + // across two scans doesn't mean nothing changed about it - a third mod starting to + // touch an already-conflicting file is real, user-relevant information. + const before = computeConflictSignature([conflict('a.ws', false, ['modA', 'modB'])]); + const after = computeConflictSignature([conflict('a.ws', false, ['modA', 'modB', 'modC'])]); + expect(before).not.toBe(after); + }); + + it('is order-independent in the contributing mod names too', () => { + expect(computeConflictSignature([conflict('a.ws', false, ['modB', 'modA'])])).toBe( + computeConflictSignature([conflict('a.ws', false, ['modA', 'modB'])]), + ); + }); +}); + +describe('notifyConflictsIfChanged', () => { + beforeEach(() => { + resetConflictNotificationState(); + }); + + it('sends a notification with the documented shape when unresolved conflicts are found', () => { + const api = fakeApi(); + + notifyConflictsIfChanged(api as never, [conflict('a.ws')]); + + expect(api.sendNotification).toHaveBeenCalledTimes(1); + const notification = api.sendNotification.mock.calls[0][0]; + expect(notification.id).toBe(WSM_CONFLICTS_NOTIFICATION_ID); + expect(notification.allowSuppress).toBe(true); + expect(notification.actions).toEqual([]); + expect(typeof notification.type).toBe('string'); + expect(typeof notification.message).toBe('string'); + }); + + it('uses a notification id distinct from the built-in game-witcher3 extension\'s "witcher3-merge"', () => { + expect(WSM_CONFLICTS_NOTIFICATION_ID).not.toBe('witcher3-merge'); + }); + + it('does not re-notify on a second call with the identical conflict set (same signature)', () => { + const api = fakeApi(); + + notifyConflictsIfChanged(api as never, [conflict('a.ws')]); + notifyConflictsIfChanged(api as never, [conflict('a.ws')]); + + expect(api.sendNotification).toHaveBeenCalledTimes(1); + }); + + it('notifies again when the conflict set actually changes', () => { + const api = fakeApi(); + + notifyConflictsIfChanged(api as never, [conflict('a.ws')]); + notifyConflictsIfChanged(api as never, [conflict('a.ws'), conflict('b.ws')]); + + expect(api.sendNotification).toHaveBeenCalledTimes(2); + }); + + it('notifies again when the same relativePath set gains a new contributing mod', () => { + const api = fakeApi(); + + notifyConflictsIfChanged(api as never, [conflict('a.ws', false, ['modA', 'modB'])]); + notifyConflictsIfChanged(api as never, [conflict('a.ws', false, ['modA', 'modB', 'modC'])]); + + expect(api.sendNotification).toHaveBeenCalledTimes(2); + }); + + it('excludes already-resolved conflicts from both the count and the change signature', () => { + const api = fakeApi(); + + notifyConflictsIfChanged(api as never, [conflict('a.ws'), conflict('resolved.ws', true)]); + // Only the already-resolved one changes (a different resolved file) - unresolved set + // ('a.ws') is identical, so this must not re-notify. + notifyConflictsIfChanged(api as never, [conflict('a.ws'), conflict('other-resolved.ws', true)]); + + expect(api.sendNotification).toHaveBeenCalledTimes(1); + }); + + it('dismisses any existing notification once the unresolved set becomes empty', () => { + const api = fakeApi(); + + notifyConflictsIfChanged(api as never, [conflict('a.ws')]); + notifyConflictsIfChanged(api as never, [conflict('a.ws', true)]); + + expect(api.sendNotification).toHaveBeenCalledTimes(1); + expect(api.dismissNotification).toHaveBeenCalledWith(WSM_CONFLICTS_NOTIFICATION_ID); + }); + + // Regression test: lastNotifiedSignature now starts at '' (computeConflictSignature's + // own value for "no conflicts"), not undefined, specifically so this first-ever check + // matches the same-signature early-return and never calls dismissNotification for a + // notification that was never sent. + it('does not call dismissNotification on the very first check of a session when there are no conflicts at all', () => { + const api = fakeApi(); + + notifyConflictsIfChanged(api as never, []); + + expect(api.dismissNotification).not.toHaveBeenCalled(); + expect(api.sendNotification).not.toHaveBeenCalled(); + }); + + describe('failure handling (send/dismiss must not corrupt suppression state)', () => { + it('does not throw, and does not record the signature as "shown", when sendNotification throws', () => { + const api = fakeApi(); + api.sendNotification.mockImplementation(() => { + throw new Error('store dispatch failed'); + }); + + expect(() => notifyConflictsIfChanged(api as never, [conflict('a.ws')])).not.toThrow(); + + // The user never actually saw a notification - a later check with the identical + // conflict set must retry, not silently treat it as already shown. + const retryApi = fakeApi(); + notifyConflictsIfChanged(retryApi as never, [conflict('a.ws')]); + expect(retryApi.sendNotification).toHaveBeenCalledTimes(1); + }); + + it('retries dismissNotification on a later call after a prior attempt failed, rather than silently treating the failure as success', () => { + const api = fakeApi(); + // First, genuinely show a notification so the next calls take the "unresolved + // set became empty" -> dismissNotification path. + notifyConflictsIfChanged(api as never, [conflict('a.ws')]); + expect(api.sendNotification).toHaveBeenCalledTimes(1); + + api.dismissNotification.mockImplementationOnce(() => { + throw new Error('store dispatch failed'); + }); + expect(() => notifyConflictsIfChanged(api as never, [conflict('a.ws', true)])).not.toThrow(); + expect(api.dismissNotification).toHaveBeenCalledTimes(1); + + // Same (still-empty) unresolved set again. If the failed attempt above had + // wrongly committed '' as the new "last known" signature (the pre-fix ordering + // bug), this call would see signature === lastNotifiedSignature and skip + // retrying entirely - the dismiss would never actually happen and a stale + // notification would linger forever. The fix leaves the prior successful ('a.ws') + // signature in place on failure, so this must retry. + notifyConflictsIfChanged(api as never, [conflict('a.ws', true)]); + expect(api.dismissNotification).toHaveBeenCalledTimes(2); + }); + }); + + describe('activity-in-progress suppression (real Vortex shape: string[] per group)', () => { + it('skips entirely (no state mutation) while a dependency install is in progress', () => { + const api = fakeApi({ installing_dependencies: ['some-mod-id'] }); + + notifyConflictsIfChanged(api as never, [conflict('a.ws')]); + expect(api.sendNotification).not.toHaveBeenCalled(); + + // Once the install activity clears, the very first real check must still notify - + // proof that the skip above didn't record 'a.ws' as already-seen. + const apiAfter = fakeApi({}); + notifyConflictsIfChanged(apiAfter as never, [conflict('a.ws')]); + expect(apiAfter.sendNotification).toHaveBeenCalledTimes(1); + }); + + // Regression test for the exact trap `activityEntries` exists to avoid: Vortex's own + // `stopActivity` reducer never deletes the group key, it filters the id out and + // leaves an empty array behind (`session.ts`: `[group]: group.filter(id => id !== + // activityId)`). A naive `Boolean(activity.installing_dependencies)` truthiness + // check would see `[]` (truthy in JS) and report "still active" forever after the + // first dependency install ever completed. + it('does NOT treat a stale, now-empty activity array (left behind by a completed install) as still active', () => { + const api = fakeApi({ installing_dependencies: [] }); + + notifyConflictsIfChanged(api as never, [conflict('a.ws')]); + + expect(api.sendNotification).toHaveBeenCalledTimes(1); + }); + + it('skips while a plain mod install is in progress (activity.mods includes "installing")', () => { + const api = fakeApi({ mods: ['installing'] }); + + notifyConflictsIfChanged(api as never, [conflict('a.ws')]); + + expect(api.sendNotification).not.toHaveBeenCalled(); + }); + + it('does NOT skip merely because activity.mods contains "deployment" - the own did-deploy handler runs during that window', () => { + const api = fakeApi({ mods: ['deployment'] }); + + notifyConflictsIfChanged(api as never, [conflict('a.ws')]); + + expect(api.sendNotification).toHaveBeenCalledTimes(1); + }); + + it('does not skip merely because some unrelated activity group is non-empty', () => { + const api = fakeApi({ discovery: ['scanning'] }); + + notifyConflictsIfChanged(api as never, [conflict('a.ws')]); + + expect(api.sendNotification).toHaveBeenCalledTimes(1); + }); + }); + + // isModOrDependencyInstallActive is exported (not just used internally by + // notifyConflictsIfChanged above) specifically so index.ts's checkForConflictsAfterDeploy + // can also call it directly, before ever calling scanWsmConflicts - see index.ts's own + // comment and index.test.ts's own test for that pre-check. Exercised directly here too, + // since the same-file suppression tests above only cover it indirectly. + describe('isModOrDependencyInstallActive (exported directly for index.ts\'s pre-scan check)', () => { + it('returns true while a dependency install is in progress', () => { + expect(isModOrDependencyInstallActive(fakeApi({ installing_dependencies: ['some-mod-id'] }) as never)).toBe(true); + }); + + it('returns false when no relevant activity is present', () => { + expect(isModOrDependencyInstallActive(fakeApi({}) as never)).toBe(false); + }); + }); + + describe('activity shape tolerance (documented-but-stale @nexusmods/vortex-api shape: single string)', () => { + it('still skips if a future/different Vortex build reports a plain string instead of an array', () => { + const api = fakeApi({ installing_dependencies: 'some-mod-id' }); + + notifyConflictsIfChanged(api as never, [conflict('a.ws')]); + + expect(api.sendNotification).not.toHaveBeenCalled(); + }); + + it('treats an empty string the same as "not active"', () => { + const api = fakeApi({ installing_dependencies: '' }); + + notifyConflictsIfChanged(api as never, [conflict('a.ws')]); + + expect(api.sendNotification).toHaveBeenCalledTimes(1); + }); + }); +}); diff --git a/vortex-extension/src/conflictNotifications.ts b/vortex-extension/src/conflictNotifications.ts new file mode 100644 index 0000000..79d0164 --- /dev/null +++ b/vortex-extension/src/conflictNotifications.ts @@ -0,0 +1,226 @@ +import { log, types } from 'vortex-api'; +import { ScanConflictsResult } from './mcpClient'; + +/** + * Dashboard-notification id this extension uses for "you have unresolved WSM script + * conflicts" - deliberately distinct from `"witcher3-merge"`, the notification id + * Vortex's own built-in `game-witcher3` extension uses for its own (unconditional, + * scan-less) post-deploy "you may need to run the script merger" prompt (confirmed + * directly against that extension's current source, + * `extensions/games/game-witcher3/src/eventHandlers.ts`'s `queryScriptMerge`, in the + * `Nexus-Mods/Vortex` monorepo - see this unit's PR description for the exact commit + * fetched). Reusing that id would let a user with both extensions installed silently + * overwrite/collide on the same notification slot instead of seeing both. + */ +export const WSM_CONFLICTS_NOTIFICATION_ID = 'witcherscriptmerger-vortex-conflicts'; + +/** + * Builds a stable, order-independent signature for a set of conflicts, used to detect + * whether the *actual* unresolved-conflict set changed since the last check this + * session (see `notifyConflictsIfChanged` below). + * + * Deliberately includes each conflict's own contributing mod names, not just its + * `relativePath` alone - a per-conflict `relativePath:sortedModNames` entry, the + * entries themselves then sorted and newline-joined. `relativePath` alone would miss a + * real, user-relevant change: WSM's own `scan_conflicts` de-duplicates by + * `relativePath` (`FileIndex/ModFileIndex.cs`'s `GetModFilesFromPaths` folds multiple + * mods touching the same file into one `ModFile` entry), so no two entries in a single + * scan result share a `relativePath` - but the *set of mods* contributing to that same + * `relativePath` can change between two scans (e.g. a third mod starts touching an + * already-conflicting file that was never merged), and a `relativePath`-only signature + * can't tell that apart from "nothing changed," silently missing the re-notification a + * user would want. `|` and `:` are both used as delimiters here, and `\n` to join + * entries - none of the three can appear in a Windows file/directory name (`|`, `:`, + * and newline are all part of Windows' reserved-character/control-character set), so + * there's no realistic way for two genuinely different conflict sets to collide onto + * the same signature string. + */ +export function computeConflictSignature(conflicts: ScanConflictsResult): string { + return conflicts + .map((c) => `${c.relativePath}:${[...c.mods.map((m) => m.name)].sort().join('|')}`) + .sort() + .join('\n'); +} + +/** + * Normalizes one `state.session.base.activity` group entry into a plain string array, + * tolerating either shape a caller might see there. + * + * **The published `@nexusmods/vortex-api` `lib/api.d.ts` types `ISession.activity` as + * `{[group: string]: string}` - confirmed stale by reading the real reducer.** Fetched + * directly (`gh api`, `src/renderer/src/reducers/session.ts` in the `Nexus-Mods/Vortex` + * monorepo - see this unit's PR description for the exact commit SHA): `startActivity` + * does `activity: {...state.activity, [group]: [...(state.activity[group] ?? []), + * activityId]}` and `stopActivity` does `[group]: (state.activity[group] ?? + * []).filter(id => id !== activityId)` - i.e. the real runtime value is a `string[]` + * per group (letting two concurrent activities share one group), and critically, + * `stopActivity` never deletes the group key - it leaves an **empty array** behind. A + * naive `Boolean(activity[group])` truthiness check would therefore see `[]` (truthy in + * JS) and report "still active" forever after the first activity in that group ever + * ran, permanently suppressing every future notification - a real, easy-to-miss trap + * this function exists specifically to avoid. Written to tolerate the *documented* + * shape too (a single string), not just the confirmed-real one, so this doesn't quietly + * break again if a future SDK version actually matches its own published types. + */ +function activityEntries(value: unknown): string[] { + if (Array.isArray(value)) { + return value as string[]; + } + if (typeof value === 'string' && value.length > 0) { + return [value]; + } + return []; +} + +/** + * True while Vortex reports a mod-install or dependency-install operation in progress, + * per `state.session.base.activity`. + * + * Exported (not just used internally by `notifyConflictsIfChanged` below) so + * `index.ts`'s `checkForConflictsAfterDeploy` can also check it *before* calling + * `scanWsmConflicts` - avoiding an entirely wasted WSM process spawn during, e.g., a + * Collection install that triggers several deploy-per-mod cycles in a row, since + * `notifyConflictsIfChanged` would just discard that scan's result anyway. That + * pre-check is a pure optimization, not a correctness requirement - this function is + * still called again inside `notifyConflictsIfChanged` itself, because activity can + * start at any point during the scan that follows a clean pre-check. + * + * The two specific group/id checks below are not a guess at plausible-sounding names - + * both are confirmed directly against the real, current `Nexus-Mods/Vortex` monorepo + * source (fetched via `gh api`; see this unit's PR description for exact file paths and + * commit SHAs): + * + * - A non-empty `installing_dependencies` group is the *exact* guard Vortex's own + * built-in `game-witcher3` extension already uses before showing its own + * `"witcher3-merge"` conflict-adjacent notification + * (`eventHandlers.ts`'s `queryScriptMerge`: `if ((state.session.base.activity + * ?.installing_dependencies ?? []).length > 0) { return; }` - note this real call + * site already assumes the array shape, another independent confirmation of the + * `activityEntries` finding above) - this is the single closest possible prior art + * for this exact "should I bug the user about script conflicts right now" decision, + * so it's mirrored here deliberately rather than independently re-derived. The group + * is populated by `startActivity("installing_dependencies", )` in + * `mod_management/InstallManager.ts`'s `withActivityTracking`, called around both + * `installRecommendationsImpl`/`installDependenciesImpl` (collection/dependency + * installs). + * - `'installing'` present in the `mods` group covers the plainer single-mod-install + * case, populated by `startActivity("mods", "installing")` in + * `mod_management/InstallContext.ts`. **Deliberately not** "the `mods` group is + * non-empty" - that same group key is also used for `startActivity("mods", + * "deployment")` in `mod_management/index.ts`, and - confirmed directly by reading + * that file - `stopActivity("mods", "deployment")` only fires *after* + * `emitAndAwait("did-deploy", ...)` resolves, i.e. after every `did-deploy` handler + * (including this extension's own) has already run. A blanket "is the `mods` group + * non-empty" check would therefore always be true from inside this extension's own + * `did-deploy` handler and permanently suppress every notification - checking for the + * specific `'installing'` entry (not `'deployment'`) is what avoids that. Unlike the + * `installing_dependencies` check above, this one has no direct precedent in + * `game-witcher3`'s own code (it doesn't check this) - included because this unit's + * own task explicitly names "mod-install" alongside "dependency-install", but worth + * flagging as the less battle-tested of the two. + */ +export function isModOrDependencyInstallActive(api: types.IExtensionApi): boolean { + // api.getState() defaults to IState (its generic parameter's own default), so + // session.base.activity is real, typed state here, not a hand-rolled shape - the + // optional chaining is defensive only (a fake `api` in a unit test need not supply + // every nested field), not a hedge against the real type being different. + const activity = api.getState()?.session?.base?.activity; + if (!activity) { + return false; + } + return ( + activityEntries(activity.installing_dependencies).length > 0 || + activityEntries(activity.mods).includes('installing') + ); +} + +/** Module-level "last seen" state - deliberately in-memory only and scoped to this + * extension's own process lifetime (one Vortex session), per this unit's own + * suppression requirement: no persisted cross-session state needed. Initialized to + * `''` - the same value `computeConflictSignature([])` produces for "no unresolved + * conflicts" - rather than `undefined`, specifically so the very first check of a + * 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. */ +let lastNotifiedSignature = ''; + +export function resetConflictNotificationState(): void { + lastNotifiedSignature = ''; +} + +/** + * Given an already-obtained `scan_conflicts` result, shows (or updates/dismisses) the + * dashboard notification for unresolved WSM conflicts - deliberately takes the scan + * result as a plain argument, rather than performing the scan itself, so it's directly + * unit-testable against a fabricated conflicts array without spawning any process (see + * `conflictNotifications.test.ts`). `index.ts`'s `did-deploy` handler is the only + * production caller, but nothing here reaches into Vortex's event system itself. + * + * Suppression: skips entirely (no state mutated at all) while + * `isModOrDependencyInstallActive` is true, so a later, real post-install `did-deploy` + * still gets a fair chance to notify against the real conflict set rather than being + * silently marked "already seen" by a scan taken mid-install. Otherwise, only sends a + * fresh notification when the *unresolved* (`!alreadyResolved`) conflict set's + * signature actually changed since the last check this session; a conflict + * `scan_conflicts` reports as `alreadyResolved` has a recorded, still-valid merge already covering it + * (`AppState.Inventory.HasResolvedConflict`, `WitcherScriptMerger.Core/Mcp/WsmMcpTools.cs`) + * and needs no user action, so it's excluded from both the notification count and the + * signature entirely - otherwise a deployment that changes nothing conflict-relevant + * would still show a stale "already-resolved" conflict as if it were new. If the + * signature changes to "no unresolved conflicts" (e.g. the user resolved them via the + * GUI since the last check), any existing notification is dismissed rather than left + * stale. + * + * `lastNotifiedSignature` is committed only *after* the `sendNotification`/ + * `dismissNotification` call itself completes without throwing - deliberately not + * before. `context.api.sendNotification`/`dismissNotification` are both typed optional + * (`?:`) on `IExtensionApi`, and this function is reachable from `index.ts`'s + * `onAsync('did-deploy', ...)` handler where a thrown error must never propagate (see + * that file's own comment on `onAsync`'s contract) - so any failure here is caught, + * logged, and swallowed locally rather than left to a misleading catch-all message + * further up the call stack. Committing the signature only on success matters + * concretely: if the call had failed *after* the signature was already recorded, the + * user would never have actually seen the notification, yet every later `did-deploy` + * with that same conflict set would silently skip re-attempting it for the rest of the + * session (the same-signature early-return above would treat it as "already shown"). + */ +export function notifyConflictsIfChanged(api: types.IExtensionApi, conflicts: ScanConflictsResult): void { + if (isModOrDependencyInstallActive(api)) { + log('debug', 'witcherscriptmerger-vortex: mod/dependency install activity in progress - skipping conflict notification check'); + return; + } + + const unresolved = conflicts.filter((c) => !c.alreadyResolved); + const signature = computeConflictSignature(unresolved); + + if (signature === lastNotifiedSignature) { + return; + } + + try { + if (unresolved.length === 0) { + api.dismissNotification?.(WSM_CONFLICTS_NOTIFICATION_ID); + } else { + api.sendNotification?.({ + id: WSM_CONFLICTS_NOTIFICATION_ID, + type: 'warning', + message: `WitcherScriptMerger: ${unresolved.length} unresolved script conflict${unresolved.length === 1 ? '' : 's'} found`, + allowSuppress: true, + actions: [], + }); + } + } catch (err) { + // Never let this escape to the caller - see this function's own doc comment above. + // Deliberately not committing lastNotifiedSignature below in this branch: a failed + // attempt must not be recorded as "already shown," or the user would silently never + // see it, this session, for this exact conflict set. + log('warn', 'witcherscriptmerger-vortex: failed to show/update the conflict notification', { + error: err instanceof Error ? err.message : String(err), + }); + return; + } + + lastNotifiedSignature = signature; +} diff --git a/vortex-extension/src/conflictScan.test.ts b/vortex-extension/src/conflictScan.test.ts new file mode 100644 index 0000000..2d61abf --- /dev/null +++ b/vortex-extension/src/conflictScan.test.ts @@ -0,0 +1,180 @@ +import * as fs from 'fs'; +import * as os from 'os'; +import * as path from 'path'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { WSM_HEADLESS_EXE_NAME } from './toolAcquisition'; + +// vi.mock factories are hoisted above imports, so the mock function itself has to come +// from vi.hoisted - same pattern index.test.ts already uses to isolate index.ts's own +// wiring from toolAcquisition.ts's real behavior. Here it isolates conflictScan.ts's own +// orchestration (exePath/env resolution, connect/scan/close sequencing) from +// mcpClient.ts's real process-spawning behavior, which is instead covered end-to-end by +// test/conflictScan.integration.test.ts. +const { connectMock } = vi.hoisted(() => ({ + connectMock: vi.fn(), +})); + +vi.mock('./mcpClient', () => ({ + WsmMcpClient: { connect: connectMock }, +})); + +import { getWsmExePath, isWsmToolAcquired, scanWsmConflicts } from './conflictScan'; + +function fakeApi(userDataDir: string, discoveredGamePath?: string) { + return { + getPath: (name: string) => (name === 'userData' ? userDataDir : `/unexpected/${name}`), + getState: () => ({ discoveryByGame: { witcher3: discoveredGamePath !== undefined ? { path: discoveredGamePath } : undefined } }), + } as unknown as Parameters[0]; +} + +describe('getWsmExePath', () => { + it('points at the acquired WSM Headless exe under the tool storage dir', () => { + const api = fakeApi(path.join('C:', 'fake', 'userData')); + expect(getWsmExePath(api)).toBe( + path.join('C:', 'fake', 'userData', 'witcherscriptmerger-vortex', 'tool', WSM_HEADLESS_EXE_NAME), + ); + }); +}); + +describe('isWsmToolAcquired', () => { + let userDataDir: string; + + beforeEach(() => { + userDataDir = fs.mkdtempSync(path.join(os.tmpdir(), 'wsm-vortex-conflictscan-test-')); + }); + + afterEach(() => { + fs.rmSync(userDataDir, { recursive: true, force: true }); + }); + + it('returns false when no exe has been acquired yet', async () => { + const api = fakeApi(userDataDir); + await expect(isWsmToolAcquired(api)).resolves.toBe(false); + }); + + it('returns true once the exe exists on disk', async () => { + const api = fakeApi(userDataDir); + const toolDir = path.join(userDataDir, 'witcherscriptmerger-vortex', 'tool'); + fs.mkdirSync(toolDir, { recursive: true }); + fs.writeFileSync(path.join(toolDir, WSM_HEADLESS_EXE_NAME), 'fake exe bytes', 'utf8'); + + await expect(isWsmToolAcquired(api)).resolves.toBe(true); + }); + + // Mirrors toolAcquisition.test.ts's equivalent test for ensureWsmToolRegistered - this + // function is documented as mirroring that module's own pathExists helper in + // substance, not just in name, specifically so a locked/permission-denied file isn't + // silently mistaken for "nothing installed yet". + it('propagates a non-ENOENT filesystem error rather than silently treating it as "not acquired"', async () => { + const api = fakeApi(userDataDir); + const accessError = Object.assign(new Error('EBUSY: resource busy or locked'), { code: 'EBUSY' }); + const accessSpy = vi.spyOn(fs.promises, 'access').mockRejectedValueOnce(accessError); + + try { + await expect(isWsmToolAcquired(api)).rejects.toThrow(/EBUSY/); + } finally { + accessSpy.mockRestore(); + } + }); +}); + +describe('scanWsmConflicts', () => { + beforeEach(() => { + connectMock.mockReset(); + }); + + it('connects with the acquired exe path and Witcher 3 discovered game directory, scans, then always closes', async () => { + const closeMock = vi.fn().mockResolvedValue(undefined); + const scanConflictsMock = vi.fn().mockResolvedValue([{ relativePath: 'foo.ws' }]); + connectMock.mockResolvedValue({ scanConflicts: scanConflictsMock, close: closeMock }); + + const api = fakeApi(path.join('C:', 'fake', 'userData'), path.join('C:', 'Games', 'Witcher3')); + + const result = await scanWsmConflicts(api); + + expect(result).toEqual([{ relativePath: 'foo.ws' }]); + expect(connectMock).toHaveBeenCalledTimes(1); + const connectArgs = connectMock.mock.calls[0][0] as { exePath: string; env: Record; requestTimeoutMs: number }; + expect(connectArgs.exePath).toBe(getWsmExePath(api)); + expect(connectArgs.env.WSM_GameDirectory).toBe(path.join('C:', 'Games', 'Witcher3')); + expect(scanConflictsMock).toHaveBeenCalledTimes(1); + expect(closeMock).toHaveBeenCalledTimes(1); + }); + + // This handler runs inside Vortex's own emitAndAwait('did-deploy', ...) await window + // (see index.ts's own doc comment) - a hung WSM process shouldn't be able to block + // Vortex's reported deployment-completion for mcpClient.ts's full general-purpose 30s + // default (up to ~60s worst case across two requests). + it('requests a shorter-than-default MCP timeout, since this runs inside did-deploy\'s own blocking window', async () => { + const closeMock = vi.fn().mockResolvedValue(undefined); + const scanConflictsMock = vi.fn().mockResolvedValue([]); + connectMock.mockResolvedValue({ scanConflicts: scanConflictsMock, close: closeMock }); + + const api = fakeApi(path.join('C:', 'fake', 'userData')); + await scanWsmConflicts(api); + + const connectArgs = connectMock.mock.calls[0][0] as { requestTimeoutMs?: number }; + expect(connectArgs.requestTimeoutMs).toBeDefined(); + expect(connectArgs.requestTimeoutMs).toBeLessThan(30_000); + expect(connectArgs.requestTimeoutMs).toBeGreaterThan(0); + }); + + it('still closes the client when scanConflicts itself throws', async () => { + const closeMock = vi.fn().mockResolvedValue(undefined); + const scanConflictsMock = vi.fn().mockRejectedValue(new Error('boom')); + connectMock.mockResolvedValue({ scanConflicts: scanConflictsMock, close: closeMock }); + + const api = fakeApi(path.join('C:', 'fake', 'userData')); + + await expect(scanWsmConflicts(api)).rejects.toThrow('boom'); + expect(closeMock).toHaveBeenCalledTimes(1); + }); + + it('does not attempt to close when connect itself fails (nothing to close)', async () => { + connectMock.mockRejectedValue(new Error('spawn failed')); + + const api = fakeApi(path.join('C:', 'fake', 'userData')); + + await expect(scanWsmConflicts(api)).rejects.toThrow('spawn failed'); + }); + + // Mirrors toolAcquisition.test.ts's "coalesces concurrent calls" test for + // acquireWsmTool - same rationale: overlapping did-deploy events must not each spawn + // their own WSM process against the same mods folder, and must not let an + // out-of-order resolution feed a stale result to a later caller. + it('coalesces overlapping calls onto a single in-flight connect/scan', async () => { + let resolveScan: ((value: unknown[]) => void) | undefined; + const scanConflictsMock = vi.fn().mockReturnValue( + new Promise((resolve) => { + resolveScan = resolve; + }), + ); + const closeMock = vi.fn().mockResolvedValue(undefined); + connectMock.mockResolvedValue({ scanConflicts: scanConflictsMock, close: closeMock }); + + const api = fakeApi(path.join('C:', 'fake', 'userData')); + + const first = scanWsmConflicts(api); + const second = scanWsmConflicts(api); + + resolveScan?.([{ relativePath: 'a.ws' }]); + const [firstResult, secondResult] = await Promise.all([first, second]); + + expect(firstResult).toBe(secondResult); + expect(connectMock).toHaveBeenCalledTimes(1); + expect(scanConflictsMock).toHaveBeenCalledTimes(1); + }); + + it('allows a fresh scan after a prior one has fully completed (does not coalesce sequential calls)', async () => { + const closeMock = vi.fn().mockResolvedValue(undefined); + const scanConflictsMock = vi.fn().mockResolvedValue([]); + connectMock.mockResolvedValue({ scanConflicts: scanConflictsMock, close: closeMock }); + + const api = fakeApi(path.join('C:', 'fake', 'userData')); + + await scanWsmConflicts(api); + await scanWsmConflicts(api); + + expect(connectMock).toHaveBeenCalledTimes(2); + }); +}); diff --git a/vortex-extension/src/conflictScan.ts b/vortex-extension/src/conflictScan.ts new file mode 100644 index 0000000..7cdd5a1 --- /dev/null +++ b/vortex-extension/src/conflictScan.ts @@ -0,0 +1,155 @@ +import * as fs from 'fs'; +import * as path from 'path'; +import { selectors, types } from 'vortex-api'; +import { WITCHER3_GAME_ID } from './gating'; +import { ScanConflictsResult, WsmMcpClient } from './mcpClient'; +import { getWsmToolDir } from './storage'; +import { WSM_HEADLESS_EXE_NAME } from './toolAcquisition'; +import { buildWsmEnv, mergeWithProcessEnv } from './wsmEnv'; + +/** + * Drives a single WSM conflict scan for Witcher 3, spawned fresh and torn down + * immediately afterward - `mcpClient.ts`'s own documented process-lifecycle policy + * ("spawn per user-initiated workflow ... not a long-lived singleton") applies just as + * much to this unit's post-deployment trigger as it does to a user-initiated one; the + * trigger here is Vortex's own `did-deploy` event rather than a button click, but the + * lifecycle rule doesn't distinguish between the two. + * + * Callers are responsible for their own gating on Witcher 3 - see `gating.ts`'s own doc + * comment for the general rule every feature this extension registers follows. This + * module's only real caller (`index.ts`'s `checkForConflictsAfterDeploy`) does *not* + * use the general-purpose `isWitcher3Active(api)` helper for that gating, though - it + * resolves `did-deploy`'s own `profileId` argument to that specific deployed profile's + * `gameId` instead, since "whichever game is active right now" and "which game this + * particular deployment was actually for" are two different questions for a + * post-deployment hook (see `index.ts`'s own doc comment for the full reasoning). Any + * *other* future caller of `scanWsmConflicts`/`isWsmToolAcquired` that isn't reacting to + * a specific past deployment should still default to `isWitcher3Active(api)`, per + * `gating.ts`'s own general rule. + */ + +/** Absolute path to the WSM Headless exe this extension would have acquired, per + * `storage.ts`'s layout convention - does not check whether it actually exists on + * disk (see `isWsmToolAcquired` below for that). */ +export function getWsmExePath(api: types.IExtensionApi): string { + return path.join(getWsmToolDir(api), WSM_HEADLESS_EXE_NAME); +} + +function isEnoent(err: unknown): boolean { + return typeof err === 'object' && err !== null && (err as NodeJS.ErrnoException).code === 'ENOENT'; +} + +/** + * Cheap, local, network-free existence check for the acquired WSM exe - mirrors + * `toolAcquisition.ts`'s own private `pathExists` helper (not exported, so duplicated + * here rather than reused) for the same reason it exists there: no WSM binary acquired + * yet is a normal, expected state (e.g. before any GitHub Release exists on this repo - + * see `index.ts`'s own doc comment), not an error worth spawning a doomed child process + * over just to discover via a failed `WsmMcpClient.connect`. Critically, this mirrors + * `pathExists` in substance, not just in name: only `ENOENT` is treated as "not + * acquired" - anything else (`EPERM`/`EBUSY` from an antivirus scan or a concurrently + * running WSM process holding the file, a permissions problem, etc.) is a real, + * unexpected condition the caller needs to see, not something to silently paper over as + * "no tool yet." A bare catch-all here would make a transient lock look identical to + * "nothing installed," silently skipping every post-deploy scan for the rest of the + * session with only a misleading 'debug'-level log line - exactly the failure mode + * `pathExists`'s own doc comment calls out. + */ +export async function isWsmToolAcquired(api: types.IExtensionApi): Promise { + try { + await fs.promises.access(getWsmExePath(api)); + return true; + } catch (err) { + if (isEnoent(err)) { + return false; + } + throw err; + } +} + +/** + * Tighter than `mcpClient.ts`'s general-purpose `DEFAULT_REQUEST_TIMEOUT_MS` (30s per + * request, so up to ~60s worst case across the `initialize` handshake and the + * `scan_conflicts` call). This specific call site runs *inside* Vortex's own + * `emitAndAwait('did-deploy', ...)` await window (confirmed by reading + * `mod_management/index.ts` - see `conflictNotifications.ts`'s own doc comment and this + * unit's PR description for the citation): `stopActivity('mods', 'deployment')` doesn't + * fire until every `did-deploy` handler, including this one, resolves. A slow or hung + * WSM process would therefore extend Vortex's own reported deployment-completion time + * by however long this waits - not something an automatic, unrequested background + * trigger should be allowed to do for a full 30-60s. A normal `scan_conflicts` call + * against a real mods folder completes in well under a second (see + * `test/conflictScan.integration.test.ts`), so this still leaves generous headroom for + * a large mod list while bounding the worst case. + */ +const POST_DEPLOY_SCAN_TIMEOUT_MS = 15_000; + +/** Coalesces overlapping calls onto a single in-flight scan, the same pattern + * `toolAcquisition.ts`'s `inFlightAcquisitions` uses for `acquireWsmTool` (see that + * module's own doc comment). Unlike that map (keyed by install dir, since multiple + * distinct installs are meaningful there), a single slot is enough here - this + * extension only ever scans one thing: Witcher 3's own mods folder. Without this, two + * overlapping `did-deploy` events (e.g. Vortex firing a deploy again while a prior + * one's handlers are still resolving) could run two concurrent WSM processes against + * the same mods folder and, worse, resolve out of order - letting a stale scan's + * result reach `notifyConflictsIfChanged` *after* a fresher one already did, showing a + * notification that no longer matches the real current conflict set and recording that + * stale signature as "already seen," suppressing the correct one until the conflict set + * changes again. */ +let inFlightScan: Promise | undefined; + +/** + * Spawns a short-lived `WsmMcpClient`, runs `scan_conflicts`, and closes the client in a + * `finally` - matching `test/mcpClient.integration.test.ts`'s own pattern exactly. + * + * Points the spawned process at Witcher 3's own discovered game directory via the + * `WSM_` env-var mechanism (`wsmEnv.ts`), the same lookup + * `toolAcquisition.ts`'s `registerAcquiredTool` already does for the same reason: a + * spawned WSM process has no idea what "the active game" is on its own, so without this + * it would fall back to whatever `GameDirectory` happens to be baked into the deployed + * `.dll.config` (which may be blank, or stale from a previous game) - see + * `docs/vortex-extension-design.md` section 4.1. + * + * See `inFlightScan`'s own doc comment for why overlapping calls coalesce onto a single + * scan rather than each spawning their own WSM process. + */ +export async function scanWsmConflicts(api: types.IExtensionApi): Promise { + if (inFlightScan) { + return inFlightScan; + } + + const promise = scanWsmConflictsUncoordinated(api); + inFlightScan = promise; + try { + return await promise; + } finally { + inFlightScan = undefined; + } +} + +async function scanWsmConflictsUncoordinated(api: types.IExtensionApi): Promise { + const gameDirectory = selectors.discoveryByGame(api.getState(), WITCHER3_GAME_ID)?.path; + const env = mergeWithProcessEnv(buildWsmEnv({ gameDirectory })); + + // Note a real, acknowledged TOCTOU gap here, not a claim of one that doesn't exist: + // this connect() re-derives the exe path independently of whatever isWsmToolAcquired + // check a caller may have already done (index.ts does one before calling this), so a + // concurrent re-acquisition (acquireWsmTool overwrites installDir in place - see + // storage.ts/toolAcquisition.ts) between that check and this connect() could spawn + // against a mid-write or momentarily-missing exe. Not fixed here: this is an + // inherent check-then-act gap in any two-step "confirm it exists, then use it" + // pattern, and the failure mode is already fully contained - WsmMcpClient.connect + // rejects, and index.ts's own try/catch around this call already logs it as a + // warning rather than crashing or hanging. Worth documenting, not worth adding + // synchronization machinery for a rare, already-safely-handled race. + const client = await WsmMcpClient.connect({ + exePath: getWsmExePath(api), + env, + requestTimeoutMs: POST_DEPLOY_SCAN_TIMEOUT_MS, + }); + try { + return await client.scanConflicts(); + } finally { + await client.close(); + } +} diff --git a/vortex-extension/src/gating.ts b/vortex-extension/src/gating.ts index 4754bdf..20715fe 100644 --- a/vortex-extension/src/gating.ts +++ b/vortex-extension/src/gating.ts @@ -12,11 +12,21 @@ export const WITCHER3_GAME_ID = 'witcher3'; * True only when Witcher 3 is the currently active game. * * Every feature this extension registers - here and in every later unit built on this - * scaffold (tool acquisition, conflict scanning, the merge panel, dashlets) - must be - * gated on this. Vortex loads every installed extension regardless of which game is + * scaffold (tool acquisition, the merge panel, dashlets) - must be gated on this by + * default. Vortex loads every installed extension regardless of which game is * currently active, so without this check, this extension's registrations would apply * (and potentially show UI) for every other game too. * + * **One documented exception**: conflict scanning's `did-deploy` handler + * (`index.ts`'s `checkForConflictsAfterDeploy`) does not use this helper. `did-deploy` + * fires for a *specific* past deployment (identified by its own `profileId` argument), + * and by the time an async handler's own turn comes up, "whichever game is active + * right now" can already differ from "which game that deployment was actually for" - + * using this helper there would risk silently skipping a real Witcher 3 deployment's + * scan. See `index.ts`'s own doc comment for the full reasoning and + * `conflictScan.ts`'s doc comment for where the general rule still applies to that + * module's other callers. + * * Prefer passing this as a live `condition` callback to whichever `context.register*` * API a later unit uses (re-evaluated by Vortex itself on every game-mode switch) * rather than only checking it once at extension-load time - a user can switch the diff --git a/vortex-extension/src/index.test.ts b/vortex-extension/src/index.test.ts index 84f1454..0c6f80b 100644 --- a/vortex-extension/src/index.test.ts +++ b/vortex-extension/src/index.test.ts @@ -2,26 +2,56 @@ import { 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 -// own wiring (what this file actually tests) from toolAcquisition.ts's real behavior -// (already thoroughly covered by toolAcquisition.test.ts). -const { ensureWsmToolRegisteredMock } = vi.hoisted(() => ({ +// own wiring (what this file actually tests) from toolAcquisition.ts/conflictScan.ts/ +// conflictNotifications.ts's own real behavior, each already thoroughly covered by their +// own *.test.ts files. +const { + ensureWsmToolRegisteredMock, + isWsmToolAcquiredMock, + scanWsmConflictsMock, + notifyConflictsIfChangedMock, + isModOrDependencyInstallActiveMock, +} = vi.hoisted(() => ({ ensureWsmToolRegisteredMock: vi.fn(), + isWsmToolAcquiredMock: vi.fn(), + scanWsmConflictsMock: vi.fn(), + notifyConflictsIfChangedMock: vi.fn(), + isModOrDependencyInstallActiveMock: vi.fn(), })); vi.mock('./toolAcquisition', () => ({ ensureWsmToolRegistered: ensureWsmToolRegisteredMock, })); +vi.mock('./conflictScan', () => ({ + isWsmToolAcquired: isWsmToolAcquiredMock, + scanWsmConflicts: scanWsmConflictsMock, +})); + +vi.mock('./conflictNotifications', () => ({ + notifyConflictsIfChanged: notifyConflictsIfChangedMock, + isModOrDependencyInstallActive: isModOrDependencyInstallActiveMock, +})); + import main from './index'; import { WITCHER3_GAME_ID } from './gating'; /** A minimal stand-in for IExtensionContext - just enough surface for index.ts's own - * logic (context.once, context.api.getState/events.on), matching gating.test.ts's own - * fakeApi philosophy: a simplified fake, not a replica of Vortex's real context shape. */ -function fakeContext(initialActiveGameId: string | undefined) { - const state = { activeGameId: initialActiveGameId }; + * logic (context.once, context.api.getState/events.on/onAsync), matching gating.test.ts's + * own fakeApi philosophy: a simplified fake, not a replica of Vortex's real context shape. + * + * `profiles` backs `selectors.profileById` (via the shared `vortexApiStub.ts`) - + * `checkForConflictsAfterDeploy` (index.ts) resolves `did-deploy`'s own `profileId` + * argument to that profile's `gameId`, deliberately *not* `activeGameId` (see index.ts's + * own comment on why those differ) - so tests exercising that handler set up a profile + * entry rather than (only) `setActiveGame`. `activeGameId`/`setActiveGame` remain for the + * unrelated `tryRegisterWsmTool`/`gamemode-activated` tests, which genuinely do gate on + * "what's active right now". */ +function fakeContext(initialActiveGameId: string | undefined, profiles: Record = {}) { + const state = { activeGameId: initialActiveGameId, profiles }; let onceCallback: (() => void) | undefined; const eventListeners = new Map void>>(); + const asyncListeners = new Map Promise>(); const context = { once: (callback: () => void) => { @@ -36,6 +66,9 @@ function fakeContext(initialActiveGameId: string | undefined) { eventListeners.set(eventName, listeners); }, }, + onAsync: (eventName: string, listener: (...args: unknown[]) => Promise) => { + asyncListeners.set(eventName, listener); + }, }, }; @@ -43,6 +76,7 @@ function fakeContext(initialActiveGameId: string | undefined) { context: context as unknown as Parameters[0], fireOnce: () => onceCallback?.(), fireEvent: (eventName: string) => eventListeners.get(eventName)?.forEach((listener) => listener()), + fireAsyncEvent: (eventName: string, ...args: unknown[]) => asyncListeners.get(eventName)?.(...args), setActiveGame: (gameId: string | undefined) => { state.activeGameId = gameId; }, @@ -115,4 +149,205 @@ describe('main (index.ts)', () => { // Let the rejected promise's .catch() handler actually run before the test ends. await new Promise((resolve) => setTimeout(resolve, 0)); }); + + describe('did-deploy conflict scanning', () => { + it('registers a did-deploy handler via onAsync (not events.on) at context.once time', () => { + ensureWsmToolRegisteredMock.mockClear().mockResolvedValue(false); + const { context, fireOnce, fireAsyncEvent } = fakeContext(undefined); + + main(context); + fireOnce(); + + // No handler registered for a plain events.on('did-deploy', ...) - only onAsync. + expect(fireAsyncEvent('did-deploy', 'profile1', undefined)).toBeInstanceOf(Promise); + }); + + it("does nothing when the deployed profile's own game is not witcher3, even if witcher3 happens to be active", async () => { + // Deliberately the inverse of what a naive isWitcher3Active(context.api) check + // would give: 'skyrimse' profile deployed while witcher3 is still the currently + // active game. The gate must follow the deployed profile, not "what's active". + ensureWsmToolRegisteredMock.mockClear().mockResolvedValue(false); + isWsmToolAcquiredMock.mockClear(); + scanWsmConflictsMock.mockClear(); + const { context, fireOnce, fireAsyncEvent } = fakeContext(WITCHER3_GAME_ID, { + profile1: { gameId: 'skyrimse' }, + }); + + main(context); + fireOnce(); + await fireAsyncEvent('did-deploy', 'profile1', undefined); + + expect(isWsmToolAcquiredMock).not.toHaveBeenCalled(); + expect(scanWsmConflictsMock).not.toHaveBeenCalled(); + }); + + it('does nothing when the profileId is unknown (no matching profile at all)', async () => { + ensureWsmToolRegisteredMock.mockClear().mockResolvedValue(false); + isWsmToolAcquiredMock.mockClear(); + scanWsmConflictsMock.mockClear(); + const { context, fireOnce, fireAsyncEvent } = fakeContext(WITCHER3_GAME_ID, {}); + + main(context); + fireOnce(); + await fireAsyncEvent('did-deploy', 'unknown-profile', undefined); + + expect(isWsmToolAcquiredMock).not.toHaveBeenCalled(); + expect(scanWsmConflictsMock).not.toHaveBeenCalled(); + }); + + it("scans a witcher3 deployment even if a different game has since become active (fixes the isWitcher3Active-at-handler-time race)", async () => { + // The scenario the profileId-based gate specifically exists to handle: the + // deployed profile really was witcher3, but by the time this async handler runs, + // the user already switched the *active* game elsewhere. A naive + // isWitcher3Active(context.api) check would wrongly skip this. + ensureWsmToolRegisteredMock.mockClear().mockResolvedValue(false); + isWsmToolAcquiredMock.mockClear().mockResolvedValue(true); + isModOrDependencyInstallActiveMock.mockClear().mockReturnValue(false); + const conflicts = [{ relativePath: 'a.ws' }]; + scanWsmConflictsMock.mockClear().mockResolvedValue(conflicts); + notifyConflictsIfChangedMock.mockClear(); + const { context, fireOnce, fireAsyncEvent, setActiveGame } = fakeContext(WITCHER3_GAME_ID, { + profile1: { gameId: WITCHER3_GAME_ID }, + }); + + main(context); + fireOnce(); + setActiveGame('skyrimse'); + await fireAsyncEvent('did-deploy', 'profile1', undefined); + + expect(scanWsmConflictsMock).toHaveBeenCalledTimes(1); + expect(notifyConflictsIfChangedMock).toHaveBeenCalledWith(context.api, conflicts); + }); + + it('skips scanning (without throwing) when no WSM tool has been acquired yet', async () => { + ensureWsmToolRegisteredMock.mockClear().mockResolvedValue(false); + isWsmToolAcquiredMock.mockClear().mockResolvedValue(false); + scanWsmConflictsMock.mockClear(); + notifyConflictsIfChangedMock.mockClear(); + const { context, fireOnce, fireAsyncEvent } = fakeContext(WITCHER3_GAME_ID, { + profile1: { gameId: WITCHER3_GAME_ID }, + }); + + main(context); + fireOnce(); + await fireAsyncEvent('did-deploy', 'profile1', undefined); + + expect(isWsmToolAcquiredMock).toHaveBeenCalledTimes(1); + expect(scanWsmConflictsMock).not.toHaveBeenCalled(); + expect(notifyConflictsIfChangedMock).not.toHaveBeenCalled(); + }); + + it('scans and notifies for a witcher3 deployment', async () => { + ensureWsmToolRegisteredMock.mockClear().mockResolvedValue(false); + isWsmToolAcquiredMock.mockClear().mockResolvedValue(true); + isModOrDependencyInstallActiveMock.mockClear().mockReturnValue(false); + 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 fireAsyncEvent('did-deploy', 'profile1', undefined); + + expect(scanWsmConflictsMock).toHaveBeenCalledTimes(1); + expect(notifyConflictsIfChangedMock).toHaveBeenCalledWith(context.api, conflicts); + }); + + // Fix for a real wasted-work case: notifyConflictsIfChanged would discard this + // scan's result anyway (it checks the same condition), so checking before ever + // spawning a WSM process avoids paying for a process spawn whose result can never + // be shown - concretely relevant during a dependency-install burst (e.g. installing + // a Collection triggers several deploy-per-mod cycles in a row). + it('does not spawn a scan at all while a mod/dependency install is in progress', async () => { + ensureWsmToolRegisteredMock.mockClear().mockResolvedValue(false); + isWsmToolAcquiredMock.mockClear().mockResolvedValue(true); + isModOrDependencyInstallActiveMock.mockClear().mockReturnValue(true); + scanWsmConflictsMock.mockClear(); + notifyConflictsIfChangedMock.mockClear(); + const { context, fireOnce, fireAsyncEvent } = fakeContext(WITCHER3_GAME_ID, { + profile1: { gameId: WITCHER3_GAME_ID }, + }); + + main(context); + fireOnce(); + await fireAsyncEvent('did-deploy', 'profile1', undefined); + + 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); + scanWsmConflictsMock.mockClear().mockRejectedValue(new Error('spawn failed')); + 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(notifyConflictsIfChangedMock).not.toHaveBeenCalled(); + }); + + // Regression test: isWsmToolAcquired is documented (conflictScan.ts) to propagate + // non-ENOENT filesystem errors rather than silently returning false. That call must + // stay inside checkForConflictsAfterDeploy's own try/catch, or a real EBUSY/EPERM + // (an antivirus scan, a concurrently running WSM process) would reject this + // onAsync('did-deploy', ...) handler's promise straight into Vortex's own + // emitAndAwait('did-deploy', ...) dispatch - exactly what onAsync's contract + // forbids. + it('resolves (never rejects) when isWsmToolAcquired throws a non-ENOENT filesystem error', async () => { + ensureWsmToolRegisteredMock.mockClear().mockResolvedValue(false); + isWsmToolAcquiredMock.mockClear().mockRejectedValue(Object.assign(new Error('EBUSY: resource busy or locked'), { code: 'EBUSY' })); + scanWsmConflictsMock.mockClear(); + 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(); + }); + + // Regression test: the try/catch wraps the entire handler body now, including the + // selectors.profileById(context.api.getState(), profileId) gate at the very top - + // not just the parts already known to be able to throw (isWsmToolAcquired, + // scanWsmConflicts). A synchronous throw from state lookup should be as unlikely + // as it is cheap to guard against, but onAsync's "never reject" contract applies to + // the whole handler. + it('resolves (never rejects) when reading state for the deployed-profile gate throws synchronously', async () => { + ensureWsmToolRegisteredMock.mockClear().mockResolvedValue(false); + isWsmToolAcquiredMock.mockClear(); + scanWsmConflictsMock.mockClear(); + notifyConflictsIfChangedMock.mockClear(); + const { context, fireOnce, fireAsyncEvent } = fakeContext(WITCHER3_GAME_ID, { + profile1: { gameId: WITCHER3_GAME_ID }, + }); + + // main()/fireOnce() run first, using the working getState for + // tryRegisterWsmTool's own unrelated isWitcher3Active check - only sabotage + // getState afterward, right before firing did-deploy, so this test isolates the + // did-deploy handler's own robustness rather than breaking context.once itself. + main(context); + fireOnce(); + context.api.getState = () => { + throw new Error('state store unavailable'); + }; + + await expect(fireAsyncEvent('did-deploy', 'profile1', undefined)).resolves.toBeUndefined(); + expect(isWsmToolAcquiredMock).not.toHaveBeenCalled(); + expect(scanWsmConflictsMock).not.toHaveBeenCalled(); + }); + }); }); diff --git a/vortex-extension/src/index.ts b/vortex-extension/src/index.ts index 773c254..6f79e43 100644 --- a/vortex-extension/src/index.ts +++ b/vortex-extension/src/index.ts @@ -1,5 +1,7 @@ -import { log, types } from 'vortex-api'; -import { isWitcher3Active } from './gating'; +import { log, selectors, types } from 'vortex-api'; +import { isWsmToolAcquired, scanWsmConflicts } from './conflictScan'; +import { isModOrDependencyInstallActive, notifyConflictsIfChanged } from './conflictNotifications'; +import { isWitcher3Active, WITCHER3_GAME_ID } from './gating'; import { ensureWsmToolRegistered } from './toolAcquisition'; /** @@ -23,12 +25,25 @@ import { ensureWsmToolRegistered } from './toolAcquisition'; * and without this, registering a previously-acquired tool would only ever happen if * Witcher 3 already happened to be active the moment Vortex loaded this extension. * - * Later units (conflict scanning, the merge panel, dashlets) each add their own - * `context.register*` calls inside the `context.once(...)` callback below, gated on - * `isWitcher3Active` (imported from `./gating`) - preferably via each registration API's - * own `condition` callback, so a live game-mode switch is honored without requiring a - * Vortex restart, the same way `tryRegisterWsmTool` below re-checks it on every - * `'gamemode-activated'` event rather than only once. + * This unit (conflict scanning) adds the second real registration: after every Vortex + * deployment for Witcher 3 finishes, scan for WSM script conflicts + * (`./conflictScan`'s `scanWsmConflicts`) and show a dashboard notification when the + * *unresolved* conflict set has changed since the last check this session + * (`./conflictNotifications`'s `notifyConflictsIfChanged`). Registered via + * `context.api.onAsync('did-deploy', ...)`, not `context.api.events.on(...)` - + * `did-deploy` is documented (`@nexusmods/vortex-api`'s own `docs/EVENTS.md`) as an + * *async* event (fired via `emitAndAwait`), and the package's own README example + * (`#### Event hooks`) and Vortex's own built-in `game-witcher3` extension + * (`extensions/games/game-witcher3/src/index.ts`: `context.api.onAsync("did-deploy", + * onDidDeploy(context.api))`) both register it that way - see this unit's PR + * description for the exact citations. + * + * Later units (the merge panel, dashlets) each add their own `context.register*` calls + * inside the `context.once(...)` callback below, gated on `isWitcher3Active` (imported + * from `./gating`) - preferably via each registration API's own `condition` callback, + * so a live game-mode switch is honored without requiring a Vortex restart, the same + * way `tryRegisterWsmTool` below re-checks it on every `'gamemode-activated'` event + * rather than only once. * * This extension must never call `context.registerGame('witcher3', ...)` - Vortex's own * built-in `game-witcher3` extension already owns that registration; this extension is a @@ -63,9 +78,83 @@ function main(context: types.IExtensionContext): boolean { }); } + // onAsync's own contract (@nexusmods/vortex-api's lib/api.d.ts doc comment on + // IExtensionApi.onAsync): "listeners should report all errors themselves, it is + // considered a bug if the listener returns a rejected promise" - so every path here + // must resolve, never reject, matching tryRegisterWsmTool's own catch-and-log + // (non-throwing) shape above. + // + // Gates on the *deployed* profile's own game (via did-deploy's own `profileId` + // argument, looked up with `selectors.profileById`), not `isWitcher3Active(context.api)` + // (whichever game happens to be active at the moment this async handler actually + // runs). Those are not the same thing: `did-deploy` fires once deployment completes, + // and by the time this handler's own turn comes up (after every other handler + // `emitAndAwait` is also waiting on), the user may already have switched to a + // different game. `isWitcher3Active` would then read the *new* active game and skip a + // real Witcher 3 deployment's scan entirely - a false negative that misses genuine + // conflicts, not just an ordering nicety. `did-deploy`'s own `IDeploymentManifest` + // exposes an optional `gameId`, but it's `gameId?: string` (not guaranteed present), + // so `profileId` -> `selectors.profileById(...).gameId` is the reliable, time-invariant + // signal used here instead. **Not a verbatim copy of `game-witcher3`'s own + // `eventHandlers.ts`/`util.ts` `validateProfile(profileId, state)`** - read directly: + // that function still ultimately keys off `selectors.activeProfile(state).gameId` (the + // same "what's active *now*" read this comment argues against), just with an added + // guard that `profileId` matches the currently active profile's own id. That's the + // right call for what `game-witcher3` uses it for (INI/load-order bookkeeping that + // only makes sense for the actively-displayed profile) but not for this extension's + // narrower job here - telling the user about conflicts from a deployment that + // genuinely happened is still correct even if they've since tabbed to another game, so + // this deliberately drops the "still the active profile" cross-check and trusts + // `profileId` alone, which is what actually removes the race rather than just + // narrowing its window. + async function checkForConflictsAfterDeploy(profileId: string): Promise { + // The entire body lives inside this one try/catch, including the deployed-game + // gate immediately below - onAsync's contract (quoted above) applies to the whole + // handler, not just the parts that were already known to be able to throw. A + // simple selectors.profileById lookup or api.getState() call is very unlikely to + // throw, but there's no reason to leave even a narrow, structurally-identical gap + // next to the one just closed for isWsmToolAcquired below. + try { + const deployedGameId = selectors.profileById(context.api.getState(), profileId)?.gameId; + if (deployedGameId !== WITCHER3_GAME_ID) { + return; + } + + if (!(await isWsmToolAcquired(context.api))) { + // Same normal, expected state tryRegisterWsmTool already logs at 'debug' above - + // no WSM binary acquired yet is not an error worth spawning a doomed process to + // discover. + log('debug', 'witcherscriptmerger-vortex: no acquired WSM tool - skipping post-deploy conflict scan'); + return; + } + + if (isModOrDependencyInstallActive(context.api)) { + // Purely an optimization, not a correctness requirement - notifyConflictsIfChanged + // (conflictNotifications.ts) checks this same condition again on whatever result + // would come back anyway, so this isn't the only thing standing between a real + // scan and a suppressed notification. What this pre-check buys is not spawning an + // entire WSM process in the first place when the answer is already known to be + // "discard this result" - worth avoiding specifically because a dependency-install + // burst (e.g. installing a Collection) can trigger several deploy-per-mod cycles + // in a row, each of which would otherwise spawn and tear down a WSM process for + // nothing. + log('debug', 'witcherscriptmerger-vortex: mod/dependency install activity in progress - skipping post-deploy conflict scan'); + return; + } + + const conflicts = await scanWsmConflicts(context.api); + notifyConflictsIfChanged(context.api, conflicts); + } catch (err) { + log('warn', 'witcherscriptmerger-vortex: post-deploy conflict scan failed', { + error: err instanceof Error ? err.message : String(err), + }); + } + } + context.once(() => { tryRegisterWsmTool(); context.api.events.on('gamemode-activated', tryRegisterWsmTool); + context.api.onAsync('did-deploy', checkForConflictsAfterDeploy); }); return true; diff --git a/vortex-extension/test/conflictScan.integration.test.ts b/vortex-extension/test/conflictScan.integration.test.ts new file mode 100644 index 0000000..a655a70 --- /dev/null +++ b/vortex-extension/test/conflictScan.integration.test.ts @@ -0,0 +1,211 @@ +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 { notifyConflictsIfChanged, resetConflictNotificationState, WSM_CONFLICTS_NOTIFICATION_ID } from '../src/conflictNotifications'; +import { WsmMcpClient } from '../src/mcpClient'; + +// Real, end-to-end integration test: spawns the actual, compiled WitcherScriptMerger +// Headless host's `mcp` verb against a scratch mods folder containing two real mods that +// both touch the same script file (a genuine conflict, per +// `FileIndex/ModFile.cs`'s `HasConflict => Mods.Count > 1`), runs a real `scan_conflicts` +// call the same way `conflictScan.ts`'s `scanWsmConflicts` does, and then feeds the real +// result into `notifyConflictsIfChanged` to confirm this unit's notification-trigger +// logic reacts to a real scan result correctly - the "scan round-trip" this unit's own +// instructions call for. Mirrors test/mcpClient.integration.test.ts's own scratch-config +// pattern exactly (same App.config shape, same build-if-missing beforeAll). + +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'); + +// See test/mcpClient.integration.test.ts's identical helper for why this is needed - +// os.tmpdir()/mkdtempSync embeds the current username on Windows, which can contain XML +// special characters. +function escapeXmlAttribute(value: string): string { + return value + .replace(/&/g, '&') + .replace(//g, '>') + .replace(/"/g, '"') + .replace(/'/g, '''); +} + +function buildScratchConfig(modsDirectory: string): string { + return ` + + + + + + + + + + + + + + + + + + +`; +} + +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 conflictScan ` + + `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-conflictscan-test-')); + fs.cpSync(HEADLESS_BUILD_DIR, scratchDir, { recursive: true }); + + const modsDir = path.join(scratchDir, 'Mods'); + // Two real mods both placing a same-named script under content\scripts - a genuine + // conflict per ModFile.HasConflict (Mods.Count > 1), the same shape + // ModFileIndex.BuildAsync scans for in a real Witcher 3 mods folder. + const mod1ScriptDir = path.join(modsDir, 'mod0001_First', 'content', 'scripts'); + const mod2ScriptDir = path.join(modsDir, 'mod0002_Second', 'content', 'scripts'); + fs.mkdirSync(mod1ScriptDir, { recursive: true }); + fs.mkdirSync(mod2ScriptDir, { recursive: true }); + fs.writeFileSync(path.join(mod1ScriptDir, 'conflicting.ws'), 'function First() {}\n', 'utf8'); + fs.writeFileSync(path.join(mod2ScriptDir, 'conflicting.ws'), 'function Second() {}\n', 'utf8'); + + fs.writeFileSync( + path.join(scratchDir, 'WitcherScriptMerger.Headless.dll.config'), + buildScratchConfig(modsDir), + 'utf8', + ); + + exePath = path.join(scratchDir, 'WitcherScriptMerger.Headless.exe'); +}, 300_000); + +afterAll(() => { + if (scratchDir) { + fs.rmSync(scratchDir, { recursive: true, force: true }); + } +}); + +describe('conflict scan round-trip (real WSM Headless process)', () => { + it('scan_conflicts reports the real conflicting file, and notifyConflictsIfChanged reacts to it', async () => { + const client = await WsmMcpClient.connect({ exePath }); + let conflicts; + try { + conflicts = await client.scanConflicts(); + } finally { + await client.close(); + } + + expect(conflicts).toHaveLength(1); + expect(conflicts[0].relativePath).toBe('conflicting.ws'); + expect(conflicts[0].alreadyResolved).toBe(false); + + resetConflictNotificationState(); + const sendNotification = createNotificationSpy(); + const dismissNotification = createDismissSpy(); + const fakeApi = { + getState: () => ({ session: { base: { activity: {} } } }), + sendNotification, + dismissNotification, + }; + + notifyConflictsIfChanged(fakeApi as never, conflicts); + + expect(sendNotification.calls).toHaveLength(1); + expect(sendNotification.calls[0].id).toBe(WSM_CONFLICTS_NOTIFICATION_ID); + expect(sendNotification.calls[0].allowSuppress).toBe(true); + + // Calling again with the identical real scan result must not re-notify. + notifyConflictsIfChanged(fakeApi as never, conflicts); + expect(sendNotification.calls).toHaveLength(1); + }, 30_000); + + it('scan_conflicts reports no conflicts against an empty mods folder', async () => { + const emptyScratchDir = fs.mkdtempSync(path.join(os.tmpdir(), 'wsm-conflictscan-empty-test-')); + try { + fs.cpSync(HEADLESS_BUILD_DIR, emptyScratchDir, { recursive: true }); + const modsDir = path.join(emptyScratchDir, 'Mods'); + fs.mkdirSync(modsDir, { recursive: true }); + fs.writeFileSync( + path.join(emptyScratchDir, 'WitcherScriptMerger.Headless.dll.config'), + buildScratchConfig(modsDir), + 'utf8', + ); + + const client = await WsmMcpClient.connect({ exePath: path.join(emptyScratchDir, 'WitcherScriptMerger.Headless.exe') }); + let conflicts; + try { + conflicts = await client.scanConflicts(); + } finally { + await client.close(); + } + + expect(conflicts).toEqual([]); + + resetConflictNotificationState(); + const sendNotification = createNotificationSpy(); + const fakeApi = { + getState: () => ({ session: { base: { activity: {} } } }), + sendNotification, + dismissNotification: createDismissSpy(), + }; + + notifyConflictsIfChanged(fakeApi as never, conflicts); + expect(sendNotification.calls).toHaveLength(0); + } finally { + fs.rmSync(emptyScratchDir, { recursive: true, force: true }); + } + }, 30_000); +}); + +// Tiny hand-rolled spies (no vi.fn() here - this file intentionally exercises the real +// mcpClient/conflictNotifications modules with no mocking whatsoever) that record calls +// for assertion. +function createNotificationSpy() { + const calls: Array<{ id?: string; type: string; message: string; allowSuppress?: boolean; actions?: unknown[] }> = []; + const fn = (notification: { id?: string; type: string; message: string; allowSuppress?: boolean; actions?: unknown[] }) => { + calls.push(notification); + return notification.id ?? 'generated-id'; + }; + return Object.assign(fn, { calls }); +} + +function createDismissSpy() { + const calls: string[] = []; + const fn = (id: string) => { + calls.push(id); + }; + return Object.assign(fn, { calls }); +} diff --git a/vortex-extension/test/testUtils/vortexApiStub.ts b/vortex-extension/test/testUtils/vortexApiStub.ts index 8e150a7..9d9ef02 100644 --- a/vortex-extension/test/testUtils/vortexApiStub.ts +++ b/vortex-extension/test/testUtils/vortexApiStub.ts @@ -18,13 +18,23 @@ import * as fs from 'fs'; // same simplified-fake-state philosophy - deliberately keyed by gameId (unlike a // same-shape "whichever game is active" selector) since `toolAcquisition.ts` always // registers its tool under a fixed game id and needs that game's own discovery -// specifically, not whatever happens to be active when it runs. +// specifically, not whatever happens to be active when it runs. `profileById` (real +// signature: `(state: IState, profileId: string) => IProfile`, per +// @nexusmods/vortex-api's own `lib/api.d.ts`) added alongside `index.ts`'s +// `checkForConflictsAfterDeploy` for the same reason as `discoveryByGame` - resolving +// `did-deploy`'s own `profileId` argument to the *deployed* profile's `gameId` is a +// different question from "what's active right now" (`activeGameId`), and conflating +// the two was itself the bug this lookup exists to avoid - see index.ts's own comment. export const selectors = { activeGameId: (state: { activeGameId?: string }): string | undefined => state?.activeGameId, discoveryByGame: ( state: { discoveryByGame?: Record }, gameId: string, ): { path?: string } | undefined => state?.discoveryByGame?.[gameId], + profileById: ( + state: { profiles?: Record }, + profileId: string, + ): { gameId?: string } | undefined => state?.profiles?.[profileId], }; // `actions.addDiscoveredTool` needs a real (if simplified) implementation, not just a