From fb1452f636b8bc2f01776b5457492ec47ceedf7f Mon Sep 17 00:00:00 2001 From: Chris Knight Date: Mon, 10 Aug 2026 15:39:17 -0400 Subject: [PATCH 1/3] Add post-deploy WSM conflict scanning and a dashboard notification (Unit G) After a Vortex deployment finishes for Witcher 3, spawn a short-lived WsmMcpClient, run scan_conflicts, and show a dashboard notification when the set of unresolved conflicts has changed since the last check this session. Registered via context.api.onAsync('did-deploy', ...), matching the real, verified event contract (async, fired via emitAndAwait) and the built-in game-witcher3 extension's own onDidDeploy wiring - not context.api.events.on, contrary to a plausible-looking guess. Uses a notification id distinct from game-witcher3's own "witcher3-merge" so both extensions can coexist without colliding on the same slot. Skips the notification while Vortex reports mod-install or dependency-install activity in progress, per state.session.base.activity. Found and fixed a real bug during development: @nexusmods/vortex-api's published types describe that field as {[group: string]: string}, but the actual Vortex reducer stores a string[] per group, and never deletes the group key on stopActivity - it leaves an empty array behind. A naive truthiness check would have seen that leftover [] as "still active" and permanently suppressed every future notification after the first install ever ran. Fixed with a shape-tolerant activityEntries() helper, regression-tested against the empty-array case. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01GXAuGMLB44T5Zv5o5ZzKah --- .../src/conflictNotifications.test.ts | 177 +++++++++++++++ vortex-extension/src/conflictNotifications.ts | 175 +++++++++++++++ vortex-extension/src/conflictScan.test.ts | 106 +++++++++ vortex-extension/src/conflictScan.ts | 69 ++++++ vortex-extension/src/index.test.ts | 101 ++++++++- vortex-extension/src/index.ts | 56 ++++- .../test/conflictScan.integration.test.ts | 211 ++++++++++++++++++ 7 files changed, 884 insertions(+), 11 deletions(-) create mode 100644 vortex-extension/src/conflictNotifications.test.ts create mode 100644 vortex-extension/src/conflictNotifications.ts create mode 100644 vortex-extension/src/conflictScan.test.ts create mode 100644 vortex-extension/src/conflictScan.ts create mode 100644 vortex-extension/test/conflictScan.integration.test.ts diff --git a/vortex-extension/src/conflictNotifications.test.ts b/vortex-extension/src/conflictNotifications.test.ts new file mode 100644 index 0000000..518cc31 --- /dev/null +++ b/vortex-extension/src/conflictNotifications.test.ts @@ -0,0 +1,177 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest'; +import { ScanConflictsResult } from './mcpClient'; +import { computeConflictSignature, notifyConflictsIfChanged, resetConflictNotificationState, WSM_CONFLICTS_NOTIFICATION_ID } from './conflictNotifications'; + +function conflict(relativePath: string, alreadyResolved = false): ScanConflictsResult[number] { + return { + relativePath, + category: 'Script', + mods: [], + 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')])); + }); +}); + +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('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); + }); + + 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); + }); + }); + + 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..8a49916 --- /dev/null +++ b/vortex-extension/src/conflictNotifications.ts @@ -0,0 +1,175 @@ +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) - a sorted, `|`-joined list of + * relative paths is enough: WSM's own `scan_conflicts` already de-duplicates by + * relative path (`FileIndex/ModFileIndex.cs`'s `GetModFilesFromPaths` folds multiple + * mods touching the same file into one `ModFile` entry with a `Mods` list), so no two + * entries in a single scan result can share a `relativePath`. + */ +export function computeConflictSignature(conflicts: ScanConflictsResult): string { + return conflicts + .map((c) => c.relativePath) + .sort() + .join('|'); +} + +/** + * 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`. + * + * 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. + */ +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. Exported reset + * hook is for test isolation only - no production caller should ever need it. */ +let lastNotifiedSignature: string | undefined; + +export function resetConflictNotificationState(): void { + lastNotifiedSignature = undefined; +} + +/** + * 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. + */ +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; + } + lastNotifiedSignature = signature; + + if (unresolved.length === 0) { + api.dismissNotification?.(WSM_CONFLICTS_NOTIFICATION_ID); + return; + } + + api.sendNotification?.({ + id: WSM_CONFLICTS_NOTIFICATION_ID, + type: 'warning', + message: `WitcherScriptMerger: ${unresolved.length} unresolved script conflict${unresolved.length === 1 ? '' : 's'} found`, + allowSuppress: true, + actions: [], + }); +} diff --git a/vortex-extension/src/conflictScan.test.ts b/vortex-extension/src/conflictScan.test.ts new file mode 100644 index 0000000..491a1f1 --- /dev/null +++ b/vortex-extension/src/conflictScan.test.ts @@ -0,0 +1,106 @@ +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); + }); +}); + +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 }; + 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); + }); + + 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'); + }); +}); diff --git a/vortex-extension/src/conflictScan.ts b/vortex-extension/src/conflictScan.ts new file mode 100644 index 0000000..fc87de7 --- /dev/null +++ b/vortex-extension/src/conflictScan.ts @@ -0,0 +1,69 @@ +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 `isWitcher3Active(api)` gating - see + * `gating.ts`'s own doc comment for why every feature this extension registers gates on + * that check, and `index.ts` for where this module's own caller does so. + */ + +/** 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); +} + +/** + * Cheap, local, network-free existence check for the acquired WSM exe - mirrors + * `toolAcquisition.ts`'s own private `pathExists` helper (not exported, so not reused + * directly here) 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`. + */ +export async function isWsmToolAcquired(api: types.IExtensionApi): Promise { + try { + await fs.promises.access(getWsmExePath(api)); + return true; + } catch { + return false; + } +} + +/** + * 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. + */ +export async function scanWsmConflicts(api: types.IExtensionApi): Promise { + const gameDirectory = selectors.discoveryByGame(api.getState(), WITCHER3_GAME_ID)?.path; + const env = mergeWithProcessEnv(buildWsmEnv({ gameDirectory })); + + const client = await WsmMcpClient.connect({ exePath: getWsmExePath(api), env }); + try { + return await client.scanConflicts(); + } finally { + await client.close(); + } +} diff --git a/vortex-extension/src/index.test.ts b/vortex-extension/src/index.test.ts index 84f1454..0e8934f 100644 --- a/vortex-extension/src/index.test.ts +++ b/vortex-extension/src/index.test.ts @@ -2,26 +2,40 @@ 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 } = vi.hoisted(() => ({ ensureWsmToolRegisteredMock: vi.fn(), + isWsmToolAcquiredMock: vi.fn(), + scanWsmConflictsMock: vi.fn(), + notifyConflictsIfChangedMock: vi.fn(), })); vi.mock('./toolAcquisition', () => ({ ensureWsmToolRegistered: ensureWsmToolRegisteredMock, })); +vi.mock('./conflictScan', () => ({ + isWsmToolAcquired: isWsmToolAcquiredMock, + scanWsmConflicts: scanWsmConflictsMock, +})); + +vi.mock('./conflictNotifications', () => ({ + notifyConflictsIfChanged: notifyConflictsIfChangedMock, +})); + 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. */ + * 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. */ function fakeContext(initialActiveGameId: string | undefined) { const state = { activeGameId: initialActiveGameId }; let onceCallback: (() => void) | undefined; const eventListeners = new Map void>>(); + const asyncListeners = new Map Promise>(); const context = { once: (callback: () => void) => { @@ -36,6 +50,9 @@ function fakeContext(initialActiveGameId: string | undefined) { eventListeners.set(eventName, listeners); }, }, + onAsync: (eventName: string, listener: (...args: unknown[]) => Promise) => { + asyncListeners.set(eventName, listener); + }, }, }; @@ -43,6 +60,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 +133,77 @@ 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')).toBeInstanceOf(Promise); + }); + + it('does nothing when witcher3 is not the active game', async () => { + ensureWsmToolRegisteredMock.mockClear().mockResolvedValue(false); + isWsmToolAcquiredMock.mockClear(); + scanWsmConflictsMock.mockClear(); + const { context, fireOnce, fireAsyncEvent } = fakeContext('skyrimse'); + + main(context); + fireOnce(); + await fireAsyncEvent('did-deploy', 'profile1', undefined); + + expect(isWsmToolAcquiredMock).not.toHaveBeenCalled(); + expect(scanWsmConflictsMock).not.toHaveBeenCalled(); + }); + + 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); + + 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 when witcher3 is active and a WSM tool is acquired', async () => { + ensureWsmToolRegisteredMock.mockClear().mockResolvedValue(false); + isWsmToolAcquiredMock.mockClear().mockResolvedValue(true); + const conflicts = [{ relativePath: 'a.ws' }]; + scanWsmConflictsMock.mockClear().mockResolvedValue(conflicts); + notifyConflictsIfChangedMock.mockClear(); + const { context, fireOnce, fireAsyncEvent } = fakeContext(WITCHER3_GAME_ID); + + main(context); + fireOnce(); + await fireAsyncEvent('did-deploy', 'profile1', undefined); + + expect(scanWsmConflictsMock).toHaveBeenCalledTimes(1); + expect(notifyConflictsIfChangedMock).toHaveBeenCalledWith(context.api, conflicts); + }); + + it('resolves (never rejects) when scanWsmConflicts throws - onAsync listeners must report their own errors', async () => { + ensureWsmToolRegisteredMock.mockClear().mockResolvedValue(false); + isWsmToolAcquiredMock.mockClear().mockResolvedValue(true); + scanWsmConflictsMock.mockClear().mockRejectedValue(new Error('spawn failed')); + notifyConflictsIfChangedMock.mockClear(); + const { context, fireOnce, fireAsyncEvent } = fakeContext(WITCHER3_GAME_ID); + + main(context); + fireOnce(); + + await expect(fireAsyncEvent('did-deploy', 'profile1', undefined)).resolves.toBeUndefined(); + expect(notifyConflictsIfChangedMock).not.toHaveBeenCalled(); + }); + }); }); diff --git a/vortex-extension/src/index.ts b/vortex-extension/src/index.ts index 773c254..17a6a80 100644 --- a/vortex-extension/src/index.ts +++ b/vortex-extension/src/index.ts @@ -1,4 +1,6 @@ import { log, types } from 'vortex-api'; +import { isWsmToolAcquired, scanWsmConflicts } from './conflictScan'; +import { notifyConflictsIfChanged } from './conflictNotifications'; import { isWitcher3Active } 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,38 @@ 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. + async function checkForConflictsAfterDeploy(): Promise { + if (!isWitcher3Active(context.api)) { + 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; + } + + try { + const conflicts = await scanWsmConflicts(context.api); + notifyConflictsIfChanged(context.api, conflicts); + } catch (err) { + log('warn', 'witcherscriptmerger-vortex: failed to scan for script conflicts after deployment', { + 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 }); +} From 6c64cdf9973487b3483938352eaa819e26a26260 Mon Sep 17 00:00:00 2001 From: Chris Knight Date: Mon, 10 Aug 2026 15:56:33 -0400 Subject: [PATCH 2/3] Fix 6 review findings in conflict scanning/notification (Unit G follow-up) A code-review pass surfaced 6 issues; all verified against the real code and fixed: 1. isWsmToolAcquired swallowed every fs.access error, not just ENOENT - a locked file (AV scan, a running WSM process) would silently look identical to "nothing installed". Now mirrors toolAcquisition.ts's pathExists exactly: re-throws anything that isn't ENOENT. 2. notifyConflictsIfChanged committed lastNotifiedSignature before the sendNotification/dismissNotification call actually succeeded. A failure there would permanently suppress the real notification for that conflict set, since the next check would see the same signature and skip. Now wrapped in try/catch, with the signature only committed on success, and the error swallowed (never thrown) per onAsync's contract. 3. scanWsmConflicts had no in-flight coalescing, unlike toolAcquisition.ts's inFlightAcquisitions. Overlapping did-deploy events could run two concurrent WSM processes and resolve out of order, feeding a stale result to notifyConflictsIfChanged after a fresher one already landed. Added the same single-slot coalescing pattern. 4. The post-deploy scan used mcpClient.ts's full 30s-per-request default, but this path runs inside Vortex's own emitAndAwait('did-deploy', ...) await window - a slow WSM process would extend Vortex's own reported deployment-completion time. Added a tighter 15s requestTimeoutMs specific to this call site (mcpClient.ts itself untouched - this uses its existing public per-call override). 5. checkForConflictsAfterDeploy gated on isWitcher3Active(context.api) - whichever game is active when the async handler happens to run - rather than the deployed profile's own game (did-deploy's own profileId argument). A user switching games between did-deploy firing and this handler's turn coming up could cause a real Witcher 3 deployment's scan to be silently skipped. Now resolves profileId's own gameId via selectors.profileById and gates on that instead - time-invariant, so immune to this race. (Verified against game-witcher3's own validateProfile as precedent for the general profileId-driven approach, but it is not a verbatim copy: that function still ultimately keys off the active profile, with an added same-profile guard - not needed for this extension's narrower job of surfacing conflicts from a deployment that genuinely happened.) 6. lastNotifiedSignature initialized to undefined instead of '', so the very first post-deploy check of a session with zero conflicts always called dismissNotification for an id that was never sent. Now initialized to '', matching computeConflictSignature([])'s own value. Also fixes a regression introduced while addressing #1: isWsmToolAcquired's new non-ENOENT throw must stay inside checkForConflictsAfterDeploy's try/catch, not ahead of it, or it would reject the onAsync('did-deploy', ...) handler's promise straight into Vortex's own dispatch. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01GXAuGMLB44T5Zv5o5ZzKah --- .../src/conflictNotifications.test.ts | 53 ++++++++++ vortex-extension/src/conflictNotifications.ts | 58 +++++++--- vortex-extension/src/conflictScan.test.ts | 76 ++++++++++++- vortex-extension/src/conflictScan.ts | 83 +++++++++++++-- vortex-extension/src/index.test.ts | 100 ++++++++++++++++-- vortex-extension/src/index.ts | 57 +++++++--- .../test/testUtils/vortexApiStub.ts | 12 ++- 7 files changed, 392 insertions(+), 47 deletions(-) diff --git a/vortex-extension/src/conflictNotifications.test.ts b/vortex-extension/src/conflictNotifications.test.ts index 518cc31..0e4852b 100644 --- a/vortex-extension/src/conflictNotifications.test.ts +++ b/vortex-extension/src/conflictNotifications.test.ts @@ -104,6 +104,59 @@ describe('notifyConflictsIfChanged', () => { 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'] }); diff --git a/vortex-extension/src/conflictNotifications.ts b/vortex-extension/src/conflictNotifications.ts index 8a49916..bef5ff1 100644 --- a/vortex-extension/src/conflictNotifications.ts +++ b/vortex-extension/src/conflictNotifications.ts @@ -115,12 +115,18 @@ function isModOrDependencyInstallActive(api: types.IExtensionApi): boolean { /** 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. Exported reset - * hook is for test isolation only - no production caller should ever need it. */ -let lastNotifiedSignature: string | undefined; + * 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 = undefined; + lastNotifiedSignature = ''; } /** @@ -145,6 +151,19 @@ export function resetConflictNotificationState(): void { * 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)) { @@ -158,18 +177,29 @@ export function notifyConflictsIfChanged(api: types.IExtensionApi, conflicts: Sc if (signature === lastNotifiedSignature) { return; } - lastNotifiedSignature = signature; - if (unresolved.length === 0) { - api.dismissNotification?.(WSM_CONFLICTS_NOTIFICATION_ID); + 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; } - api.sendNotification?.({ - id: WSM_CONFLICTS_NOTIFICATION_ID, - type: 'warning', - message: `WitcherScriptMerger: ${unresolved.length} unresolved script conflict${unresolved.length === 1 ? '' : 's'} found`, - allowSuppress: true, - actions: [], - }); + lastNotifiedSignature = signature; } diff --git a/vortex-extension/src/conflictScan.test.ts b/vortex-extension/src/conflictScan.test.ts index 491a1f1..2d61abf 100644 --- a/vortex-extension/src/conflictScan.test.ts +++ b/vortex-extension/src/conflictScan.test.ts @@ -60,6 +60,22 @@ describe('isWsmToolAcquired', () => { 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', () => { @@ -78,13 +94,31 @@ describe('scanWsmConflicts', () => { expect(result).toEqual([{ relativePath: 'foo.ws' }]); expect(connectMock).toHaveBeenCalledTimes(1); - const connectArgs = connectMock.mock.calls[0][0] as { exePath: string; env: Record }; + 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')); @@ -103,4 +137,44 @@ describe('scanWsmConflicts', () => { 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 index fc87de7..4272ce5 100644 --- a/vortex-extension/src/conflictScan.ts +++ b/vortex-extension/src/conflictScan.ts @@ -27,23 +27,69 @@ 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 not reused - * directly here) 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`. + * `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 { - return false; + } 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. @@ -55,12 +101,33 @@ export async function isWsmToolAcquired(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 })); - const client = await WsmMcpClient.connect({ exePath: getWsmExePath(api), env }); + const client = await WsmMcpClient.connect({ + exePath: getWsmExePath(api), + env, + requestTimeoutMs: POST_DEPLOY_SCAN_TIMEOUT_MS, + }); try { return await client.scanConflicts(); } finally { diff --git a/vortex-extension/src/index.test.ts b/vortex-extension/src/index.test.ts index 0e8934f..24be48a 100644 --- a/vortex-extension/src/index.test.ts +++ b/vortex-extension/src/index.test.ts @@ -30,9 +30,17 @@ 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/onAsync), 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 }; + * 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>(); @@ -143,14 +151,19 @@ describe('main (index.ts)', () => { fireOnce(); // No handler registered for a plain events.on('did-deploy', ...) - only onAsync. - expect(fireAsyncEvent('did-deploy')).toBeInstanceOf(Promise); + expect(fireAsyncEvent('did-deploy', 'profile1', undefined)).toBeInstanceOf(Promise); }); - it('does nothing when witcher3 is not the active game', async () => { + 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('skyrimse'); + const { context, fireOnce, fireAsyncEvent } = fakeContext(WITCHER3_GAME_ID, { + profile1: { gameId: 'skyrimse' }, + }); main(context); fireOnce(); @@ -160,12 +173,51 @@ describe('main (index.ts)', () => { 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); + 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); + const { context, fireOnce, fireAsyncEvent } = fakeContext(WITCHER3_GAME_ID, { + profile1: { gameId: WITCHER3_GAME_ID }, + }); main(context); fireOnce(); @@ -176,13 +228,15 @@ describe('main (index.ts)', () => { expect(notifyConflictsIfChangedMock).not.toHaveBeenCalled(); }); - it('scans and notifies when witcher3 is active and a WSM tool is acquired', async () => { + it('scans and notifies for a witcher3 deployment', async () => { ensureWsmToolRegisteredMock.mockClear().mockResolvedValue(false); isWsmToolAcquiredMock.mockClear().mockResolvedValue(true); const conflicts = [{ relativePath: 'a.ws' }]; scanWsmConflictsMock.mockClear().mockResolvedValue(conflicts); notifyConflictsIfChangedMock.mockClear(); - const { context, fireOnce, fireAsyncEvent } = fakeContext(WITCHER3_GAME_ID); + const { context, fireOnce, fireAsyncEvent } = fakeContext(WITCHER3_GAME_ID, { + profile1: { gameId: WITCHER3_GAME_ID }, + }); main(context); fireOnce(); @@ -197,12 +251,38 @@ describe('main (index.ts)', () => { isWsmToolAcquiredMock.mockClear().mockResolvedValue(true); scanWsmConflictsMock.mockClear().mockRejectedValue(new Error('spawn failed')); notifyConflictsIfChangedMock.mockClear(); - const { context, fireOnce, fireAsyncEvent } = fakeContext(WITCHER3_GAME_ID); + 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(); }); }); diff --git a/vortex-extension/src/index.ts b/vortex-extension/src/index.ts index 17a6a80..98ccbf7 100644 --- a/vortex-extension/src/index.ts +++ b/vortex-extension/src/index.ts @@ -1,7 +1,7 @@ -import { log, types } from 'vortex-api'; +import { log, selectors, types } from 'vortex-api'; import { isWsmToolAcquired, scanWsmConflicts } from './conflictScan'; import { notifyConflictsIfChanged } from './conflictNotifications'; -import { isWitcher3Active } from './gating'; +import { isWitcher3Active, WITCHER3_GAME_ID } from './gating'; import { ensureWsmToolRegistered } from './toolAcquisition'; /** @@ -83,24 +83,55 @@ function main(context: types.IExtensionContext): boolean { // 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. - async function checkForConflictsAfterDeploy(): Promise { - if (!isWitcher3Active(context.api)) { - 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'); + // + // 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 { + const deployedGameId = selectors.profileById(context.api.getState(), profileId)?.gameId; + if (deployedGameId !== WITCHER3_GAME_ID) { return; } + // isWsmToolAcquired is inside this try, not gating ahead of it: it can now throw + // for a non-ENOENT filesystem error (a locked/permission-denied exe path, e.g. an + // antivirus scan or a concurrently running WSM process - see conflictScan.ts's own + // doc comment), and the onAsync contract this handler must honor forbids a + // rejected promise reaching Vortex's own did-deploy dispatch, exactly like a + // failure from scanWsmConflicts/notifyConflictsIfChanged below. try { + 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; + } + const conflicts = await scanWsmConflicts(context.api); notifyConflictsIfChanged(context.api, conflicts); } catch (err) { - log('warn', 'witcherscriptmerger-vortex: failed to scan for script conflicts after deployment', { + log('warn', 'witcherscriptmerger-vortex: post-deploy conflict scan failed', { error: err instanceof Error ? err.message : String(err), }); } 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 From ab2e878c16eaf674737d2d42dffde73742130338 Mon Sep 17 00:00:00 2001 From: Chris Knight Date: Mon, 10 Aug 2026 16:07:12 -0400 Subject: [PATCH 3/3] Address the original code-review agent's delayed findings (Unit G follow-up 2) The code-review skill invocation from earlier in this unit's work finally delivered its results (delayed by a background-agent naming collision). It had reviewed the already-fixed commit and confirmed those 6 fixes, but surfaced 7 further findings. Verified each against the real code; fixed 6 of them: - conflictScan.ts's and gating.ts's own doc comments still described conflict scanning as gated on isWitcher3Active(api) - stale since the profileId-based gating fix. Corrected both to describe the actual, deliberate exception and why. - computeConflictSignature keyed only on relativePath, so a conflict whose contributing mod set changed (e.g. a third mod starts touching an already-conflicting file) produced the same signature as before and was silently never re-notified. Now includes each conflict's sorted mod names in its signature entry. - checkForConflictsAfterDeploy spawned a full WSM process unconditionally whenever a tool was acquired, checking install-activity only afterward (inside notifyConflictsIfChanged, which would then discard the result). Exported isModOrDependencyInstallActive from conflictNotifications.ts and added a pre-check before scanning, avoiding wasted process spawns during e.g. a Collection install's deploy-per-mod bursts. The later check inside notifyConflictsIfChanged stays as defense-in-depth, since activity can start during the scan itself. - The outer selectors.profileById(...) gate sat outside checkForConflictsAfterDeploy's try/catch - a narrower version of the same class of bug just fixed for isWsmToolAcquired. Widened the try/catch to cover the entire handler body. Two findings were verified as real but left as documented, not code changes: - A TOCTOU gap between isWsmToolAcquired's check and scanWsmConflicts's own connect() - already safely contained by the existing try/catch (a caught warning, not a crash), so just documented rather than adding synchronization machinery for an already-handled race. - The dotnet build race between integration test files - already disclosed in this PR's own description as a known, pre-existing test-infra property; not restructured here since sibling units may add their own integration test files with the same pattern. 89 unit tests (up from 82), 95 total with integration. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01GXAuGMLB44T5Zv5o5ZzKah --- .../src/conflictNotifications.test.ts | 54 +++++++++++++-- vortex-extension/src/conflictNotifications.ts | 37 ++++++++--- vortex-extension/src/conflictScan.ts | 25 ++++++- vortex-extension/src/gating.ts | 14 +++- vortex-extension/src/index.test.ts | 66 ++++++++++++++++++- vortex-extension/src/index.ts | 38 +++++++---- 6 files changed, 204 insertions(+), 30 deletions(-) diff --git a/vortex-extension/src/conflictNotifications.test.ts b/vortex-extension/src/conflictNotifications.test.ts index 0e4852b..685b299 100644 --- a/vortex-extension/src/conflictNotifications.test.ts +++ b/vortex-extension/src/conflictNotifications.test.ts @@ -1,12 +1,18 @@ import { beforeEach, describe, expect, it, vi } from 'vitest'; import { ScanConflictsResult } from './mcpClient'; -import { computeConflictSignature, notifyConflictsIfChanged, resetConflictNotificationState, WSM_CONFLICTS_NOTIFICATION_ID } from './conflictNotifications'; - -function conflict(relativePath: string, alreadyResolved = false): ScanConflictsResult[number] { +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: [], + mods: modNames.map((name) => ({ name, hash: '', isOutdated: false })), defaultOrder: [], alreadyResolved, }; @@ -40,6 +46,22 @@ describe('computeConflictSignature', () => { 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', () => { @@ -83,6 +105,15 @@ describe('notifyConflictsIfChanged', () => { 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(); @@ -210,6 +241,21 @@ describe('notifyConflictsIfChanged', () => { }); }); + // 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' }); diff --git a/vortex-extension/src/conflictNotifications.ts b/vortex-extension/src/conflictNotifications.ts index bef5ff1..79d0164 100644 --- a/vortex-extension/src/conflictNotifications.ts +++ b/vortex-extension/src/conflictNotifications.ts @@ -17,17 +17,29 @@ export const WSM_CONFLICTS_NOTIFICATION_ID = 'witcherscriptmerger-vortex-conflic /** * 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) - a sorted, `|`-joined list of - * relative paths is enough: WSM's own `scan_conflicts` already de-duplicates by - * relative path (`FileIndex/ModFileIndex.cs`'s `GetModFilesFromPaths` folds multiple - * mods touching the same file into one `ModFile` entry with a `Mods` list), so no two - * entries in a single scan result can share a `relativePath`. + * 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) + .map((c) => `${c.relativePath}:${[...c.mods.map((m) => m.name)].sort().join('|')}`) .sort() - .join('|'); + .join('\n'); } /** @@ -64,6 +76,15 @@ function activityEntries(value: unknown): string[] { * 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 @@ -98,7 +119,7 @@ function activityEntries(value: unknown): string[] { * own task explicitly names "mod-install" alongside "dependency-install", but worth * flagging as the less battle-tested of the two. */ -function isModOrDependencyInstallActive(api: types.IExtensionApi): boolean { +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 diff --git a/vortex-extension/src/conflictScan.ts b/vortex-extension/src/conflictScan.ts index 4272ce5..7cdd5a1 100644 --- a/vortex-extension/src/conflictScan.ts +++ b/vortex-extension/src/conflictScan.ts @@ -15,9 +15,17 @@ import { buildWsmEnv, mergeWithProcessEnv } from './wsmEnv'; * 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 `isWitcher3Active(api)` gating - see - * `gating.ts`'s own doc comment for why every feature this extension registers gates on - * that check, and `index.ts` for where this module's own caller does so. + * 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 @@ -123,6 +131,17 @@ 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, 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 24be48a..0c6f80b 100644 --- a/vortex-extension/src/index.test.ts +++ b/vortex-extension/src/index.test.ts @@ -5,11 +5,18 @@ import { describe, expect, it, vi } from 'vitest'; // 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 } = vi.hoisted(() => ({ +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', () => ({ @@ -23,6 +30,7 @@ vi.mock('./conflictScan', () => ({ vi.mock('./conflictNotifications', () => ({ notifyConflictsIfChanged: notifyConflictsIfChangedMock, + isModOrDependencyInstallActive: isModOrDependencyInstallActiveMock, })); import main from './index'; @@ -194,6 +202,7 @@ describe('main (index.ts)', () => { // 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(); @@ -231,6 +240,7 @@ describe('main (index.ts)', () => { 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(); @@ -246,9 +256,33 @@ describe('main (index.ts)', () => { 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, { @@ -285,5 +319,35 @@ describe('main (index.ts)', () => { 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 98ccbf7..6f79e43 100644 --- a/vortex-extension/src/index.ts +++ b/vortex-extension/src/index.ts @@ -1,6 +1,6 @@ import { log, selectors, types } from 'vortex-api'; import { isWsmToolAcquired, scanWsmConflicts } from './conflictScan'; -import { notifyConflictsIfChanged } from './conflictNotifications'; +import { isModOrDependencyInstallActive, notifyConflictsIfChanged } from './conflictNotifications'; import { isWitcher3Active, WITCHER3_GAME_ID } from './gating'; import { ensureWsmToolRegistered } from './toolAcquisition'; @@ -108,18 +108,18 @@ function main(context: types.IExtensionContext): boolean { // `profileId` alone, which is what actually removes the race rather than just // narrowing its window. async function checkForConflictsAfterDeploy(profileId: string): Promise { - const deployedGameId = selectors.profileById(context.api.getState(), profileId)?.gameId; - if (deployedGameId !== WITCHER3_GAME_ID) { - return; - } - - // isWsmToolAcquired is inside this try, not gating ahead of it: it can now throw - // for a non-ENOENT filesystem error (a locked/permission-denied exe path, e.g. an - // antivirus scan or a concurrently running WSM process - see conflictScan.ts's own - // doc comment), and the onAsync contract this handler must honor forbids a - // rejected promise reaching Vortex's own did-deploy dispatch, exactly like a - // failure from scanWsmConflicts/notifyConflictsIfChanged below. + // 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 @@ -128,6 +128,20 @@ function main(context: types.IExtensionContext): boolean { 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) {