fix(specialist): make overwrites atomic

docs/SPECIALISTS.md

@@ -138,3 +138,45 @@ output or stop a still-running task by id.
 If Zero is restarted while a background task is still marked `running`, the new
 manager marks that task `error` and clears its PID. This avoids sending
 `TaskStop` to a stale PID that may now belong to an unrelated process.
+
+## Recovering an Interrupted Overwrite
+
+Zero writes and flushes a complete temporary file before publishing an overwrite,
+so a write failure before publication leaves the existing manifest unchanged
+instead of truncating it. On Unix, publication uses a same-directory rename and
+preserves the existing file's permission bits.
+
+On Windows, Zero uses `ReplaceFileW` to preserve the destination DACL instead of
+silently replacing it with the temporary file's inherited DACL. `ReplaceFileW`
+is not observer-atomic: another process can briefly observe the destination path
+as absent during replacement. Zero serializes specialist loads and managed
+mutations within one process, but cannot synchronize external processes or
+editors.
+
+Windows errors 1176 (`ERROR_UNABLE_TO_MOVE_REPLACEMENT`) and 1177
+(`ERROR_UNABLE_TO_MOVE_REPLACEMENT_2`) are partial replacement failures. With
+Zero's managed backup, 1176 leaves the original names intact and needs no manual
+recovery. For 1177, Zero has moved the original aside and tries to move it back.
+That rollback almost always succeeds, and the failed write changes nothing.
+
+If the rollback itself fails — typically because another process is holding a
+lock on the file — the original is not lost, but it is left under a name Zero
+does not read:
+
+```text
+<specialist dir>/.zero-replace-<random>.backup
+```
+
+Only `*.md` files are loaded as specialists, so until that file is renamed the
+specialist will not appear in `zero specialist list` or resolve by name. Zero's
+error message includes both the backup path and destination path. Recover by
+closing whatever holds the lock and renaming the backup back:
+
+```powershell
+Move-Item .zero-replace-<random>.backup <name>.md
+```
+
+A `.zero-replace-*.backup` can also linger after a *successful* overwrite if the
+backup could not be deleted afterward. That case is reported as a warning rather
+than an error — the new manifest is already in place, and the leftover file is
+safe to delete.

internal/cli/specialist.go

