fix(desktop): fsync the artifact directory after a successful rename

[Adarsh-Me]

Summary

Narrowed scope. The destination-loss fix this PR opened with is already upstream — #4833 (6cd72229ae, "preserve destination on artifact export failure") removes the same await rm(targetPath, { force: true }) that sits before the rename. Verified against the commit, not just the review note on this PR.

What is left here is the durability gap that #4833 did not close: materializeArtifact syncs the staging file and then renames it, but never syncs the directory whose entry the rename just created.

What was removed from this PR, and why

• The unlink removal — superseded by fix(desktop): preserve destination on artifact export failure #4833. On current main materializeArtifact already goes handle.sync() → handle.close() → rename() with nothing before it.

• The replaceDestination injection seam and its tests — main's Save As table already drives exactly that fault through the public IPC:

  ["rename", "replace_failed"],
  ["directory", "replace_failed"],

  with t.mock.method(fs, "rename", ...) + syncBuiltinESMExports(), asserting the destination still reads ORIGINAL. So the seam existed to re-cover ground fix(desktop): preserve destination on artifact export failure #4833 covered, and it did so by exporting a module-private function and adding a test-only sixth parameter — the P3 on this review. Both are gone; the diff is now production code only.

Change

     } catch (error) {
       throw new ArtifactMaterializationError("replace_failed", error);
     }
+    await syncDirectory(dirname(targetPath)).catch(() => undefined);

plus a module-private syncDirectory, shaped like the ones the codebase already carries (runtime-host-local-remote-access.ts:1385, and the packages/runtime-host stores): open(path, "r") → handle.sync() → close() in a finally, with the process.platform === "win32" early return. There is no shared helper to import — the convention here is one private copy per module, so this follows it rather than adding an eleventh variant's replacement.

Best-effort by design: the save has already landed when the sync runs, so a failing sync must not report a failed save.

Verification

biome check on the changed file	no findings
Diff	1 file, +18, no test changes
Traced all 9 Save As preserves the destination and reports … correctly cases against the new path	stream / total / length / rename / directory / write / sync / close all throw before the rename resolves, so syncDirectory is never reached; only none reaches it, and its throw is absorbed by .catch, so the { ok: true } assertion cannot flip
CI test on ed1b7fe06	green in 7m3s — job 106837150797. Select affected test surfaces planned this file, and all nine Save As preserves the destination … cases pass on the runner, along with Typecheck, Build, Knip (both workspaces), Desktop e2e, Lint and the architecture ledger

I could not execute the desktop unit tests locally, so the trace row above was written before any result existed and CI has now superseded it. The local blocker is repo-wide, not this change: @maka/ui cannot type-check because virtua@0.50.6 is declared in packages/ui/package.json but has no entry in package-lock.json (grep for node_modules/virtua: 0), so npm install never fetches it and build:workspace-deps stops there. npm run build:main additionally fails in files unrelated to this change. Worth its own issue.

What the green run does not establish: no test asserts the directory is synced, so no counterfactual exists — reverting these 18 lines leaves CI equally green. It proves nothing regressed, not that the fsync happens.

No test is added. The call is observable on Linux and skipped on Windows by design, so a meaningful assertion is platform-bound; the mockability the seam used to give is gone. Say the word and I will add a Linux-only case that mocks fs.open and asserts the directory handle is synced after a successful save, and that the save still reports ok: true when that sync throws.

Note on the previously red check

The test check that was failing on this PR was not this PR's failure: e2e/skill-draft-lifecycle.spec.ts was broken repo-wide and upstream fixed it in 29f13bdb3 (#5565) ten days after this branch was cut — its own message says the spec "fails on most branches right now, including main". Rebasing onto 6cb8c5808 picks that up (Desktop e2e is success there).

Review state

Both reviews on this PR self-identify as automated — me2seeks: "Automated review by OpenAI Codex … This is an automated technical assessment, not an independent human review" — and CONTRIBUTING.md requires an approving review from a committer other than the author, which an AI review does not satisfy. The approval is also recorded on 8191028fe, two heads back. Could a committer take a human look at the 18 lines?

Does this PR entail a change in behavior?

No for the user-visible path: a successful save behaves identically. The one difference is that after the rename lands, the destination's containing directory is fsynced on non-Windows platforms (and that a crash in the window before this sync could previously leave the old destination content plus a leftover staging file).

AI use

Select exactly one:

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

Tool(s) and scope: Qoder (AI coding assistant) authored the implementation and this description; the contributor of record directs the work and owns its accuracy, provenance, licensing and the merge decision. The tip commit carries Generated-by: Qoder (AI assistant), which is what a squash merge retains. This box was previously checked the other way, which was wrong — corrected while narrowing the scope.

Checklist

