From 051adfa765a378cd47e8a6d617d531d9755496e4 Mon Sep 17 00:00:00 2001 From: Pixnop <77785313+Pixnop@users.noreply.github.com> Date: Fri, 28 Aug 2026 23:56:25 +0200 Subject: [PATCH 1/6] ci(test): run the test job on windows too Extend the test job to the same os matrix the build job already uses, so the win32 branches (pathsHandlersWin32, atomic-write rename semantics, symlink cases that skipIf on win32) run for real instead of only in their skipped form. Refs #267 --- .github/workflows/ci.yml | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index fbf08f60..8e9b085b 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -33,7 +33,11 @@ jobs: - run: npm run format:check test: - runs-on: ubuntu-latest + strategy: + fail-fast: false + matrix: + os: [ubuntu-latest, windows-latest] + runs-on: ${{ matrix.os }} steps: - uses: actions/checkout@v4 - uses: actions/setup-node@v4 From 75b9862fb1987344a4fed11bf3526c463d573d6a Mon Sep 17 00:00:00 2001 From: Pixnop <77785313+Pixnop@users.noreply.github.com> Date: Sat, 29 Aug 2026 00:05:26 +0200 Subject: [PATCH 2/6] test: fix windows-only environmental failures the new job surfaced The first real run of the test job on windows-latest found a handful of tests that fail purely because of platform differences the tests never accounted for, not bugs in the code they cover: - accountLoginFlow.test.ts read the handler source without normalizing line endings, so its "\n"-based slice landed in the wrong place once git checked the file out with CRLF. - accountStore.test.ts, configHandlers.test.ts, modsHandlers.test.ts and permissions.test.ts all read a POSIX mode bit (0o600, 0o755, and friends) back off a real file after chmod. NTFS has no such bits; chmod there only toggles the read-only attribute. These now skipIf(win32), the same pattern backgroundHandlers.test.ts and pathsHandlers.test.ts already use for symlink-only cases. - pathsHandlers.test.ts had one RUN_INSTALLER test whose own header comment already documented it as covering "the not-windows arm, real unstubbed behavior on the Linux host these tests run on." On an actual windows host that arm can't fire, so it now skips there too. A separate, larger set of gameHandlers.test.ts and extraction.test.ts failures is left as is; those need a closer look before deciding whether they're more test gaps or something the launcher itself gets wrong on Windows. --- tests/ipc/accountLoginFlow.test.ts | 5 ++++- tests/ipc/accountStore.test.ts | 4 +++- tests/ipc/configHandlers.test.ts | 4 +++- tests/ipc/modsHandlers.test.ts | 5 ++++- tests/ipc/pathsHandlers.test.ts | 5 ++++- tests/ipc/permissions.test.ts | 16 ++++++++++------ 6 files changed, 28 insertions(+), 11 deletions(-) diff --git a/tests/ipc/accountLoginFlow.test.ts b/tests/ipc/accountLoginFlow.test.ts index 9227952f..f56fbf6a 100644 --- a/tests/ipc/accountLoginFlow.test.ts +++ b/tests/ipc/accountLoginFlow.test.ts @@ -18,7 +18,10 @@ import { describe, it } from "vitest" * union with no member asking for another request, so the domain cannot lead the * handler into a third pass either. */ -const HANDLER_SOURCE = readFileSync(resolve(__dirname, "../../src/ipc/handlers/accountHandlers.ts"), "utf8") +// Normalized to LF: on Windows, git checks this file out with CRLF line +// endings, and every "\n"-based search below (and in the case-slicing test) +// assumes LF. +const HANDLER_SOURCE = readFileSync(resolve(__dirname, "../../src/ipc/handlers/accountHandlers.ts"), "utf8").replace(/\r\n/g, "\n") function countOccurrences(haystack: string, needle: string): number { return haystack.split(needle).length - 1 diff --git a/tests/ipc/accountStore.test.ts b/tests/ipc/accountStore.test.ts index e2e24d65..6e921804 100644 --- a/tests/ipc/accountStore.test.ts +++ b/tests/ipc/accountStore.test.ts @@ -134,7 +134,9 @@ describe("saveAccountSecrets", () => { assert.equal(JSON.parse(raw).version, 2) }) - it("keeps the file readable only by its owner", async () => { + // chmod on Windows only toggles the read-only attribute; it cannot produce + // the POSIX 0o600 this reads back off disk. + it.skipIf(process.platform === "win32")("keeps the file readable only by its owner", async () => { const store = await loadStore() await store.saveAccountSecrets("uid-a", ACCOUNT_A) diff --git a/tests/ipc/configHandlers.test.ts b/tests/ipc/configHandlers.test.ts index 7d74031d..91e00106 100644 --- a/tests/ipc/configHandlers.test.ts +++ b/tests/ipc/configHandlers.test.ts @@ -157,7 +157,9 @@ describe("SAVE_CONFIG", () => { assert.equal(reread.defaultInstallationsFolder, join(appDataFolder, "RiftLauncherInstallations")) }) - it("reports write-failed when the config file cannot be written", async () => { + // chmod 0o500 does not stop a write on Windows: NTFS enforces read-only + // through the file attribute, not POSIX write bits on the containing folder. + it.skipIf(process.platform === "win32")("reports write-failed when the config file cannot be written", async () => { const event = await createTrustedEvent() // Make the userData folder itself read-only so the temp-file write inside diff --git a/tests/ipc/modsHandlers.test.ts b/tests/ipc/modsHandlers.test.ts index f7d7f111..269b1005 100644 --- a/tests/ipc/modsHandlers.test.ts +++ b/tests/ipc/modsHandlers.test.ts @@ -162,7 +162,10 @@ describe("EXPORT_MODPACK", () => { assert.equal(vi.mocked(dialog.showSaveDialog).mock.calls.length, 0) }) - it("returns success: false when the picked destination cannot be written to (permission denied)", async () => { + // chmod 0o500 on the directory does not stop the rename on Windows, which + // only checks the file's own read-only attribute, not POSIX write bits on + // the containing folder. + it.skipIf(process.platform === "win32")("returns success: false when the picked destination cannot be written to (permission denied)", async () => { // Pre-seed an existing file, then take away write permission on its // DIRECTORY. writeJsonAtomic writes the temp file and renames it over the // destination rather than truncating it in place, so a read-only target diff --git a/tests/ipc/pathsHandlers.test.ts b/tests/ipc/pathsHandlers.test.ts index bf0722d5..4530f72f 100644 --- a/tests/ipc/pathsHandlers.test.ts +++ b/tests/ipc/pathsHandlers.test.ts @@ -997,7 +997,10 @@ describe("before-quit: the limiters stop admitting work", () => { }) describe("RUN_INSTALLER", () => { - it("resolves not-windows on a non-Windows host, before any worker or spawn", async () => { + // This exercises the real, unstubbed process.platform !== "win32" branch + // (see the file header): on an actual Windows host that branch can't fire, + // so RUN_INSTALLER proceeds into the win32 arm this file doesn't cover. + it.skipIf(process.platform === "win32")("resolves not-windows on a non-Windows host, before any worker or spawn", async () => { // assertManagedPath for the installer path requires it to exist (it is // not called with { allowMissing: true }), so the not-windows check -- // which comes after it -- still needs a real file to reach. diff --git a/tests/ipc/permissions.test.ts b/tests/ipc/permissions.test.ts index 2f79fb51..fc19f4a1 100644 --- a/tests/ipc/permissions.test.ts +++ b/tests/ipc/permissions.test.ts @@ -79,7 +79,11 @@ afterEach(() => { }) describe("changePermissions", () => { - it("applies the mode to the root, its files and everything nested under it", () => { + // chmod on Windows only toggles the read-only attribute; it cannot produce + // POSIX mode bits like 0o755 or 0o600, so the mode a real tree ends up with + // has nothing to do with what changePermissions asked for. These read the + // mode back off disk, so they only mean anything on a POSIX filesystem. + it.skipIf(process.platform === "win32")("applies the mode to the root, its files and everything nested under it", () => { changePermissions({ paths: [installation], perms: 0o755 }) assert.equal(modeOf(installation), 0o755) @@ -88,14 +92,14 @@ describe("changePermissions", () => { assert.equal(modeOf(installation, "assets", "version.txt"), 0o755) }) - it("applies the mode to a single file given directly", () => { + it.skipIf(process.platform === "win32")("applies the mode to a single file given directly", () => { changePermissions({ paths: [join(installation, "Vintagestory")], perms: 0o750 }) assert.equal(modeOf(installation, "Vintagestory"), 0o750) assert.equal(modeOf(installation, "assets", "version.txt"), 0o600) }) - it("walks every root it is given", () => { + it.skipIf(process.platform === "win32")("walks every root it is given", () => { const second = workspacePath("data") mkdirSync(second) writeFileSync(join(second, "clientsettings.json"), "{}", { mode: 0o600 }) @@ -106,19 +110,19 @@ describe("changePermissions", () => { assert.equal(modeOf(second, "clientsettings.json"), 0o755) }) - it("skips a path that is not there", () => { + it.skipIf(process.platform === "win32")("skips a path that is not there", () => { assert.doesNotThrow(() => changePermissions({ paths: [workspacePath("never-installed"), installation], perms: 0o755 })) assert.equal(modeOf(installation, "Vintagestory"), 0o755) }) - it("does nothing at all when given no paths", () => { + it.skipIf(process.platform === "win32")("does nothing at all when given no paths", () => { changePermissions({ paths: [], perms: 0o755 }) assert.equal(modeOf(installation, "Vintagestory"), 0o600) }) - it("refuses a symbolic link rather than applying the mode to what it points at", () => { + it.skipIf(process.platform === "win32")("refuses a symbolic link rather than applying the mode to what it points at", () => { const outsider = workspacePath("outsider.txt") writeFileSync(outsider, "not the launcher's file", { mode: 0o600 }) symlinkSync(outsider, join(installation, "shortcut")) From eb285ba7695f6dc0bbba46538db7627781431530 Mon Sep 17 00:00:00 2001 From: Pixnop <77785313+Pixnop@users.noreply.github.com> Date: Sat, 29 Aug 2026 01:11:41 +0200 Subject: [PATCH 3/6] test: give the 17 remaining windows failures their verdict None of them turned out to be a Windows bug in the launcher. The twelve EXECUTE_GAME failures all came from one fixture assumption: every test in gameHandlers.test.ts writes a game binary called "Vintagestory", and buildGameLaunchPlan only ever looks for "Vintagestory.exe" on Windows. So the folder held no game, the handler answered no-executable, and every outcome those tests were written for went unreached, adoption included. Renaming the file by platform in one place restores all twelve, and the two that looked like they diverged into session adoption with mismatched uids were the same cascade one level further down: with no launch there is no session write, so nothing was there to adopt. Reproduced on Linux by pointing the same helper at a name the launcher does not know, which fails all twelve with the exact Windows messages, mismatched uids and all. Three tests in the same file cannot work on Windows whatever the binary is called. Two make a write fail by taking write permission off the installation folder and one makes a folder unlistable with chmod 0o000; NTFS has no such mode bits, so the write lands and the folder lists. They skip there with the reason on them. The three CHANGE_PERMS failures share one cause too: the handler returns false on anything that is not Linux before it looks at its arguments, which is right, since POSIX mode bits are the only thing it has to apply. So the two validation tests get no throw to catch and the worker test waits for a worker that is never started. Also reproduced on Linux, by making that early return fire here. Of the two in extraction.test.ts, one asked the filesystem whether "vintagestory" exists to prove the wrapping folder was flattened away, which on a case-insensitive filesystem answers about the "Vintagestory" file sitting next to it. The full listing on the line above already says it, and says it better, since an extra folder could not hide from it either; breaking the flattening still fails the test with that line gone. The other spends two 7-Zip processes on a 2000 file archive and runs past the five second default on a Windows runner. Nothing in the coalescing it covers is platform-specific, and #274 replaces the test with a yauzl one that spawns nothing, so it skips on Windows for now. --- tests/ipc/extraction.test.ts | 13 +++++++-- tests/ipc/gameHandlers.test.ts | 52 +++++++++++++++++++++++---------- tests/ipc/pathsHandlers.test.ts | 10 +++++-- 3 files changed, 55 insertions(+), 20 deletions(-) diff --git a/tests/ipc/extraction.test.ts b/tests/ipc/extraction.test.ts index 0d6489ff..981bf8b8 100644 --- a/tests/ipc/extraction.test.ts +++ b/tests/ipc/extraction.test.ts @@ -124,9 +124,13 @@ describe("runExtraction on a gzipped tar", () => { await runExtraction({ filePath: archivePath, outputPath: workspacePath("target"), deleteArchive: false, sevenZipBin }) + // The listing is what says the wrapping "vintagestory" folder was stepped + // into rather than copied along: it names every entry, so an extra folder + // could not hide in it. Asking the filesystem whether "vintagestory" + // exists cannot say that on Windows, where it resolves to the + // "Vintagestory" file next to it. assert.deepEqual(readdirSync(workspacePath("target")).sort(), ["Vintagestory", "assets"]) assert.equal(readFileSync(workspacePath("target", "Vintagestory"), "utf8"), "elf") - assert.equal(existsSync(workspacePath("target", "vintagestory")), false) }) it("keeps a zero byte marker as a zero byte file", async () => { @@ -221,7 +225,12 @@ describe("runExtraction on a zip", () => { assert.equal(statSync(workspacePath("target", "vintagestory", "assets", "version-1.22.6.txt")).size, 0) }) - it("coalesces 7-Zip progress and emits one terminal 100", async () => { + // Two 7-Zip processes over a 2000 file archive run past the 5 s default on a + // Windows runner, where spawning and scanning cost far more than on Linux. + // The coalescing itself has nothing platform-specific in it, so the ubuntu + // run covers it; and PR #274 replaces this test outright, dropping 7-Zip for + // a yauzl reader that needs no process at all. + it.skipIf(process.platform === "win32")("coalesces 7-Zip progress and emits one terminal 100", async () => { const source = workspacePath("many-files") mkdirSync(source, { recursive: true }) for (let index = 0; index < 2_000; index++) writeFileSync(join(source, `file-${index}.bin`), Buffer.alloc(2_048, index % 251)) diff --git a/tests/ipc/gameHandlers.test.ts b/tests/ipc/gameHandlers.test.ts index f48bd579..427f608e 100644 --- a/tests/ipc/gameHandlers.test.ts +++ b/tests/ipc/gameHandlers.test.ts @@ -57,6 +57,17 @@ type LookForAGameVersionHandler = (event: IpcMainInvokeEvent, path: unknown) => /** The key the game writes after prompting the player, which the launcher has never seen. */ const GAME_REFRESHED_KEY = "game-session-key" +/** + * The game binary's file name on the host these tests run on. + * + * buildGameLaunchPlan, and detectInstalledGameVersion with it, looks for + * `Vintagestory.exe` on Windows and the native `Vintagestory` on Linux, so a + * fixture that hard-codes either name is only a game folder on one of the two. + * On the other, the handler finds nothing and every test below gets + * `no-executable` back instead of the outcome it was written for. + */ +const GAME_EXECUTABLE = process.platform === "win32" ? "Vintagestory.exe" : "Vintagestory" + let temporaryRoot: string let managedFolder: string let versionsFolder: string @@ -166,7 +177,10 @@ describe("EXECUTE_GAME", () => { assert.deepEqual(result, { ok: false, reason: "invalid-request" }) }) - it("resolves no-executable when the version folder cannot be listed", async () => { + // chmod 0o000 cannot make a folder unlistable on Windows: NTFS has no POSIX + // mode bits, so readdir succeeds there and the readdir-failure arm this + // covers is unreachable. + it.skipIf(process.platform === "win32")("resolves no-executable when the version folder cannot be listed", async () => { const gameVersionFolder = join(versionsFolder, "1.20.0") const installationFolder = join(managedFolder, "Main") mkdirSync(gameVersionFolder, { recursive: true }) @@ -190,7 +204,7 @@ describe("EXECUTE_GAME", () => { mkdirSync(installationFolder, { recursive: true }) const realTarget = join(temporaryRoot, "real-binary") writeFileSync(realTarget, "", "utf-8") - symlinkSync(realTarget, join(gameVersionFolder, "Vintagestory")) + symlinkSync(realTarget, join(gameVersionFolder, GAME_EXECUTABLE)) writeConfig({ gameVersions: [{ version: "1.20.0", path: gameVersionFolder }] as unknown as ConfigType["gameVersions"] }) const event = await createTrustedEvent() @@ -227,7 +241,7 @@ describe("EXECUTE_GAME", () => { // That's what exercises the account-less branch of // "if (account && accountSecrets)" and gameProcessOutcomeToResult's // `started: false` arm. - const executablePath = join(gameVersionFolder, "Vintagestory") + const executablePath = join(gameVersionFolder, GAME_EXECUTABLE) writeFileSync(executablePath, "not a real binary", { mode: 0o644 }) writeConfig({ gameVersions: [{ version: "1.20.0", path: gameVersionFolder }] as unknown as ConfigType["gameVersions"] }) @@ -241,7 +255,7 @@ describe("EXECUTE_GAME", () => { const installationFolder = join(managedFolder, "Main") mkdirSync(gameVersionFolder, { recursive: true }) mkdirSync(installationFolder, { recursive: true }) - const executablePath = join(gameVersionFolder, "Vintagestory") + const executablePath = join(gameVersionFolder, GAME_EXECUTABLE) writeFileSync(executablePath, "not a real binary", { mode: 0o644 }) writeConfig({ gameVersions: [{ version: "1.20.0", path: gameVersionFolder }] as unknown as ConfigType["gameVersions"], @@ -263,7 +277,10 @@ describe("EXECUTE_GAME", () => { assert.deepEqual(vi.mocked(writeJsonAtomic).mock.calls.filter((call) => call[0] === join(installationFolder, "clientsettings.json")).length, 1) }) - it("resolves session-write-failed when the account session cannot be written into clientsettings.json", async () => { + // chmod 0o500 on the installation folder does not stop the write on Windows, + // which gates writes on the file's own read-only attribute rather than on + // POSIX write bits of the folder containing it. + it.skipIf(process.platform === "win32")("resolves session-write-failed when the account session cannot be written into clientsettings.json", async () => { const gameVersionFolder = join(versionsFolder, "1.20.0") const installationFolder = join(managedFolder, "Main") mkdirSync(gameVersionFolder, { recursive: true }) @@ -271,7 +288,7 @@ describe("EXECUTE_GAME", () => { // A real, non-symlink "Vintagestory" file is what buildGameLaunchPlan and // assertExecutable both need to see, on Linux, to hand back a plan instead // of a launchPlanFailureResult/invalidExecutableResult. - writeFileSync(join(gameVersionFolder, "Vintagestory"), "", "utf-8") + writeFileSync(join(gameVersionFolder, GAME_EXECUTABLE), "", "utf-8") writeConfig({ gameVersions: [{ version: "1.20.0", path: gameVersionFolder }] as unknown as ConfigType["gameVersions"], accounts: [{ email: "player@example.com", playerName: "Player", playerUid: "1", playerEntitlements: null, hostGameServer: false }], @@ -304,7 +321,7 @@ describe("EXECUTE_GAME", () => { const installationFolder = join(managedFolder, "Main") mkdirSync(gameVersionFolder, { recursive: true }) mkdirSync(installationFolder, { recursive: true }) - writeFileSync(join(gameVersionFolder, "Vintagestory"), "not a real binary", { mode: 0o644 }) + writeFileSync(join(gameVersionFolder, GAME_EXECUTABLE), "not a real binary", { mode: 0o644 }) writeConfig({ gameVersions: [{ version: "1.20.0", path: gameVersionFolder }] as unknown as ConfigType["gameVersions"], accounts: [{ email: "player@example.com", playerName: "Player", playerUid: "1", playerEntitlements: null, hostGameServer: false }], @@ -338,7 +355,7 @@ describe("EXECUTE_GAME", () => { const installationFolder = join(managedFolder, "Main") mkdirSync(gameVersionFolder, { recursive: true }) mkdirSync(installationFolder, { recursive: true }) - writeFileSync(join(gameVersionFolder, "Vintagestory"), "not a real binary", { mode: 0o644 }) + writeFileSync(join(gameVersionFolder, GAME_EXECUTABLE), "not a real binary", { mode: 0o644 }) writeConfig({ gameVersions: [{ version: "1.20.0", path: gameVersionFolder }] as unknown as ConfigType["gameVersions"], accounts: [{ email: "player@example.com", playerName: "Player", playerUid: "1", playerEntitlements: null, hostGameServer: false }], @@ -363,12 +380,15 @@ describe("EXECUTE_GAME", () => { assert.equal(settings.stringSettings.sessionkey, "our-own-stale-key", "our own session, even a stale one, is left exactly as it was") }) - it("resolves session-write-failed when a foreign session cannot be cleared", async () => { + // Same reason as the write test above: the read-only folder that blocks + // writeJsonAtomic's rename on a POSIX filesystem does not block it on NTFS, + // which has no such mode bits. + it.skipIf(process.platform === "win32")("resolves session-write-failed when a foreign session cannot be cleared", async () => { const gameVersionFolder = join(versionsFolder, "1.20.0") const installationFolder = join(managedFolder, "Main") mkdirSync(gameVersionFolder, { recursive: true }) mkdirSync(installationFolder, { recursive: true }) - writeFileSync(join(gameVersionFolder, "Vintagestory"), "", "utf-8") + writeFileSync(join(gameVersionFolder, GAME_EXECUTABLE), "", "utf-8") writeConfig({ gameVersions: [{ version: "1.20.0", path: gameVersionFolder }] as unknown as ConfigType["gameVersions"], accounts: [{ email: "player@example.com", playerName: "Player", playerUid: "1", playerEntitlements: null, hostGameServer: false }], @@ -408,7 +428,7 @@ describe("EXECUTE_GAME", () => { const installationFolder = join(managedFolder, "Main") mkdirSync(gameVersionFolder, { recursive: true }) mkdirSync(installationFolder, { recursive: true }) - writeFileSync(join(gameVersionFolder, "Vintagestory"), "not a real binary", { mode: 0o644 }) + writeFileSync(join(gameVersionFolder, GAME_EXECUTABLE), "not a real binary", { mode: 0o644 }) writeConfig({ gameVersions: [{ version: "1.20.0", path: gameVersionFolder }] as unknown as ConfigType["gameVersions"], accounts: [{ email: "player@example.com", playerName: "Player", playerUid: "1", playerEntitlements: null, hostGameServer: false }], @@ -448,7 +468,7 @@ describe("EXECUTE_GAME", () => { const installationFolder = join(managedFolder, "Main") mkdirSync(gameVersionFolder, { recursive: true }) mkdirSync(installationFolder, { recursive: true }) - writeFileSync(join(gameVersionFolder, "Vintagestory"), "not a real binary", { mode: 0o644 }) + writeFileSync(join(gameVersionFolder, GAME_EXECUTABLE), "not a real binary", { mode: 0o644 }) writeConfig({ gameVersions: [{ version: "1.20.0", path: gameVersionFolder }] as unknown as ConfigType["gameVersions"], accounts: [{ email: "player@example.com", playerName: "Player", playerUid: "1", playerEntitlements: null, hostGameServer: false }], @@ -493,7 +513,7 @@ describe("EXECUTE_GAME", () => { const installationFolder = join(managedFolder, "Main") mkdirSync(gameVersionFolder, { recursive: true }) mkdirSync(installationFolder, { recursive: true }) - writeFileSync(join(gameVersionFolder, "Vintagestory"), "not a real binary", { mode: 0o644 }) + writeFileSync(join(gameVersionFolder, GAME_EXECUTABLE), "not a real binary", { mode: 0o644 }) writeConfig({ gameVersions: [{ version: "1.20.0", path: gameVersionFolder }] as unknown as ConfigType["gameVersions"], accounts: [ @@ -529,7 +549,7 @@ describe("EXECUTE_GAME", () => { const installationFolder = join(managedFolder, "Main") mkdirSync(gameVersionFolder, { recursive: true }) mkdirSync(installationFolder, { recursive: true }) - writeFileSync(join(gameVersionFolder, "Vintagestory"), "not a real binary", { mode: 0o644 }) + writeFileSync(join(gameVersionFolder, GAME_EXECUTABLE), "not a real binary", { mode: 0o644 }) writeConfig({ gameVersions: [{ version: "1.20.0", path: gameVersionFolder }] as unknown as ConfigType["gameVersions"], accounts: [ @@ -576,7 +596,7 @@ describe("EXECUTE_GAME", () => { const installationFolder = join(managedFolder, "Main") mkdirSync(gameVersionFolder, { recursive: true }) mkdirSync(installationFolder, { recursive: true }) - writeFileSync(join(gameVersionFolder, "Vintagestory"), "not a real binary", { mode: 0o644 }) + writeFileSync(join(gameVersionFolder, GAME_EXECUTABLE), "not a real binary", { mode: 0o644 }) writeConfig({ gameVersions: [{ version: "1.20.0", path: gameVersionFolder }] as unknown as ConfigType["gameVersions"], accounts: [ @@ -640,7 +660,7 @@ describe("LOOK_FOR_A_GAME_VERSION", () => { // A directory named like the Linux candidate: pickExecutable matches it by // name alone, so the probe is attempted, and assertExecutable's own // isFile() check is what refuses it -- no process ever spawns. - mkdirSync(join(folder, "Vintagestory"), { recursive: true }) + mkdirSync(join(folder, GAME_EXECUTABLE), { recursive: true }) writeConfig({ gameVersions: [{ version: "1.20.0", path: folder }] as unknown as ConfigType["gameVersions"] }) const event = await createTrustedEvent() diff --git a/tests/ipc/pathsHandlers.test.ts b/tests/ipc/pathsHandlers.test.ts index 4530f72f..c92edd98 100644 --- a/tests/ipc/pathsHandlers.test.ts +++ b/tests/ipc/pathsHandlers.test.ts @@ -259,7 +259,11 @@ describe("DOWNLOAD_ON_PATH / EXTRACT_ON_PATH / RUN_INSTALLER / COMPRESS_ON_PATH: }) }) -describe("CHANGE_PERMS: assertion throws", () => { +// CHANGE_PERMS returns false on anything that is not Linux before it looks at +// its arguments at all (pathsHandlers.ts: `if (os.platform() !== "linux") return +// false`), since POSIX mode bits are the only thing it has to apply. On Windows +// nothing here can throw, so these two cover the Linux arm only. +describe.skipIf(process.platform === "win32")("CHANGE_PERMS: assertion throws", () => { it("throws on an empty paths array", async () => { const event = await createTrustedEvent() await assert.rejects(() => handler(IPC_CHANNELS.PATHS_MANAGER.CHANGE_PERMS)(event, [], 0o644), /Invalid permissions paths/) @@ -832,7 +836,9 @@ describe("COMPRESS_ON_PATH: runTrackedWorker via a fake worker", () => { }) }) -describe("CHANGE_PERMS: runTrackedWorker via a fake worker", () => { +// Same Linux-only early return: on Windows the handler resolves false without +// ever starting a worker, so the fake worker this waits for never arrives. +describe.skipIf(process.platform === "win32")("CHANGE_PERMS: runTrackedWorker via a fake worker", () => { it("resolves true once the worker finishes", async () => { const event = await createTrustedEvent() const workerPromise = nextTrackedWorker() From 37e86de1dbcd479aaa8a90059c74600b6581298a Mon Sep 17 00:00:00 2001 From: Pixnop <77785313+Pixnop@users.noreply.github.com> Date: Sat, 29 Aug 2026 01:18:08 +0200 Subject: [PATCH 4/6] fix(game): report a spawn that throws as a failed launch, not as an exception With the fixtures naming the binary Windows actually looks for, the Windows job got far enough to spawn it, and nine tests then failed on a raw "spawn UNKNOWN" coming out of the handler itself. child_process.spawn only reports ENOENT, EACCES, EAGAIN, EMFILE and ENFILE through an "error" event. Everything else it throws where it stands, and Windows answers a file that is not a valid executable with UNKNOWN, which is none of those five. Both spawns in this file were written for the event alone, so the throw went straight past the promise and out through the handler. EXECUTE_GAME rejected instead of resolving launch-failed, which is the exact anti-pattern gameProcessOutcomeToResult exists to end, and LOOK_FOR_A_GAME_VERSION rejected instead of reporting no version found. What reaches the player is a game version whose executable a stopped download truncated or an antivirus emptied: on Linux that is EACCES and an ordinary "couldn't run it" notice, on Windows it was the generic error the renderer shows for an exception, with none of the log lines the failure path writes. Both spawns now catch it and settle the way the error event does. The two tests pinning it drive the throw through a spawn wrapper rather than through a real Windows failure, so they hold the contract on every platform rather than only where the bug shows. --- src/ipc/handlers/gameHandlers.ts | 29 +++++++++++++- tests/ipc/gameHandlers.test.ts | 67 ++++++++++++++++++++++++++++++++ 2 files changed, 94 insertions(+), 2 deletions(-) diff --git a/src/ipc/handlers/gameHandlers.ts b/src/ipc/handlers/gameHandlers.ts index af58e508..e418b4a4 100644 --- a/src/ipc/handlers/gameHandlers.ts +++ b/src/ipc/handlers/gameHandlers.ts @@ -1,5 +1,6 @@ import { ipcMain } from "electron" import { spawn } from "node:child_process" +import type { ChildProcessWithoutNullStreams } from "node:child_process" import fse from "fs-extra" import { join } from "node:path" import os from "node:os" @@ -123,7 +124,21 @@ function realGameProcess(): GameProcess { resolve(outcome) } - const externalApp = spawn(request.command, request.args, { env: { ...request.env }, cwd: request.cwd, shell: false, windowsHide: true }) + let externalApp: ChildProcessWithoutNullStreams + try { + externalApp = spawn(request.command, request.args, { env: { ...request.env }, cwd: request.cwd, shell: false, windowsHide: true }) + } catch (err) { + // spawn reports ENOENT, EACCES, EAGAIN, EMFILE and ENFILE through an "error" + // event and THROWS everything else, which is not a distinction the caller can + // do anything with. Windows answers a file that is not a real executable with + // UNKNOWN, so a truncated or quarantined Vintagestory.exe lands here rather + // than on the event below; without this the whole handler rejects and the + // launch stops being a reason the player is told. + logMessage("error", `[back] [ipc] [ipc/handlers/gameHandlers.ts] [EXECUTE_GAME] Error running Vintage Story.`) + logMessage("verbose", `[back] [ipc] [ipc/handlers/gameHandlers.ts] [EXECUTE_GAME] ${getErrorMessage(err)}`) + settle({ started: false, error: getErrorMessage(err) }) + return + } externalApp.stdout.resume() @@ -338,7 +353,17 @@ function realProcessProbe(): ProcessProbe { resolve(outcome) } - const externalApp = spawn(request.command, request.args, { shell: false, windowsHide: true }) + let externalApp: ChildProcessWithoutNullStreams + try { + externalApp = spawn(request.command, request.args, { shell: false, windowsHide: true }) + } catch (err) { + // Same throw-instead-of-emit split as EXECUTE_GAME's spawn above. Resolved + // rather than settled: no timer exists yet at this point. + logMessage("error", `[back] [ipc] [gameHandlers.ts] [LOOK_FOR_A_GAME_VERSION] Error looking for the Vintage Story version.`) + logMessage("verbose", `[back] [ipc] [gameHandlers.ts] [LOOK_FOR_A_GAME_VERSION] ${getErrorMessage(err)}`) + resolve({ ok: false, stdout: "", error: getErrorMessage(err) }) + return + } const timer = setTimeout(() => { logMessage("error", `[back] [ipc] [gameHandlers.ts] [LOOK_FOR_A_GAME_VERSION] Timed out waiting for Vintage Story to report its version.`) diff --git a/tests/ipc/gameHandlers.test.ts b/tests/ipc/gameHandlers.test.ts index 427f608e..ddb54cfc 100644 --- a/tests/ipc/gameHandlers.test.ts +++ b/tests/ipc/gameHandlers.test.ts @@ -43,6 +43,31 @@ vi.mock("@src/ipc/accountStore", () => ({ adoptLegacySingleAccountSecrets: vi.fn(async () => false) })) +/** + * Makes the next spawn throw the way Windows throws, on demand. + * + * `child_process.spawn` reports ENOENT, EACCES, EAGAIN, EMFILE and ENFILE + * through an "error" event and throws every other failure synchronously. Linux + * answers this file's fixtures with EACCES and so only ever takes the event + * path, while Windows answers a file that is not a real executable with + * UNKNOWN and takes the throw path, which used to reject the whole handler. + * Every other test here keeps the real spawn: the flag is off unless a test + * turns it on. + */ +const spawnThrow = vi.hoisted(() => ({ next: false })) + +vi.mock("node:child_process", async (importOriginal) => { + const actual = await importOriginal() + return { + ...actual, + spawn: (...args: Parameters): ReturnType => { + if (!spawnThrow.next) return actual.spawn(...args) + spawnThrow.next = false + throw Object.assign(new Error("spawn UNKNOWN"), { code: "UNKNOWN", errno: -4094, syscall: "spawn" }) + } + } +}) + // Real implementation, wrapped, so the crash-safety guarantee stays covered by // atomicJsonFile.test.ts and this file only asserts that the settings write // goes through the shared adapter rather than a bare fse.writeJSON. @@ -250,6 +275,31 @@ describe("EXECUTE_GAME", () => { assert.deepEqual(result, { ok: false, reason: "launch-failed" }) }) + /** + * A spawn that throws instead of emitting is still a launch that did not + * happen, and this handler's whole contract is that a launch a player can + * fail to complete comes back as a reason rather than as an exception. It + * only shows up on Windows, where a Vintagestory.exe that is not a valid + * executable (a truncated download, a file antivirus emptied) is answered + * with UNKNOWN rather than with one of the five codes spawn reports through + * an event. Both spawns in this file were written for the event alone. + */ + it("resolves launch-failed when the spawn throws instead of emitting an error", async () => { + const gameVersionFolder = join(versionsFolder, "1.20.0") + const installationFolder = join(managedFolder, "Main") + mkdirSync(gameVersionFolder, { recursive: true }) + mkdirSync(installationFolder, { recursive: true }) + writeFileSync(join(gameVersionFolder, GAME_EXECUTABLE), "not a real binary", { mode: 0o644 }) + writeConfig({ gameVersions: [{ version: "1.20.0", path: gameVersionFolder }] as unknown as ConfigType["gameVersions"] }) + + spawnThrow.next = true + const event = await createTrustedEvent() + const result = await executeGameHandler()(event, { version: "1.20.0", path: gameVersionFolder }, baseInstallation({ path: installationFolder })) + + assert.deepEqual(result, { ok: false, reason: "launch-failed" }) + assert.equal(spawnThrow.next, false, "the throwing spawn is the one this test ran") + }) + it("resolves launch-failed after successfully writing the account session first", async () => { const gameVersionFolder = join(versionsFolder, "1.20.0") const installationFolder = join(managedFolder, "Main") @@ -668,6 +718,23 @@ describe("LOOK_FOR_A_GAME_VERSION", () => { assert.deepEqual(result, { exists: false }) }) + // The probe's own spawn has the same throw-instead-of-emit gap EXECUTE_GAME's + // does, and reaching it needs a candidate that gets past assertExecutable, so + // this one is a real file rather than a directory. + it("reports not found when the probe's spawn throws instead of emitting an error", async () => { + const folder = join(versionsFolder, "throwing-probe") + mkdirSync(folder, { recursive: true }) + writeFileSync(join(folder, GAME_EXECUTABLE), "not a real binary", { mode: 0o644 }) + writeConfig({ gameVersions: [{ version: "1.20.0", path: folder }] as unknown as ConfigType["gameVersions"] }) + + spawnThrow.next = true + const event = await createTrustedEvent() + const result = await lookForAGameVersionHandler()(event, folder) + + assert.deepEqual(result, { exists: false }) + assert.equal(spawnThrow.next, false, "the throwing spawn is the one this test ran") + }) + it("reports not found when only the mono fallback candidate (Vintagestory.exe) is present and fails its probe", async () => { const folder = join(versionsFolder, "mono-fallback") // No "Vintagestory" (the native candidate checked first), so From 0d8984f4b962214ef89f5e704e1e2532daad1764 Mon Sep 17 00:00:00 2001 From: Pixnop <77785313+Pixnop@users.noreply.github.com> Date: Sat, 29 Aug 2026 16:15:01 +0200 Subject: [PATCH 5/6] ci: keep a job named test so the required context still exists dev branch protection requires a status context literally named "test", and a matrixed job cannot produce one: it reports "test (ubuntu-latest)" and "test (windows-latest)" instead. Rename the matrix job to test-matrix and add a small gate job that keeps the required name, so the protection rule needs no coordinated edit. The gate runs with always() because a plain needs would skip it when a leg fails, and protection counts a skipped required job as satisfied. It then compares needs.test-matrix.result against success, which is only the case when every leg passed, so a failed, cancelled or skipped matrix turns the gate red. --- .github/workflows/ci.yml | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 8e9b085b..acfd8dd1 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -32,7 +32,7 @@ jobs: - run: npm run lint:ci - run: npm run format:check - test: + test-matrix: strategy: fail-fast: false matrix: @@ -47,6 +47,22 @@ jobs: - run: npm ci - run: npm run test:coverage + # `dev` branch protection requires a context literally named `test`, and a + # matrixed job can't produce one: it reports `test (ubuntu-latest)` and + # `test (windows-latest)` instead. This job keeps the required name alive. + # `always()` makes it run even when a leg fails (without it the gate would be + # skipped, and protection counts a skipped required job as satisfied), and + # the explicit comparison fails the gate unless every leg succeeded, so a + # failed, cancelled or skipped matrix turns it red. + test: + needs: [test-matrix] + if: always() + runs-on: ubuntu-latest + steps: + - run: | + echo "test-matrix result: ${{ needs.test-matrix.result }}" + [ "${{ needs.test-matrix.result }}" = "success" ] + # Informational quality scan: never blocks a merge, so a SonarCloud outage # or configuration change can't turn the whole run red. sonarcloud: From f3fe5d2724a046489cf653cd7842fbf88fac63a6 Mon Sep 17 00:00:00 2001 From: Pixnop <77785313+Pixnop@users.noreply.github.com> Date: Sat, 29 Aug 2026 16:24:10 +0200 Subject: [PATCH 6/6] fix(game): settle a thrown probe spawn like the error event, and pin line endings Three follow-ups from review, all in the same file set. The probe's spawn catch resolved the promise directly while every other exit from that executor went through settle, because settle closed over a timer declared below the try and calling it earlier would have hit the temporal dead zone. The timer now starts as undefined above settle, so the catch settles like the "error" event does, clearTimeout ignoring an undefined handle. An asymmetry in how a spawn failure settles is the same family as the bug this branch fixes, and it was one moved declaration away from becoming a double-settle. spawnThrow.next is a vi.hoisted object, which vi.restoreAllMocks does not touch, so it now resets in beforeEach next to the rest of the per-test state. No test passes for the wrong reason today: both tests that set the flag assert it was consumed. The leak needs a test to fail before it reaches the spawn, and then it lands on whichever test runs next. .gitattributes normalises text to LF in the repository and on checkout, which is what a CRLF checkout of accountHandlers.ts needed on the Windows job. Two other tests read source the same way and pass only because neither asserts across a line ending. The crafted fixtures are marked binary so nothing rewrites a byte inside them; tests/fixtures/not-a-zip.bin is the one that needs it, since it holds no NUL byte and text=auto would otherwise treat it as text. Nothing in the index is CRLF today, so this renormalises no existing file. --- .gitattributes | 25 +++++++++++++++++++++++++ src/ipc/handlers/gameHandlers.ts | 12 ++++++++---- tests/ipc/gameHandlers.test.ts | 5 +++++ 3 files changed, 38 insertions(+), 4 deletions(-) create mode 100644 .gitattributes diff --git a/.gitattributes b/.gitattributes new file mode 100644 index 00000000..efbcb6ce --- /dev/null +++ b/.gitattributes @@ -0,0 +1,25 @@ +# Every text file is stored and checked out with LF, on Windows too. +# +# Three tests read a source file as text and assert on its contents +# (tests/ipc/accountLoginFlow.test.ts, tests/security-boundaries.test.ts, +# tests/renderer-dom/moddbVisibilityPrompt.test.tsx). Under a CRLF checkout the +# first of those fails, which is how the Windows CI job found this; the other +# two pass only because neither happens to assert across a line ending. +* text=auto eol=lf + +# Byte-exact fixtures. These are crafted archives and compressed streams, and +# the tests assert on declared sizes, offsets and digests inside them, so a +# single rewritten line ending changes the answer. tests/fixtures/not-a-zip.bin +# is the one that actually needs saying: it holds no NUL byte, so the rule above +# would classify it as text on its own. +*.zip binary +*.bin binary +*.lzma1 binary +*.lzma2 binary + +# Assets. Git detects these as binary by itself; the markers keep that true if +# one is ever regenerated in a format that looks textual. +*.png binary +*.jpg binary +*.ico binary +*.icns binary diff --git a/src/ipc/handlers/gameHandlers.ts b/src/ipc/handlers/gameHandlers.ts index e418b4a4..0541c67d 100644 --- a/src/ipc/handlers/gameHandlers.ts +++ b/src/ipc/handlers/gameHandlers.ts @@ -345,6 +345,10 @@ function realProcessProbe(): ProcessProbe { let stdout = "" let settled = false + // Declared before settle so that every exit from this executor, the spawn + // throw included, goes through settle. clearTimeout ignores undefined, so + // settling before the timer exists is safe. + let timer: ReturnType | undefined = undefined const settle = (outcome: ProcessProbeOutcome): void => { if (settled) return @@ -357,15 +361,15 @@ function realProcessProbe(): ProcessProbe { try { externalApp = spawn(request.command, request.args, { shell: false, windowsHide: true }) } catch (err) { - // Same throw-instead-of-emit split as EXECUTE_GAME's spawn above. Resolved - // rather than settled: no timer exists yet at this point. + // Same throw-instead-of-emit split as EXECUTE_GAME's spawn above, settled + // the way the "error" event below settles it. logMessage("error", `[back] [ipc] [gameHandlers.ts] [LOOK_FOR_A_GAME_VERSION] Error looking for the Vintage Story version.`) logMessage("verbose", `[back] [ipc] [gameHandlers.ts] [LOOK_FOR_A_GAME_VERSION] ${getErrorMessage(err)}`) - resolve({ ok: false, stdout: "", error: getErrorMessage(err) }) + settle({ ok: false, stdout, error: getErrorMessage(err) }) return } - const timer = setTimeout(() => { + timer = setTimeout(() => { logMessage("error", `[back] [ipc] [gameHandlers.ts] [LOOK_FOR_A_GAME_VERSION] Timed out waiting for Vintage Story to report its version.`) externalApp.kill() settle({ ok: false, stdout, error: "Timed out waiting for a response." }) diff --git a/tests/ipc/gameHandlers.test.ts b/tests/ipc/gameHandlers.test.ts index ddb54cfc..4208e151 100644 --- a/tests/ipc/gameHandlers.test.ts +++ b/tests/ipc/gameHandlers.test.ts @@ -141,6 +141,11 @@ function writeConfig(config: Partial): void { } beforeEach(async () => { + // vi.restoreAllMocks() in afterEach does not reach a vi.hoisted object, so a + // test that turns this on and fails before the spawn consumes it would leave + // it on for the next test, whose real spawn would then throw. + spawnThrow.next = false + temporaryRoot = mkdtempSync(join(tmpdir(), "game-handlers-")) managedFolder = join(temporaryRoot, "Installations") versionsFolder = join(temporaryRoot, "Versions")