fix(windows): create the shortcut directory before writing the shortcut

[PathGao]

The cause underneath #122, still present after #364.

The defect

create_shortcut builds dir.join("Markpad.lnk") and hands it to mslnk, which does a plain file create and will not build the path leading to it. There is no create_dir_all, no existence check, anywhere between the PathBuf::from(directory) and create_lnk.

Both call sites are in perform_install step 3, and the directories come from environment variables:

Call site	all_users	Directory
desktop	true / false	%PUBLIC%\Desktop / %USERPROFILE%\Desktop
start menu	true / false	%ProgramData%\…\Start Menu\Programs / %APPDATA%\…\Start Menu\Programs

Both propagate with ?, so either one aborts the install. #364 made that abort clean and named the directory in the error — but the directory still never gets created, so the user still cannot install.

The fix

Two new platform-independent functions wired in before ShellLink::new:

• missing_directories(dir, exists) — the levels to create, shallowest first
• ensure_directory(dir, exists, create, steps) — creates each level, pushing an InstallStep::CreatedDir per level

Per-level rather than create_dir_all: on a profile missing the whole …\Start Menu\Programs chain, create_dir_all would create two directories but let us record only one. Recording each level in creation order means #364's rollback_actions reverses them and its already-non-recursive, already-only-if-we-created-it remove_dir removes the deepest first. No change to the rollback machinery was needed — the new steps ride it unchanged.

AlreadyExists is tolerated but records nothing. We did not create it, so we must not delete it — same semantics as create_dir_all.

Judgement call: a failed create_dir fails the install, it does not skip the shortcut

1. fix(windows): make install and uninstall recoverable #364 defined exactly two outcomes: complete, or rolled back. Skipping adds a third — "success", minus an icon the user explicitly asked for. That is OS Error 3 #122's user-visible symptom with the actionable error removed.
2. The error carries the fix. Routed through io_error_message, an ERROR_ACCESS_DENIED on %PUBLIC%\Desktop still emits the locale-independent ELEVATION_REQUIRED: prefix the frontend branches on. Skipping destroys that signal.
3. "Fail" was only harsh before fix(windows): make install and uninstall recoverable #364, when it stranded a half-installed exe. Post-fix(windows): make install and uninstall recoverable #364 the failure is clean, so the argument for degrading to a silent skip is much weaker.