• Tests cover the change and fail without it — not satisfied. No test is added here; see Verification for why, and the offered Linux-only case.
• Lint and format pass locally — biome check on the changed file: no findings. Typecheck and the affected suites do not run on this host (missing virtua in the lockfile blocks @maka/ui), so CI is the verifier for those.
• Scope of this PR matches the linked issue / discussion — the original scope did not (superseded by fix(desktop): preserve destination on artifact export failure #4833), which is why it was narrowed.

[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.

[Adarsh-Me]

Both P3s are addressed at f18b7ce:

• Directory fsync: after a successful replacement, a best-effort syncDirectory(dirname(targetPath)) now runs — the same durability tier the other desktop write paths use (open dir r → sync, skipped on Windows where the handle cannot be opened this way and the rename already persists the entry). Failure of the sync does not fail the save, since the user's file is already on disk once the rename landed.
• Test-only seam: the materializeArtifact docs now state explicitly that both production call sites pass nothing (the default is rename), that the injected failure does not cover the default path, and that the default path is asserted only on success via the save-dialog happy-path test.

Desktop suite for the file passes 14/14 (including "A failed final replacement leaves the previous destination intact" and the happy-path replacement test). Also noted the gate comment on test never being scheduled here — leaving merge timing to the maintainers.

[Adarsh-Me]

Re-scoped and rebased this PR; doing that surfaced two things that change what it is.

1. The fix this PR opened with is already upstream. #4833 (6cd72229ae, "fix(desktop): preserve destination on artifact export failure") removes the same line:

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

I checked the commit itself rather than relying on the review note here, so the headline change was a duplicate. I also dropped the replaceDestination injection seam: main's Save As table already mocks fs.rename (through syncBuiltinESMExports()) and asserts the destination survives replace_failed, so the seam existed to re-cover that ground from production code — which is exactly the P3 raised on this review.

What remains is the part #4833 did not close: handle.sync() covers the staging file's content, not the directory entry the rename creates, so a crash just after a successful save can still show the old destination content next to a leftover staging file. 18 lines, production file only, with a module-private syncDirectory shaped like runtime-host-local-remote-access.ts:1385.

2. The red test check was not this PR's failure either. It was e2e/skill-draft-lifecycle.spec.ts:86, and this branch predates that spec's fix. From 29f13bdb3 (#5565), merged ten days after this branch was cut:

skill-draft-lifecycle fails on most branches right now, including main, with "Skill 调用失败，消息未发送" never becoming visible.

Rebased onto 6cb8c5808, where Desktop e2e is success. Before narrowing, git range-diff showed both original commits byte-identical on the new base, so nothing was re-resolved by hand.

What I have not done: run the desktop unit suite against this. @maka/ui cannot type-check on this host — virtua@0.50.6 is declared in packages/ui/package.json but has no entry in package-lock.json, so npm install never fetches it and build:workspace-deps stops there. That is a separate repo issue, not caused by this PR, but it means CI on ed1b7fe06 is the first real execution of the nine Save As preserves the destination and reports … correctly cases against this change. Tracing them by hand: only the none case reaches the new call, and a throw there is absorbed by .catch, so no assertion in that table can flip. I would rather wait for CI than claim a green run I do not have.

If a test for the sync itself is wanted, the shape is a Linux-only case asserting the directory handle is synced after a successful save, and that the save still returns ok: true when that sync throws. The injected seam was the portable-but-worse alternative, and it is gone deliberately.

@Astro-Han you merge most of the desktop work here and raised the P3s this responds to. Both reviews on this PR self-identify as automated, so it needs a human approval either way — it is 18 lines whenever you get to it.

[Adarsh-Me]

CI is green on ed1b7fe06 — test job 106837150797, 7m3s.

Worth recording what it actually executed, since the runner selects rather than runs everything:

step	result
Select affected test surfaces	CI test plan includes apps/desktop/src/main/runtime-host-artifacts-ipc-main.ts — so this file's suite was selected, not skipped
Run affected standard workspace tests	all nine Save As preserves the destination and reports … correctly cases pass (none, stream, total, length, rename, directory, write, sync, close)
Typecheck, Build	success — the gap that stopped this host was the missing virtua lockfile entry, not the change
Knip (apps/desktop)	success — relevant here, because the replaceDestination seam I removed is exactly a production export kept alive by a test
Desktop e2e	success, which is the step that was red before the rebase
Lint, Check formatting, Check renderer architecture	success

The nine passing cases match the hand trace in my previous comment: only the none case reaches the new syncDirectory call, the eight fault cases throw at or before the rename and return via the outer catch, and in the one case that does reach it a throw is absorbed.

What this run does not establish. There is no test here that asserts the directory is synced, so no counterfactual exists: reverting these 18 lines leaves CI exactly as green as it is now. The run proves nothing regressed; it does not prove the fsync happens. That is the real gap in this PR, and it is the Linux-only case offered above — say the word and it is a small follow-up commit.

[hqhq1025]

Reviewed commit ed1b7fe. The only PR change adds a best-effort directory sync after the completed staging-file rename in Desktop's artifact Save As path. The staging file is synced and closed before rename, and a directory-sync failure does not incorrectly turn a completed save into a reported failure. I found no substantiated P0–P3 issue in this change.

The current-head test check is successful, git diff --check is clean, and the branch merges cleanly with current main. I reviewed the write/rename/error path but did not run a real Desktop Save As flow, crash-durability test, or Windows/macOS filesystem test. The remaining platform/durability judgment is for a human maintainer.

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

[Astro-Han]

Approved at @Astro-Han's explicit request: a small, focused fix with no blocking findings in the automated review of this head and green CI.