s1d3sw1ped/SwipedModSwitcher

Fork 0

switch: Never delete a real Mods folder when picking a profile #46

Merged

linus merged 2 commits from ash/safe-switch-45 into develop

2026-10-24 17:05:35 +00:00

ash commented

2026-10-24 16:58:23 +00:00

Collaborator

Closes #45

Why

Picking a profile ran os.RemoveAll on the game mods path and ignored the error. The first time someone used the switcher, their real Mods folder (full of mods) was wiped with no prompt or backup, and a half-deleted folder looked like a clean switch.

What changed

New package switcher (no fyne import, so it's testable) with Switch(profile, modsPath). It Lstats the mods path first:
- symlink: removes only the link (os.Remove) and makes the new one. If the new link fails, the old link is put back.
- real folder or file: refused with ErrRealModsFolder, nothing touched. I picked refuse over move-and-confirm because it's simpler and can't lose anything. The dialog tells the user to move the contents into a new folder under the profiles dir and then remove or rename the Mods folder.
- nothing there: just makes the link.
Remove and link errors are returned and shown with dialog.NewError (real error text), instead of the generic "not admin" message.
CI: added a Test on windows step (go test ./switcher/...) to the windows job in pull-request-ci and push-ci. The Linux test job already runs go test -race ./....

Tests

switcher/switch_test.go, 4 tests, all passing locally (go test -race -count=1 -v ./switcher/):

empty path → link made
link → link: target switches, both profiles keep their files
real folder: ErrRealModsFolder, folder and nested file untouched
link failure: real error returned, existing link unchanged

Risk / rollback

Users whose Mods path is a real folder now get a refusal instead of a wipe; that's intended. Rollback: revert this commit.

Closes #45 ## Why Picking a profile ran `os.RemoveAll` on the game mods path and ignored the error. The first time someone used the switcher, their real Mods folder (full of mods) was wiped with no prompt or backup, and a half-deleted folder looked like a clean switch. ## What changed - New package `switcher` (no fyne import, so it's testable) with `Switch(profile, modsPath)`. It Lstats the mods path first: - **symlink**: removes only the link (`os.Remove`) and makes the new one. If the new link fails, the old link is put back. - **real folder or file**: refused with `ErrRealModsFolder`, nothing touched. I picked **refuse** over move-and-confirm because it's simpler and can't lose anything. The dialog tells the user to move the contents into a new folder under the profiles dir and then remove or rename the Mods folder. - **nothing there**: just makes the link. - Remove and link errors are returned and shown with `dialog.NewError` (real error text), instead of the generic "not admin" message. - CI: added a `Test on windows` step (`go test ./switcher/...`) to the windows job in pull-request-ci and push-ci. The Linux test job already runs `go test -race ./...`. ## Tests `switcher/switch_test.go`, 4 tests, all passing locally (`go test -race -count=1 -v ./switcher/`): - empty path → link made - link → link: target switches, both profiles keep their files - real folder: `ErrRealModsFolder`, folder and nested file untouched - link failure: real error returned, existing link unchanged ## Risk / rollback Users whose Mods path is a real folder now get a refusal instead of a wipe; that's intended. Rollback: revert this commit.

ash added 1 commit

2026-10-24 16:58:23 +00:00

switch: Never delete a real Mods folder when picking a profile

CI / Build windows (pull_request) Failing after 11s

Details

CI / Build (pull_request) Successful in 45s

Details

CI / Test (pull_request) Successful in 47s

Details

CI / Lint (pull_request) Successful in 53s

Details

a0a46baa30

Picking a profile ran os.RemoveAll on the game mods path and ignored the
error, so the first switch wiped a real Mods folder full of mods.

Move the switch logic into package switcher (no fyne, so it is testable)
and Lstat the mods path first: a symlink is removed with os.Remove and
replaced, a real folder is refused with ErrRealModsFolder and left
untouched, and an empty path just gets the link. Remove and link errors
are returned and shown; if the new link fails, the old link is put back.

Run the switcher tests on the Windows runner too.

Closes #45

ash requested review from eva

2026-10-24 16:58:23 +00:00

ash added 1 commit

2026-10-24 17:00:07 +00:00

switcher: Skip symlink tests where the account cannot make symlinks

CI / Build windows (pull_request) Successful in 11s

Details

CI / Build (pull_request) Successful in 41s

Details

CI / Test (pull_request) Successful in 47s

Details

CI / Lint (pull_request) Successful in 51s

Details

7391559d8a

The Windows CI runner lacks SeCreateSymbolicLinkPrivilege, so os.Symlink
fails there. Skip the link cases with the real error as the reason; the
real-folder test still runs on Windows.

eva approved these changes

2026-10-24 17:00:39 +00:00

eva left a comment

GO from me on 7391559.

This fixes the real problem in #45: a real Mods folder is never deleted now. Switch Lstats first, only ever removes a symlink, refuses a real folder or file with ErrRealModsFolder, and puts the old link back if the new one fails. The dialog now shows the actual error instead of the "not admin" guess. Refusing instead of moving is the right call for a first fix, since it can't lose anything. Head is on the current develop tip.

One small thing, not blocking: TestSwitchLinkFailureLeavesOldLink links a different path (missing/Mods), so it never goes through the remove-then-relink branch and the restore code isn't actually tested. If you want real coverage there, a follow-up could make the new link fail on the same path (for example a profile path that's unusable on that OS). Fine to leave for later.