@@ -286,7 +286,11 @@ func runSpecialistCreate(paths specialist.Paths, name string, options specialist
 	if err != nil {
 		return writeExecUsageError(stderr, err.Error())
 	}
-	if options.json {
+	return writeSpecialistCreateResult(manifest, options.json, stdout, stderr)
+}
+
+func writeSpecialistCreateResult(manifest specialist.Manifest, jsonOutput bool, stdout io.Writer, stderr io.Writer) int {
+	if jsonOutput {
 		if err := writePrettyJSON(stdout, manifest); err != nil {
 			return exitCrash
 		}
@@ -295,6 +299,11 @@ func runSpecialistCreate(paths specialist.Paths, name string, options specialist
 	if _, err := fmt.Fprintf(stdout, "Created specialist %s at %s\n", manifest.Metadata.Name, manifest.FilePath); err != nil {
 		return exitCrash
 	}
+	for _, warning := range manifest.Warnings {
+		if _, err := fmt.Fprintf(stderr, "warning: %s\n", warning); err != nil {
+			return exitCrash
+		}
+	}
 	return exitSuccess
 }
 

internal/cli/specialist_test.go

@@ -10,6 +10,7 @@ import (
 	"testing"
 
 	"github.com/Gitlawb/zero/internal/config"
+	"github.com/Gitlawb/zero/internal/specialist"
 )
 
 func TestRunSpecialistListShowAndPath(t *testing.T) {
@@ -187,6 +188,42 @@ func TestRunSpecialistCreateDeleteAndEdit(t *testing.T) {
 	}
 }
 
+func TestWriteSpecialistCreateResultSurfacesWarningsOnlyForHumanOutput(t *testing.T) {
+	manifest := specialist.Manifest{
+		Metadata: specialist.Metadata{Name: "triage"},
+		FilePath: "/specialists/triage.md",
+		Warnings: []string{"replacement backup /specialists/.triage.bak retained: access denied"},
+	}
+
+	var stdout bytes.Buffer
+	var stderr bytes.Buffer
+	if code := writeSpecialistCreateResult(manifest, false, &stdout, &stderr); code != exitSuccess {
+		t.Fatalf("human output exit code = %d", code)
+	}
+	if got, want := stdout.String(), "Created specialist triage at /specialists/triage.md\n"; got != want {
+		t.Fatalf("stdout = %q, want %q", got, want)
+	}
+	if got, want := stderr.String(), "warning: replacement backup /specialists/.triage.bak retained: access denied\n"; got != want {
+		t.Fatalf("stderr = %q, want %q", got, want)
+	}
+
+	stdout.Reset()
+	stderr.Reset()
+	if code := writeSpecialistCreateResult(manifest, true, &stdout, &stderr); code != exitSuccess {
+		t.Fatalf("JSON output exit code = %d", code)
+	}
+	var payload specialist.Manifest
+	if err := json.Unmarshal(stdout.Bytes(), &payload); err != nil {
+		t.Fatalf("decode JSON: %v\n%s", err, stdout.String())
+	}
+	if len(payload.Warnings) != 1 || payload.Warnings[0] != manifest.Warnings[0] {
+		t.Fatalf("JSON warnings = %#v", payload.Warnings)
+	}
+	if stderr.Len() != 0 {
+		t.Fatalf("JSON warning duplicated to stderr: %q", stderr.String())
+	}
+}
+
 func TestRunSpecialistEditRejectsSymlink(t *testing.T) {
 	if runtime.GOOS == "windows" {
 		t.Skip("symlink creation needs extra privileges on Windows")

internal/fsutil/rename.go

@@ -3,12 +3,47 @@ package fsutil
 
 import (
 	"errors"
+	"fmt"
 	"os"
 	"runtime"
 	"syscall"
 	"time"
 )
 
+// CommittedReplacementCleanupError reports that a replacement was committed,
+// but the old destination retained at BackupPath could not be removed. Callers
+// must treat the replacement itself as successful and surface the cleanup
+// problem separately.
+type CommittedReplacementCleanupError struct {
+	BackupPath string
+	Cause      error
+}
+
+func (err *CommittedReplacementCleanupError) Error() string {
+	return fmt.Sprintf("replacement committed, but backup %s could not be removed: %v", err.BackupPath, err.Cause)
+}
+
+func (err *CommittedReplacementCleanupError) Unwrap() error {
+	return err.Cause
+}
+
+// ReplaceWithRetry publishes src over dst using the platform's replacement
+// primitive, retrying on the same transient Windows lock errors RenameWithRetry
+// handles. On Windows it uses ReplaceFileW so an existing destination keeps its
+// DACL and selected metadata instead of receiving the temporary file's inherited
+// DACL. ReplaceFileW is not observer-atomic and may briefly leave dst absent;
+// callers that cannot tolerate that must synchronize their own readers. On Unix
+// replacement uses os.Rename.
+//
+// replace overrides the platform primitive so tests can exercise the retry path;
+// pass nil for the default.
+func ReplaceWithRetry(src, dst string, replace func(src, dst string) error) error {
+	if replace == nil {
+		replace = replaceExisting
+	}
+	return RenameWithRetry(src, dst, replace)
+}
+
 // RenameWithRetry renames src to dst, retrying briefly on Windows when the
 // destination is transiently locked (antivirus scanners, search indexers, or
 // a concurrent reader holding the file open). rename overrides os.Rename so
@@ -23,6 +58,12 @@ func RenameWithRetry(src, dst string, rename func(src, dst string) error) error
 		if err == nil {
 			return nil
 		}
+		var committed *CommittedReplacementCleanupError
+		if errors.As(err, &committed) {
+			// The source has already been consumed. In particular, never retry if
+			// the cleanup cause itself is a transient Windows sharing violation.
+			break
+		}
 		if runtime.GOOS == "windows" {
 			if os.IsPermission(err) || isWindowsSharingOrLockViolation(err) {
 				time.Sleep(10 * time.Millisecond)

internal/fsutil/replace_other.go

@@ -0,0 +1,12 @@
+//go:build !windows
+
+package fsutil
+
+import "os"
+
+// replaceExisting publishes src over dst. rename(2) already replaces the
+// destination atomically within one filesystem on Unix, and it neither creates
+// nor consults an ACL, so there is nothing extra to preserve here.
+func replaceExisting(src, dst string) error {
+	return os.Rename(src, dst)
+}

internal/fsutil/replace_other_test.go

@@ -0,0 +1,50 @@
+//go:build !windows
+
+package fsutil
+
+import (
+	"os"
+	"path/filepath"
+	"testing"
+)
+
+// On Unix the replacement primitive is rename(2), which already publishes
+// atomically within one filesystem and neither creates nor consults an ACL. These
+// cover both shapes so the shared helper is exercised on every platform.
+func TestReplaceWithRetryPublishesOverExistingAndMissingDestinations(t *testing.T) {
+	for _, tc := range []struct {
+		name     string
+		existing bool
+	}{
+		{name: "existing destination", existing: true},
+		{name: "missing destination"},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			dir := t.TempDir()
+			src := filepath.Join(dir, ".manifest.tmp")
+			dst := filepath.Join(dir, "manifest.md")
+			if err := os.WriteFile(src, []byte("new"), 0o600); err != nil {
+				t.Fatalf("WriteFile src: %v", err)
+			}
+			if tc.existing {
+				if err := os.WriteFile(dst, []byte("old"), 0o600); err != nil {
+					t.Fatalf("WriteFile dst: %v", err)
+				}
+			}
+
+			if err := ReplaceWithRetry(src, dst, nil); err != nil {
+				t.Fatalf("ReplaceWithRetry: %v", err)
+			}
+			data, err := os.ReadFile(dst)
+			if err != nil {
+				t.Fatalf("ReadFile dst: %v", err)
+			}
+			if string(data) != "new" {
+				t.Fatalf("destination content = %q, want the replacement bytes", data)
+			}
+			if _, err := os.Lstat(src); !os.IsNotExist(err) {
+				t.Fatalf("the replacement file should be consumed by the replace: %v", err)
+			}
+		})
+	}
+}