fix(desktop): keep the previous artifact destination when its replacement fails

[Adarsh-Me]

Summary

Fixes #4832. app:saveArtifactAs removed the destination file before renaming the staging file into place:

await rm(targetPath, { force: true });
await rename(stagingPath, targetPath);

A rename that failed after a successful unlink therefore deleted a file the user already had, and the save reported write_failed — the exact sequence the issue's fault-injection reproduction produced (ORIGINAL gone, ENOENT afterwards).

rename(2) replaces an existing destination atomically on every platform Electron runs on (POSIX rename, Windows MoveFileExW with MOVEFILE_REPLACE_EXISTING), so the unlink was both unnecessary and destructive. The replacement now runs directly against the staged file: a failed rename leaves the previous destination byte-identical, and the staging file is still cleaned up by the existing catch. This also removes the non-atomic window between unlink and rename in the success path, during which a crash left no file at either path.

materializeArtifact is now exported with an injected replacement step (defaulting to rename) so the fault-injection test can fail the final replacement deterministically — the issue's reproduction needed EIO injection on exactly that call, which is otherwise not reachable on all platforms.

Verification

• Repro of the issue's scenario as a regression test: existing destination containing ORIGINAL, injection fails only the final replacement — the test asserts the destination still reads ORIGINAL and the directory contains no stray staging files. Fails against the unfixed code (the seam does not exist there, and with rm first the destination is lost); passes with the fix. Verified both ways locally by rebuilding with and without the source change.
• Control test: a normal app:saveArtifactAs overwrite of an existing destination succeeds and replaces the content end-to-end.
• Full runtime-host-artifacts-ipc-main.test.js: 5/5 pass (3 pre-existing + 2 new).
• biome check on both changed files: clean.
• Builds: @maka/core, @maka/storage, @maka/runtime, @maka/runtime-host (protocol), desktop build:main — all emit; the desktop tsc run reports pre-existing missing-workspace errors for unbuilt UI packages only, unrelated to these files.

AI use

Select exactly one:

• No generative tool made a substantive contribution
• Generative tooling made a substantive contribution

Tool(s) and scope:

Checklist

• Tests cover the change and fail without it — the fault-injection test fails against the unfixed code (verified by rebuild).
• Lint, format, typecheck and the affected suites pass locally — see Verification; the full Desktop suite runs in CI.

Does this PR entail a change in behavior?

• No — a failed overwrite now preserves the previous destination, matching the issue's stated expected behavior; successful saves behave identically.

[Astro-Han]

Reviewed current head 8191028fe158e5650caa465f010dcaa1472c72ca (OPEN). Technical GO — no P0–P2, two P3s below. The fix is right, and the reasoning is in the code. Note on gate state: this head carries only label:success — test has never been scheduled here, so merge should wait for a scheduled run or proceed explicitly on that basis.

P3 — a private function exported for tests, with a test-only parameter

materializeArtifact went from module-private to exported with a sixth parameter replaceDestination defaulting to rename — the comment honestly states this opens a seam for the #4832 fault-injection tests. Cost: the production and test paths are not the same one. The test injects a directly-throwing function, verifying "target unchanged when replaceDestination throws" — not "target unchanged when the real rename fails", which is what users hit. Between them sits the untested assumption that the default is indeed rename. Acceptable (making real rename fail portably without flakiness is hard), but suggest at least an assertion that the default path goes through rename, or a comment stating the default path is uncovered. Also: once exported with that parameter, nothing stops a future production caller passing something else — both current production call sites (:96, :121) correctly pass nothing.

P3 — no directory fsync after rename

handle.sync() guarantees file content; the directory-entry change from rename is not synced. On crash/power loss, some filesystems can show "content present, rename not applied" — old target content plus a leftover staging file. This repo cares elsewhere (syncDirectory helpers on storage write paths); here it is desktop "save as", user-triggered and retryable, so durability demands are a tier lower — P3, not higher. But the established helper exists, so one directory sync is cheap to add.

Checked: Windows rename semantics

The comment's "replaces atomically on every platform" holds for replacement (libuv MoveFileExW with MOVEFILE_REPLACE_EXISTING). Difference worth knowing: on Windows rename fails if the target is open elsewhere, where POSIX succeeds — but that is no regression here: the old code's rm(targetPath) failed on held files too, earlier and worse. The new code is strictly better in that case.

What I could not judge

No tests/build/desktop run locally; the #4832 issue report itself was not re-read (problem reconstructed from code and comments).

---

Automated review notice: This comment was posted by an automated review agent operated by Astro-Han. It is not an independent human review and does not replace one.

简体中文

评审结论来自自动化审查流程；发布者没有读这份 diff，核的是当前 head 有没有漂移。当前 head 是 8191028，未关闭，test 从未被调度。修法是对的，技术上无阻断问题，两条 P3：测试开的口子与默认路径假设、改名后没 sync 目录。等人类拍板。

[me2seeks]

Automated review by OpenAI Codex, operated by me2seeks, at 8191028fe158e5650caa465f010dcaa1472c72ca. This is an automated technical assessment, not an independent human review. Approval is submitted at the operator's explicit direction.

No actionable findings. Removing the pre-rename unlink preserves the existing destination when replacement fails. All five exact-source IPC tests passed, including the public Save As overwrite using the default rename path. Reintroducing unlink before replacement makes the fault-injection test fail with ENOENT for the original destination. However, current main already fixed this in #4833 (6cd7222), with public IPC fault coverage and classified failure reasons.

1. Optimal for the actual problem: Correct at this branch head, but superseded on current main by the more complete #4833 implementation.
2. Production code that can be deleted: The unsafe unlink is correctly deleted. Do not restore it or replace the current-main implementation while resolving conflicts.
3. Low-quality tests that can be deleted or replaced: No deletion needed in the branch; current main already has broader public Save As fault tests.
4. Deeper refactor: No deeper refactor is needed.
5. Ready to merge: Content passes, but this PR is superseded on main; do not merge a conflict resolution that restores old code.
6. Residual risks / verification: Five bundled exact-source tests passed and the old-unlink mutation failed. No Windows or Electron GUI execution was performed. The test-only replacement seam is unnecessary to port because main tests the public IPC boundary. Directory crash durability beyond existing behavior was not part of this repair.