The CI change only adds a go test ./switcher/... step on Windows, with no secrets and no new triggers. Because it touches workflows I'm adding Blake. Linus merges once Blake says GO and CI is green.

GO from me on 7391559. This fixes the real problem in #45: a real Mods folder is never deleted now. `Switch` Lstats first, only ever removes a symlink, refuses a real folder or file with `ErrRealModsFolder`, and puts the old link back if the new one fails. The dialog now shows the actual error instead of the "not admin" guess. Refusing instead of moving is the right call for a first fix, since it can't lose anything. Head is on the current develop tip. One small thing, not blocking: `TestSwitchLinkFailureLeavesOldLink` links a different path (`missing/Mods`), so it never goes through the remove-then-relink branch and the restore code isn't actually tested. If you want real coverage there, a follow-up could make the new link fail on the same path (for example a profile path that's unusable on that OS). Fine to leave for later. The CI change only adds a `go test ./switcher/...` step on Windows, with no secrets and no new triggers. Because it touches workflows I'm adding Blake. Linus merges once Blake says GO and CI is green.

eva requested review from blake

2026-10-24 17:00:40 +00:00

ash commented

2026-10-24 17:01:13 +00:00

Author

Collaborator

Update at 7391559: the Windows runner account can't create symlinks (A required privilege is not held by the client, run 166). The three link tests now skip there and give that error as the reason. The real-folder test, which is the no-data-loss guarantee, runs and passes on Windows (run 167). All four tests run on Linux. To get full link coverage on Windows, the win-runner service account needs Developer Mode or SeCreateSymbolicLinkPrivilege. That's an ops follow-up, not part of this PR.

Update at 7391559: the Windows runner account can't create symlinks (`A required privilege is not held by the client`, run 166). The three link tests now skip there and give that error as the reason. The real-folder test, which is the no-data-loss guarantee, runs and passes on Windows (run 167). All four tests run on Linux. To get full link coverage on Windows, the win-runner service account needs Developer Mode or SeCreateSymbolicLinkPrivilege. That's an ops follow-up, not part of this PR.

blake approved these changes

2026-10-24 17:05:08 +00:00

blake left a comment

Blake security review: APPROVE at 7391559d8a.

Workflows: only adds a "Test on windows" step (go test ./switcher/...) to the existing windows job in pull-request-ci.yaml and push-ci.yaml. Triggers unchanged, no permissions: change, no secrets or env secrets used.
Actions: unchanged, all pinned to tags from official sources (actions/*@v4/v5, golangci-lint-action@v8, code.forgejo.org upload-artifact@v4). No @main in this workflow.
Runner: existing windows label. Tests write only under t.TempDir and skip when the account can't make symlinks, so they run fine as the non-admin runner user and touch no locked toolchain dirs.
Code: switcher.Switch refuses to delete a real Mods folder and only removes symlinks; that removes the old unconditional RemoveAll. Safer than before.

Blake security review: APPROVE at 7391559d8a. - Workflows: only adds a "Test on windows" step (`go test ./switcher/...`) to the existing windows job in pull-request-ci.yaml and push-ci.yaml. Triggers unchanged, no `permissions:` change, no secrets or env secrets used. - Actions: unchanged, all pinned to tags from official sources (actions/*@v4/v5, golangci-lint-action@v8, code.forgejo.org upload-artifact@v4). No `@main` in this workflow. - Runner: existing `windows` label. Tests write only under t.TempDir and skip when the account can't make symlinks, so they run fine as the non-admin runner user and touch no locked toolchain dirs. - Code: switcher.Switch refuses to delete a real Mods folder and only removes symlinks; that removes the old unconditional RemoveAll. Safer than before.