io_error_message is used rather than the existing describe closure because this is a real io::Error, so the permission case can be read off error.kind() instead of guessed at with a second write probe. (describe exists because mslnk's error is not an io::Error.)

Uninstall does not remove these directories. Even a non-recursive "only if empty" removal would target %USERPROFILE%\Desktop or the shared Start Menu\Programs. An empty Desktop is a normal state, and Programs belongs to every application. The asymmetry with rollback is deliberate: rollback runs seconds after creation, inside one operation, while the directory is still empty — that is the only window where "we made it and nobody has used it" holds.

Tests

Eight new. Six run against a real filesystem on macOS: ensure_directory takes exists/create as closures, so the tests drive it with std::fs against a real temp directory. They cover creating a missing two-level chain, handing each created level to rollback_actions deepest-first, recording nothing for an existing directory, tolerating a directory that appears underneath us, and returning an error rather than skipping.

One is a source-text assertion, for the only thing unreachable from macOS: whether create_shortcut calls the helper at all, and before create_lnk. It slices out just that function body so it cannot match its own text. It proves the call exists and is ordered. It proves nothing about Windows behaviour or about mslnk. No Windows run was performed.

Source-text test alone, on unmodified master	24 passed / 1 failed — a real assertion failure, not a compile error
Final	setup module 32 / 0; whole crate 106 / 0 (baseline 98)
Mutation check	5/5 caught — swallow the error, don't push steps, drop missing.reverse(), record AlreadyExists as ours, unwire the call

npm run check   432 files, 0 errors
npm test        401 / 401
cargo test      106 / 106
cargo clippy    identical 3 baseline warnings

A Windows-only compile error this caught

Passing fs::create_dir directly as the create argument does not compile for a Windows target — the generic function item binds one concrete lifetime and fails the higher-ranked Fn(&Path) bound. macOS CI can never surface that. I built a stub harness and compiled setup.rs for a real x86_64-pc-windows-msvc target: clean, and reverting that one line reproduces the error. (Full cargo check --target is impossible here — ring needs a Windows C toolchain.) The call site uses a closure, with a comment saying why.

Worth knowing: this changes the failure mode, it does not make it right

These directories come from environment variables, not SHGetKnownFolderPath. %USERPROFILE%\Desktop is simply wrong when the Desktop is redirected — OneDrive folder backup, or Group Policy redirection to a network share, both common on exactly the enterprise images #122 is reported from.

After this fix that case changes from "install fails" to "install succeeds, and a stray %USERPROFILE%\Desktop is created with the shortcut in it, where the shell does not show it." Still a strict improvement — the app installs, and the Start Menu shortcut lands in the rarely-redirected %APPDATA% path — but it is a different failure mode and you should know.

The proper fix is SHGetKnownFolderPath with FOLDERID_Desktop / CommonDesktopDirectory / Programs / CommonPrograms. The crate already depends on windows 0.61.3 for Windows targets, but enabling the Shell feature is a Cargo.toml change and it is untestable FFI from here, so I left it out rather than ship unverified Windows FFI. Happy to do it if you'd like it.

Side audit: one more "assumes a directory exists"

uninstall_app writes its batch/vbs helpers into env::temp_dir() via create_new(true), which does not create the parent; Windows' GetTempPath explicitly does not verify the directory exists. It is already safe by ordering — it happens before shortcut removal and registry deletion and returns Err, so the installation stays intact and uninstallable. Fixing it means choosing a fallback temp location; separate change.

Everything else checks out: install step 1 already create_dir_alls the install dir, can_write_dir deliberately probes the nearest existing ancestor, registry create_subkey creates intermediates, and the batch script's rmdir is guarded by a following if exist.

🤖 Generated with Claude Code

[PathGao]

Windows cargo test fixed — two independent bugs, both in mod tests; the production change is untouched and Windows compiled fine.

test create_shortcut_creates_the_target_directory_before_writing_the_lnk ... FAILED
  panicked at src\setup.rs:1634:14: create_shortcut must be brace-terminated

test ensure_directory_does_not_claim_a_directory_that_appeared_underneath_it ... FAILED
  panicked at src\setup.rs:1590:10: losing the race is not an install failure:
  Os { code: 5, kind: PermissionDenied, message: "Access is denied." }

1 — CRLF, the same trap as #389. No .gitattributes, so the Windows runner checks out with core.autocrlf=true and include_str!("setup.rs") hands the test CRLF; the "\n}\n" terminator never matches. Normalised at the read. Reproduced on macOS by converting the file in place — same line, same message as CI.

2 — the race test was accidentally green on macOS. Its exists closure was |_| false, so missing_directories' take_while never stopped and walked to the filesystem root. The first level handed to create was not Desktop — it was /:

	create_dir on the root returns
macOS	AlreadyExists → swallowed by the race branch → test continued
Windows	PermissionDenied → returned Err → test failed

So every ancestor level was noise, and that noise happens to have a platform-dependent error kind. The closure is now |candidate| candidate != desktop — the same shape production has, where candidate.exists() stops the walk at the first existing ancestor.

Production is not affected: create_shortcut passes |candidate| candidate.exists(), a drive root always exists, so the walk stops there and a root is never created. UNC is safe too — \\\\server\\share\\'s parent() is None, so the chain cannot degrade to \\\\server.

No assertion was weakened — both got stronger. The source-text test still requires ensure_directory inside create_shortcut and before create_lnk; only the slicing changed. The race test's assertions are byte-identical, and now the swallowed AlreadyExists is the one the test deliberately creates rather than incidental ancestor noise — deleting the AlreadyExists branch still turns it red.

	LF	CRLF
before	106 / 0	105 / 1
after	106 / 0	106 / 0

Worth noting for a separate decision: this is the second PR bitten by the missing .gitattributes (#389 was the first, same root cause). A * text=auto eol=lf would close the class, but it changes checkout behaviour for every contributor and can produce a whole-repo re-normalisation, so it is a repo-policy call rather than something to slip into a fix. Happy to open it separately if you want it.

[PathGao]

Withdrawing the .gitattributes suggestion from my previous comment — I weighted it wrong.

The exposure is two call sites, both of which are now fixed at the read. Everything else that reads source as text (the ~44 TypeScript convention tests) matches with patterns that don't depend on exact newlines, which is why npm test has been green on the Windows runner throughout, including on both of these failures.

So a repo-wide checkout policy would be guarding an empty set, and it would only flatten the environment rather than make the pattern robust — a contributor whose editor writes CRLF would still hit it. Please disregard; nothing to decide here.