diff --git a/docs/SPECIALISTS.md b/docs/SPECIALISTS.md index 7435a56e0..5ef27b714 100644 --- a/docs/SPECIALISTS.md +++ b/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 +/.zero-replace-.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-.backup .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. diff --git a/internal/cli/specialist.go b/internal/cli/specialist.go index f77887714..b5014d6ef 100644 --- a/internal/cli/specialist.go +++ b/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 } diff --git a/internal/cli/specialist_test.go b/internal/cli/specialist_test.go index cd20c22f8..ab01e22e8 100644 --- a/internal/cli/specialist_test.go +++ b/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") diff --git a/internal/fsutil/rename.go b/internal/fsutil/rename.go index ace33502a..4f74e8544 100644 --- a/internal/fsutil/rename.go +++ b/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) diff --git a/internal/fsutil/replace_other.go b/internal/fsutil/replace_other.go new file mode 100644 index 000000000..35a985518 --- /dev/null +++ b/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) +} diff --git a/internal/fsutil/replace_other_test.go b/internal/fsutil/replace_other_test.go new file mode 100644 index 000000000..9c53c9225 --- /dev/null +++ b/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) + } + }) + } +} diff --git a/internal/fsutil/replace_windows.go b/internal/fsutil/replace_windows.go new file mode 100644 index 000000000..7cf6be44c --- /dev/null +++ b/internal/fsutil/replace_windows.go @@ -0,0 +1,237 @@ +//go:build windows + +package fsutil + +import ( + "errors" + "fmt" + "os" + "path/filepath" + "syscall" + "time" + "unsafe" +) + +// replaceFileFlags is deliberately ZERO. Every REPLACEFILE_* flag ReplaceFileW +// accepts either defeats the reason this function exists or does nothing: +// +// - REPLACEFILE_IGNORE_MERGE_ERRORS (0x2) and REPLACEFILE_IGNORE_ACL_ERRORS +// (0x4). Microsoft documents BOTH with the same consequence: "if you specify +// this flag and do not have WRITE_DAC access, the function succeeds but the +// ACLs are not preserved." A silent success that publishes the temporary +// file's inherited directory DACL over an explicitly restricted specialist — +// exposing its system prompt — is precisely the failure this function exists +// to prevent, so a merge failure MUST surface as an error and leave the +// destination untouched. Passing 0x2 while omitting only 0x4 buys nothing: +// ACL merging is part of the metadata merge 0x2 covers. +// - REPLACEFILE_WRITE_THROUGH (0x1) is documented as "This value is not +// supported", so it cannot be relied on to flush anything. +const replaceFileFlags = 0 + +const replaceBackupPattern = ".zero-replace-*.backup" + +const ( + // Returned when the volume or redirector cannot provide ReplaceFileW's + // DACL-preserving semantics. Existing destinations must fail closed. + errorInvalidFunction = syscall.Errno(1) + errorNotSupported = syscall.Errno(50) + + // Partial-failure codes: ReplaceFileW got far enough to move or delete + // something, so the on-disk state needs repair rather than a bare error. See + // recoverPartialReplace. + errorUnableToRemoveReplaced = syscall.Errno(1175) + errorUnableToMoveReplacement = syscall.Errno(1176) + errorUnableToMoveReplacement2 = syscall.Errno(1177) +) + +var ( + replaceKernel32 = syscall.NewLazyDLL("kernel32.dll") + replaceProcReplaceFil = replaceKernel32.NewProc("ReplaceFileW") +) + +// replaceExisting publishes src over dst with ReplaceFileW rather than +// MoveFileEx (what os.Rename uses) to preserve destination metadata. The +// replacement is a freshly created temporary file, so it carries the directory's +// inherited DACL. Renaming it over the destination would therefore replace the +// destination's ACL - silently widening access to a file that had been restricted +// explicitly (os.File.Chmod cannot express that on Windows; Go only maps the +// owner-write bit). ReplaceFileW carries the replaced file's DACL and selected +// metadata over to the replacement instead. +// +// ReplaceFileW combines multiple filesystem steps and is not observer-atomic: an +// external reader can briefly see dst absent even when replacement succeeds. +// Callers that cannot tolerate that window must synchronize their own readers. +// +// No REPLACEFILE_* flag is passed at all — see replaceFileFlags for why each one +// would either silently lose the descriptor this function exists to preserve or +// do nothing. A merge failure therefore surfaces as an error, leaving the +// destination untouched and the caller free to clean up its temporary file, +// except in the partial-failure states recoverPartialReplace repairs. +func replaceExisting(src, dst string) error { + return replaceExistingWith(src, dst, callReplaceFile, nil) +} + +func replaceExistingWith(src, dst string, replace func(string, string, string) error, restore func(string, string) error) error { + return replaceExistingWithCleanup(src, dst, replace, restore, removeReplaceBackup) +} + +func replaceExistingWithCleanup(src, dst string, replace func(string, string, string) error, restore func(string, string) error, removeBackup func(string) error) error { + info, err := os.Lstat(dst) + if err != nil { + if os.IsNotExist(err) { + // Nothing to replace and no descriptor to preserve. + return os.Rename(src, dst) + } + return err + } + if info.Mode()&os.ModeSymlink != 0 { + return fmt.Errorf("refusing to replace symlink destination: %s", dst) + } + + backup, err := prepareReplaceBackup(dst) + if err != nil { + return fmt.Errorf("prepare replacement backup: %w", err) + } + callErr := replace(dst, src, backup) + if callErr == nil { + if err := removeBackup(backup); err != nil { + return &CommittedReplacementCleanupError{BackupPath: backup, Cause: err} + } + return nil + } + if errors.Is(callErr, errorInvalidFunction) || errors.Is(callErr, errorNotSupported) { + callErr = fmt.Errorf("DACL-preserving replacement is not supported for %s: %w", dst, callErr) + } + return recoverPartialReplace(callErr, dst, backup, restore) +} + +func callReplaceFile(replacedPath, replacementPath, backupPath string) error { + replaced, err := syscall.UTF16PtrFromString(replacedPath) + if err != nil { + return err + } + replacement, err := syscall.UTF16PtrFromString(replacementPath) + if err != nil { + return err + } + backup, err := syscall.UTF16PtrFromString(backupPath) + if err != nil { + return err + } + result, _, callErr := replaceProcReplaceFil.Call( + uintptr(unsafe.Pointer(replaced)), + uintptr(unsafe.Pointer(replacement)), + uintptr(unsafe.Pointer(backup)), + uintptr(replaceFileFlags), + 0, + 0, + ) + if result != 0 { + return nil + } + if callErr == nil || errors.Is(callErr, syscall.Errno(0)) { + return fmt.Errorf("replace %s: ReplaceFileW failed", replacedPath) + } + return callErr +} + +func prepareReplaceBackup(dst string) (string, error) { + file, err := os.CreateTemp(filepath.Dir(dst), replaceBackupPattern) + if err != nil { + return "", err + } + path := file.Name() + if err := file.Close(); err != nil { + cleanupErr := removeReplaceBackup(path) + return "", fmt.Errorf("close backup placeholder %s: %w (cleanup error: %v)", path, err, cleanupErr) + } + if err := removeReplaceBackup(path); err != nil { + return "", fmt.Errorf("release backup path %s: %w", path, err) + } + return path, nil +} + +func removeReplaceBackup(path string) error { + var err error + for i := 0; i < 10; i++ { + err = os.Remove(path) + if err == nil || os.IsNotExist(err) { + return nil + } + if os.IsPermission(err) { + // ReplaceFileW can leave the original's read-only attribute on the + // backup. Chmod clears that bit on Windows without changing its DACL. + _ = os.Chmod(path, 0o600) + } + if os.IsPermission(err) || isWindowsSharingOrLockViolation(err) { + time.Sleep(10 * time.Millisecond) + continue + } + break + } + return err +} + +func cleanupReplaceBackup(callErr error, backup string) error { + if err := removeReplaceBackup(backup); err != nil { + return fmt.Errorf("%w (backup %s could not be removed: %v)", callErr, backup, err) + } + return callErr +} + +// recoverPartialReplace restores the original after ReplaceFileW reports a +// partial failure, then returns the original error. Supplying a managed backup +// changes the dangerous failure states documented by Microsoft: +// +// - ERROR_UNABLE_TO_MOVE_REPLACEMENT (1176) leaves both files under their +// original names, so the failed write can discard src normally. +// - ERROR_UNABLE_TO_MOVE_REPLACEMENT_2 (1177) leaves src in place and moves the +// original destination to backup. Moving backup back to dst rolls the failed +// overwrite back without losing the original content or its DACL. +// +// The replacement is left alone either way — nothing here deletes what it was +// handed — but the errors below deliberately say nothing about where it ended up, +// which is also why its path is not a parameter. Whether it still exists when the +// error surfaces is the caller's business: the specialist writer, the only caller +// today, removes its temporary file in a deferred cleanup, so an error promising +// a replacement "remains at" that path would send an operator looking for a file +// that was already gone. What these errors do describe is what this function +// itself left on disk — the state of dst, and of the managed backup holding the +// original. +func recoverPartialReplace(callErr error, dst, backup string, restore func(string, string) error) error { + if !errors.Is(callErr, errorUnableToMoveReplacement2) { + // For 1175, 1176, unsupported volumes, and ordinary failures, Windows + // documents that dst remains at its original name. Remove any redundant + // backup only after confirming that the destination still exists. + if _, err := os.Lstat(dst); err == nil { + return cleanupReplaceBackup(callErr, backup) + } else if !os.IsNotExist(err) { + return fmt.Errorf("replace %s: %w (inspect destination during recovery: %v)", dst, callErr, err) + } + } + + if _, err := os.Lstat(backup); err != nil { + return fmt.Errorf("replace %s: %w (original backup expected at %s is unavailable: %v)", dst, callErr, backup, err) + } + if _, err := os.Lstat(dst); err == nil { + return fmt.Errorf("replace %s: %w (destination unexpectedly exists; the original also remains at backup %s)", dst, callErr, backup) + } else if !os.IsNotExist(err) { + return fmt.Errorf("replace %s: %w (inspect destination before restoring backup %s: %v)", dst, callErr, backup, err) + } + if err := RenameWithRetry(backup, dst, restore); err != nil { + // Only callErr is wrapped. Exposing a sharing violation from the exhausted + // rollback would make the outer ReplaceWithRetry call retry after the + // filesystem was already mutated. + // + // This is the one terminal state that needs a human: nothing is at dst, and + // the original's only copy is under a name no tool looks for (the specialist + // loader reads *.md and skips everything else), so it is invisible until + // somebody moves it back. Say that outright rather than leaving an operator + // to infer it from a bare error code. + return fmt.Errorf( + "replace %s: %w (rolling the original back failed: %v; %s no longer exists and the original survives only as %s, which must be moved back by hand to restore it)", + dst, callErr, err, dst, backup, + ) + } + return fmt.Errorf("replace %s: %w (the original was restored)", dst, callErr) +} diff --git a/internal/fsutil/replace_windows_test.go b/internal/fsutil/replace_windows_test.go new file mode 100644 index 000000000..3455540d0 --- /dev/null +++ b/internal/fsutil/replace_windows_test.go @@ -0,0 +1,559 @@ +//go:build windows + +package fsutil + +import ( + "errors" + "os" + "path/filepath" + "strings" + "syscall" + "testing" + + "golang.org/x/sys/windows" +) + +// TestReplaceWithRetryPreservesDestinationCreationTime proves the publish goes +// through ReplaceFileW and not os.Rename: ReplaceFileW carries selected metadata, +// including creation time, from the destination to the replacement. A rename +// would carry the temporary file's creation time over instead. +func TestReplaceWithRetryPreservesDestinationCreationTime(t *testing.T) { + dir := t.TempDir() + dst := filepath.Join(dir, "manifest.md") + if err := os.WriteFile(dst, []byte("old"), 0o600); err != nil { + t.Fatalf("WriteFile dst: %v", err) + } + created := creationTime(t, dst) + + src := filepath.Join(dir, ".manifest.tmp") + if err := os.WriteFile(src, []byte("new"), 0o600); err != nil { + t.Fatalf("WriteFile src: %v", err) + } + // Push the replacement's own creation time clearly past the destination's so a + // rename would be visible in the comparison below. + future := windows.NsecToFiletime(created.Nanoseconds() + int64(10*1e9)) + setCreationTime(t, src, future) + + 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) + } + assertNoReplaceBackups(t, dir) + if got := creationTime(t, dst); got != created { + t.Fatalf("creation time = %v, want the replaced file's %v (a rename would not preserve it)", got, created) + } +} + +// TestReplaceWithRetryPreservesDestinationDACL is the regression test for the +// second half of the finding: the replacement is a freshly created temporary file +// carrying the directory's inherited DACL, so publishing it with a rename would +// REPLACE the restrictive descriptor an explicitly locked-down file had. +func TestReplaceWithRetryPreservesDestinationDACL(t *testing.T) { + dir := t.TempDir() + dst := filepath.Join(dir, "manifest.md") + if err := os.WriteFile(dst, []byte("old"), 0o600); err != nil { + t.Fatalf("WriteFile dst: %v", err) + } + // A protected DACL granting only the owner: distinct from whatever the temp + // file inherits from the directory. + restricted, err := windows.SecurityDescriptorFromString("D:P(A;;FA;;;OW)") + if err != nil { + t.Skipf("cannot build a test security descriptor: %v", err) + } + dacl, _, err := restricted.DACL() + if err != nil { + t.Skipf("cannot read the test DACL: %v", err) + } + if err := windows.SetNamedSecurityInfo( + dst, + windows.SE_FILE_OBJECT, + windows.DACL_SECURITY_INFORMATION|windows.PROTECTED_DACL_SECURITY_INFORMATION, + nil, nil, dacl, nil, + ); err != nil { + t.Skipf("cannot apply a restrictive DACL on this filesystem: %v", err) + } + want := describeDACL(t, dst) + + src := filepath.Join(dir, ".manifest.tmp") + if err := os.WriteFile(src, []byte("new"), 0o600); err != nil { + t.Fatalf("WriteFile src: %v", err) + } + if inherited := describeDACL(t, src); inherited == want { + t.Skip("the temporary file already carries the same DACL; this filesystem cannot show the difference") + } + + if err := ReplaceWithRetry(src, dst, nil); err != nil { + t.Fatalf("ReplaceWithRetry: %v", err) + } + if got := describeDACL(t, dst); got != want { + t.Fatalf("DACL after replace = %q, want the destination's own %q", got, want) + } + assertNoReplaceBackups(t, dir) +} + +// TestReplaceWithRetryPublishesWhenDestinationIsMissing covers the no-destination +// case: ReplaceFileW requires an existing file to replace, so there is a rename +// fallback (and nothing to preserve). +func TestReplaceWithRetryPublishesWhenDestinationIsMissing(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 err := ReplaceWithRetry(src, dst, nil); err != nil { + t.Fatalf("ReplaceWithRetry: %v", err) + } + if data, err := os.ReadFile(dst); err != nil || string(data) != "new" { + t.Fatalf("destination = %q err=%v, want the replacement bytes", data, err) + } + assertNoReplaceBackups(t, dir) +} + +// TestReplaceWithRetryRetriesTransientLockViolation keeps the retry behavior that +// RenameWithRetry provides for antivirus/indexer holds. +func TestReplaceWithRetryRetriesTransientLockViolation(t *testing.T) { + attempts := 0 + err := ReplaceWithRetry("src", "dst", func(src, dst string) error { + attempts++ + if attempts < 3 { + return &os.PathError{Op: "replace", Path: dst, Err: syscall.Errno(32)} // ERROR_SHARING_VIOLATION + } + return nil + }) + if err != nil { + t.Fatalf("ReplaceWithRetry: %v", err) + } + if attempts != 3 { + t.Fatalf("attempts = %d, want the transient violations retried", attempts) + } +} + +// TestReplaceFileFlagsDoNotIgnoreMergeErrors is the regression test for jatmn's +// #757 P1 finding on flags: the call used to pass REPLACEFILE_IGNORE_MERGE_ERRORS +// (0x2) while the comment claimed ACL failures were fail-closed. Microsoft +// documents 0x2 and REPLACEFILE_IGNORE_ACL_ERRORS (0x4) identically — with either +// one set, a call lacking WRITE_DAC "succeeds but the ACLs are not preserved" — +// so passing 0x2 let a --force overwrite silently publish the temporary file's +// inherited directory DACL over a restricted specialist and expose its system +// prompt. +// +// This asserts the flag word directly rather than a live denied merge because a +// denied merge is not constructible for a file this process owns: Windows grants +// an object's owner READ_CONTROL and WRITE_DAC implicitly, so no DACL a test can +// apply to its own temp file can withhold WRITE_DAC from it. Pinning the flags is +// what actually prevents the regression — re-adding either bit fails here. +func TestReplaceFileFlagsDoNotIgnoreMergeErrors(t *testing.T) { + const ( + ignoreMergeErrors = 0x00000002 + ignoreACLErrors = 0x00000004 + ) + if replaceFileFlags&ignoreMergeErrors != 0 { + t.Error("REPLACEFILE_IGNORE_MERGE_ERRORS must not be set: it makes ReplaceFileW succeed WITHOUT preserving ACLs when it cannot obtain WRITE_DAC") + } + if replaceFileFlags&ignoreACLErrors != 0 { + t.Error("REPLACEFILE_IGNORE_ACL_ERRORS must not be set: it makes ReplaceFileW succeed WITHOUT preserving ACLs when it cannot obtain WRITE_DAC") + } +} + +func TestReplaceExistingRejectsUnsupportedDACLReplacement(t *testing.T) { + for _, tc := range []struct { + name string + code syscall.Errno + }{ + {name: "ERROR_INVALID_FUNCTION", code: errorInvalidFunction}, + {name: "ERROR_NOT_SUPPORTED", code: errorNotSupported}, + } { + 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 err := os.WriteFile(dst, []byte("old"), 0o600); err != nil { + t.Fatalf("WriteFile dst: %v", err) + } + + err := replaceExistingWith(src, dst, func(replaced, replacement, backup string) error { + if replaced != dst || replacement != src { + t.Fatalf("ReplaceFileW paths = (%q, %q), want (%q, %q)", replaced, replacement, dst, src) + } + if filepath.Dir(backup) != dir { + t.Fatalf("backup directory = %q, want sibling directory %q", filepath.Dir(backup), dir) + } + return tc.code + }, nil) + if !errors.Is(err, tc.code) { + t.Fatalf("error = %v, want unsupported error %v", err, tc.code) + } + if !strings.Contains(err.Error(), "DACL-preserving replacement is not supported") { + t.Fatalf("error = %v, want a DACL-preserving-replacement explanation", err) + } + assertFileContent(t, dst, "old") + assertFileContent(t, src, "new") + assertNoReplaceBackups(t, dir) + }) + } +} + +func TestReplaceExistingCleansManagedBackup(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 err := os.WriteFile(dst, []byte("old"), 0o600); err != nil { + t.Fatalf("WriteFile dst: %v", err) + } + + var backupPath string + err := replaceExistingWith(src, dst, func(replaced, replacement, backup string) error { + backupPath = backup + if matched, matchErr := filepath.Match(replaceBackupPattern, filepath.Base(backup)); matchErr != nil || !matched { + t.Fatalf("backup path = %q, want pattern %q (match error: %v)", backup, replaceBackupPattern, matchErr) + } + if _, statErr := os.Lstat(backup); !os.IsNotExist(statErr) { + t.Fatalf("backup path must be vacant before ReplaceFileW: %v", statErr) + } + if renameErr := os.Rename(replaced, backup); renameErr != nil { + t.Fatalf("stage backup: %v", renameErr) + } + // Windows refuses to remove a read-only file. The cleanup path must clear + // that attribute without changing the backup's DACL, then remove it. + if chmodErr := os.Chmod(backup, 0o400); chmodErr != nil { + t.Fatalf("make backup read-only: %v", chmodErr) + } + return os.Rename(replacement, replaced) + }, nil) + if err != nil { + t.Fatalf("replaceExistingWith: %v", err) + } + assertFileContent(t, dst, "new") + if _, err := os.Lstat(src); !os.IsNotExist(err) { + t.Fatalf("replacement source should be consumed: %v", err) + } + if _, err := os.Lstat(backupPath); !os.IsNotExist(err) { + t.Fatalf("managed backup should be removed: %v", err) + } + assertNoReplaceBackups(t, dir) +} + +func TestReplaceWithRetryDoesNotRetryCommittedReplacementWhenBackupCleanupFails(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.Fatal(err) + } + if err := os.WriteFile(dst, []byte("old"), 0o600); err != nil { + t.Fatal(err) + } + + attempts := 0 + cleanupErr := &os.PathError{Op: "remove", Path: "managed backup", Err: syscall.Errno(32)} + var backupPath string + err := ReplaceWithRetry(src, dst, func(src, dst string) error { + attempts++ + return replaceExistingWithCleanup(src, dst, func(replaced, replacement, backup string) error { + backupPath = backup + if err := os.Rename(replaced, backup); err != nil { + return err + } + return os.Rename(replacement, replaced) + }, nil, func(got string) error { + if got != backupPath { + t.Fatalf("cleanup backup path = %q, want %q", got, backupPath) + } + return cleanupErr + }) + }) + var outcome *CommittedReplacementCleanupError + if !errors.As(err, &outcome) { + t.Fatalf("error = %v, want committed cleanup outcome", err) + } + if outcome.BackupPath != backupPath || !errors.Is(outcome, syscall.Errno(32)) { + t.Fatalf("outcome = %#v, want backup %q and sharing violation", outcome, backupPath) + } + if attempts != 1 { + t.Fatalf("replacement attempts = %d, want 1", attempts) + } + assertFileContent(t, dst, "new") + assertFileContent(t, backupPath, "old") +} + +func TestReplaceExistingKeeps1176FilesAtOriginalNames(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 err := os.WriteFile(dst, []byte("old"), 0o600); err != nil { + t.Fatalf("WriteFile dst: %v", err) + } + + err := replaceExistingWith(src, dst, func(_, _, _ string) error { + // With a backup name, Microsoft documents that 1176 leaves the replaced + // and replacement files under their original names. + return errorUnableToMoveReplacement + }, nil) + if !errors.Is(err, errorUnableToMoveReplacement) { + t.Fatalf("error = %v, want %v", err, errorUnableToMoveReplacement) + } + assertFileContent(t, dst, "old") + assertFileContent(t, src, "new") + assertNoReplaceBackups(t, dir) +} + +func TestReplaceExistingRollsBack1177AndRetriesTransientRestore(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 err := os.WriteFile(dst, []byte("old"), 0o600); err != nil { + t.Fatalf("WriteFile dst: %v", err) + } + + var backupPath string + restoreAttempts := 0 + err := replaceExistingWith(src, dst, func(replaced, _, backup string) error { + backupPath = backup + if renameErr := os.Rename(replaced, backup); renameErr != nil { + t.Fatalf("stage documented 1177 backup: %v", renameErr) + } + return errorUnableToMoveReplacement2 + }, func(backup, destination string) error { + restoreAttempts++ + if restoreAttempts < 3 { + return &os.PathError{Op: "rename", Path: backup, Err: syscall.Errno(32)} + } + return os.Rename(backup, destination) + }) + if !errors.Is(err, errorUnableToMoveReplacement2) { + t.Fatalf("error = %v, want %v", err, errorUnableToMoveReplacement2) + } + if restoreAttempts != 3 { + t.Fatalf("restore attempts = %d, want transient lock failures retried", restoreAttempts) + } + assertErrorDoesNotAdvertiseReplacement(t, err, src) + assertFileContent(t, dst, "old") + assertFileContent(t, src, "new") + if _, err := os.Lstat(backupPath); !os.IsNotExist(err) { + t.Fatalf("backup should be consumed by rollback: %v", err) + } + assertNoReplaceBackups(t, dir) +} + +func TestReplaceExistingPreserves1177BackupWhenRollbackFails(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 err := os.WriteFile(dst, []byte("old"), 0o600); err != nil { + t.Fatalf("WriteFile dst: %v", err) + } + + var backupPath string + replaceAttempts := 0 + err := ReplaceWithRetry(src, dst, func(src, dst string) error { + replaceAttempts++ + return replaceExistingWith(src, dst, func(replaced, _, backup string) error { + backupPath = backup + if renameErr := os.Rename(replaced, backup); renameErr != nil { + t.Fatalf("stage documented 1177 backup: %v", renameErr) + } + return errorUnableToMoveReplacement2 + }, func(_, _ string) error { + return &os.PathError{Op: "rename", Path: backupPath, Err: syscall.Errno(32)} + }) + }) + if !errors.Is(err, errorUnableToMoveReplacement2) { + t.Fatalf("error = %v, want %v", err, errorUnableToMoveReplacement2) + } + if replaceAttempts != 1 { + t.Fatalf("replace attempts = %d, want no retry after a partial failure", replaceAttempts) + } + if os.IsPermission(err) || isWindowsSharingOrLockViolation(err) { + t.Fatalf("partial-failure error exposes the rollback lock error to the outer retry loop: %v", err) + } + if !strings.Contains(err.Error(), backupPath) { + t.Fatalf("error = %v, want retained backup path %q", err, backupPath) + } + // The terminal state an operator has to fix by hand: the original exists only + // under the backup name and nothing is at dst, so the error has to name both + // and say what to do, not just report the Windows code. + if !strings.Contains(err.Error(), dst) { + t.Fatalf("error = %v, want the destination %q an operator must restore to", err, dst) + } + if !strings.Contains(err.Error(), "moved back by hand") { + t.Fatalf("error = %v, want an explicit instruction to move the backup back", err) + } + assertErrorDoesNotAdvertiseReplacement(t, err, src) + if _, err := os.Lstat(dst); !os.IsNotExist(err) { + t.Fatalf("destination should remain absent after failed rollback: %v", err) + } + assertFileContent(t, backupPath, "old") + assertFileContent(t, src, "new") +} + +func TestReplaceExistingRejectsSymlinkDestination(t *testing.T) { + dir := t.TempDir() + target := filepath.Join(dir, "target.md") + dst := filepath.Join(dir, "manifest.md") + src := filepath.Join(dir, ".manifest.tmp") + if err := os.WriteFile(target, []byte("old"), 0o600); err != nil { + t.Fatalf("WriteFile target: %v", err) + } + if err := os.WriteFile(src, []byte("new"), 0o600); err != nil { + t.Fatalf("WriteFile src: %v", err) + } + if err := os.Symlink(target, dst); err != nil { + t.Skipf("symlink unavailable: %v", err) + } + + called := false + err := replaceExistingWith(src, dst, func(_, _, _ string) error { + called = true + return nil + }, nil) + if err == nil || !strings.Contains(err.Error(), "refusing to replace symlink") { + t.Fatalf("symlink replacement error = %v", err) + } + if called { + t.Fatal("ReplaceFileW callback was called for a symlink destination") + } + assertFileContent(t, target, "old") + assertFileContent(t, src, "new") + if info, err := os.Lstat(dst); err != nil || info.Mode()&os.ModeSymlink == 0 { + t.Fatalf("destination symlink was changed: info=%v err=%v", info, err) + } + assertNoReplaceBackups(t, dir) +} + +func TestRecoverPartialReplaceLeavesIntactStatesAlone(t *testing.T) { + for _, tc := range []struct { + name string + code syscall.Errno + }{ + {name: "ERROR_UNABLE_TO_REMOVE_REPLACED", code: errorUnableToRemoveReplaced}, + {name: "ERROR_ACCESS_DENIED", code: syscall.Errno(5)}, + } { + t.Run(tc.name, func(t *testing.T) { + dir := t.TempDir() + src := filepath.Join(dir, ".manifest.tmp") + dst := filepath.Join(dir, "manifest.md") + backup := filepath.Join(dir, ".zero-replace-test.backup") + if err := os.WriteFile(src, []byte("new"), 0o600); err != nil { + t.Fatalf("WriteFile src: %v", err) + } + if err := os.WriteFile(dst, []byte("old"), 0o600); err != nil { + t.Fatalf("WriteFile dst: %v", err) + } + + if err := recoverPartialReplace(tc.code, dst, backup, nil); !errors.Is(err, tc.code) { + t.Fatalf("error = %v, want the original %v unchanged", err, tc.code) + } + assertFileContent(t, dst, "old") + assertFileContent(t, src, "new") + if _, err := os.Lstat(backup); !os.IsNotExist(err) { + t.Fatalf("unexpected backup residue: %v", err) + } + }) + } +} + +// assertErrorDoesNotAdvertiseReplacement guards the messaging fixed for jatmn's +// #757 P3 finding: recovery errors used to say the replacement "remains at" the +// caller's temporary path, but the only caller removes that file in a deferred +// cleanup before the error ever surfaces. Recovery is always about the original, +// so an operator must never be sent looking for the replacement. +func assertErrorDoesNotAdvertiseReplacement(t *testing.T, err error, replacement string) { + t.Helper() + if err == nil { + t.Fatal("expected a partial-replacement error") + } + if strings.Contains(err.Error(), replacement) { + t.Fatalf("error = %v, must not point an operator at the replacement %q, which its caller deletes", err, replacement) + } +} + +func assertFileContent(t *testing.T, path, want string) { + t.Helper() + data, err := os.ReadFile(path) + if err != nil { + t.Fatalf("ReadFile %s: %v", path, err) + } + if string(data) != want { + t.Fatalf("content of %s = %q, want %q", path, data, want) + } +} + +func assertNoReplaceBackups(t *testing.T, dir string) { + t.Helper() + matches, err := filepath.Glob(filepath.Join(dir, replaceBackupPattern)) + if err != nil { + t.Fatalf("Glob replacement backups: %v", err) + } + if len(matches) != 0 { + t.Fatalf("replacement backups remain: %v", matches) + } +} + +func creationTime(t *testing.T, path string) syscall.Filetime { + t.Helper() + info, err := os.Stat(path) + if err != nil { + t.Fatalf("Stat %s: %v", path, err) + } + data, ok := info.Sys().(*syscall.Win32FileAttributeData) + if !ok { + t.Skipf("no Windows file attributes for %s", path) + } + return data.CreationTime +} + +func setCreationTime(t *testing.T, path string, created windows.Filetime) { + t.Helper() + pathPtr, err := windows.UTF16PtrFromString(path) + if err != nil { + t.Fatalf("UTF16PtrFromString: %v", err) + } + handle, err := windows.CreateFile(pathPtr, windows.FILE_WRITE_ATTRIBUTES, 0, nil, windows.OPEN_EXISTING, windows.FILE_ATTRIBUTE_NORMAL, 0) + if err != nil { + t.Fatalf("CreateFile %s: %v", path, err) + } + defer func() { + _ = windows.CloseHandle(handle) + }() + if err := windows.SetFileTime(handle, &created, nil, nil); err != nil { + t.Fatalf("SetFileTime %s: %v", path, err) + } +} + +func describeDACL(t *testing.T, path string) string { + t.Helper() + sd, err := windows.GetNamedSecurityInfo(path, windows.SE_FILE_OBJECT, windows.DACL_SECURITY_INFORMATION) + if err != nil { + t.Skipf("cannot read the security descriptor of %s: %v", path, err) + } + text := sd.String() + if index := strings.Index(text, "D:"); index >= 0 { + return text[index:] + } + return text +} diff --git a/internal/specialist/generate_tool.go b/internal/specialist/generate_tool.go index a9b523b7f..b96c4596c 100644 --- a/internal/specialist/generate_tool.go +++ b/internal/specialist/generate_tool.go @@ -104,9 +104,21 @@ func (tool *GenerateTool) Run(ctx context.Context, args map[string]any) tools.Re if err != nil { return taskError(err) } + return generateToolResult(manifest) +} + +func generateToolResult(manifest Manifest) tools.Result { + lines := []string{ + fmt.Sprintf("specialist: %s", manifest.Metadata.Name), + fmt.Sprintf("location: %s", manifest.Location), + fmt.Sprintf("path: %s", manifest.FilePath), + } + for _, warning := range manifest.Warnings { + lines = append(lines, "warning: "+warning) + } return tools.Result{ Status: tools.StatusOK, - Output: fmt.Sprintf("specialist: %s\nlocation: %s\npath: %s", manifest.Metadata.Name, manifest.Location, manifest.FilePath), + Output: strings.Join(lines, "\n"), Meta: map[string]string{ "name": manifest.Metadata.Name, "location": string(manifest.Location), diff --git a/internal/specialist/generate_tool_test.go b/internal/specialist/generate_tool_test.go index 79b064b38..fdeeabe9f 100644 --- a/internal/specialist/generate_tool_test.go +++ b/internal/specialist/generate_tool_test.go @@ -40,6 +40,37 @@ func TestGenerateToolCreatesSpecialist(t *testing.T) { } } +func TestGenerateToolResultAppendsWarnings(t *testing.T) { + manifest := Manifest{ + Metadata: Metadata{Name: "api-review"}, + Location: LocationProject, + FilePath: "/project/.zero/specialists/api-review.md", + Warnings: []string{"specialist was updated, but replacement backup /project/.zero/specialists/.api-review.bak could not be removed: access denied"}, + } + + result := generateToolResult(manifest) + + want := "specialist: api-review\nlocation: project\npath: /project/.zero/specialists/api-review.md\nwarning: " + manifest.Warnings[0] + if result.Status != tools.StatusOK || result.Output != want { + t.Fatalf("result = %#v, want output %q", result, want) + } +} + +func TestGenerateToolResultNormalOutputUnchanged(t *testing.T) { + manifest := Manifest{ + Metadata: Metadata{Name: "api-review"}, + Location: LocationProject, + FilePath: "/project/.zero/specialists/api-review.md", + } + + result := generateToolResult(manifest) + + want := "specialist: api-review\nlocation: project\npath: /project/.zero/specialists/api-review.md" + if result.Output != want { + t.Fatalf("output = %q, want %q", result.Output, want) + } +} + func TestGenerateToolDerivesNameAndDefaultPrompt(t *testing.T) { projectDir := filepath.Join(t.TempDir(), "project") tool := NewGenerateTool(NewStorage(Paths{ProjectDir: projectDir})) diff --git a/internal/specialist/manifest.go b/internal/specialist/manifest.go index 445c48e07..5335596c2 100644 --- a/internal/specialist/manifest.go +++ b/internal/specialist/manifest.go @@ -9,6 +9,7 @@ import ( "sort" "strconv" "strings" + "sync" "time" "github.com/Gitlawb/zero/internal/config" @@ -72,6 +73,12 @@ type LoadResult struct { var namePattern = regexp.MustCompile(`^[a-z][a-z0-9-]{0,63}$`) +// specialistFilesMu prevents Zero-managed loads from observing a specialist +// mutation in progress. This is especially important on Windows, where +// ReplaceFileW can briefly leave the destination name absent. It cannot +// synchronize external editors or other Zero processes. +var specialistFilesMu sync.RWMutex + var knownMetadataKeys = map[string]bool{ "name": true, "description": true, @@ -143,6 +150,8 @@ func Load(options LoadOptions) (LoadResult, error) { } paths.UserDir = resolved.UserDir } + specialistFilesMu.RLock() + defer specialistFilesMu.RUnlock() manifests := Builtins() warnings := []string{} @@ -476,6 +485,14 @@ func loadDirectory(dir string, location Location) ([]Manifest, []string, error) manifests := []Manifest{} warnings := []string{} for _, entry := range entries { + // Only *.md is a specialist. Everything else in the directory is + // deliberately invisible here, including the two kinds of sibling files an + // interrupted overwrite can leave: a .specialist-*.tmp replacement that was + // never published, and a .zero-replace-*.backup holding an original whose + // rollback failed on Windows. Neither is a manifest, and guessing that one + // of them is would be worse than the gap. The recovery for a backup that + // really does hold the last good copy is a manual rename, spelled out both + // in the fsutil error that reports it and in docs/SPECIALISTS.md. if entry.IsDir() || strings.ToLower(filepath.Ext(entry.Name())) != ".md" { continue } diff --git a/internal/specialist/storage.go b/internal/specialist/storage.go index 952f4f6f9..3b42f00f4 100644 --- a/internal/specialist/storage.go +++ b/internal/specialist/storage.go @@ -1,15 +1,20 @@ package specialist import ( + "errors" "fmt" "os" "path/filepath" + "runtime" "strconv" "strings" + + "github.com/Gitlawb/zero/internal/fsutil" ) type Storage struct { - paths Paths + paths Paths + writeReplacement func(string, string) error } type CreateInput struct { @@ -63,25 +68,28 @@ func (storage *Storage) Create(input CreateInput) (Manifest, error) { return Manifest{}, fmt.Errorf("specialist %q requires a system prompt", manifest.Metadata.Name) } content := FormatMarkdown(manifest) - if err := os.MkdirAll(filepath.Dir(path), 0o700); err != nil { + dir := filepath.Dir(path) + specialistFilesMu.Lock() + defer specialistFilesMu.Unlock() + if err := os.MkdirAll(dir, 0o700); err != nil { return Manifest{}, fmt.Errorf("create specialist directory: %w", err) } if input.Overwrite { - info, err := os.Lstat(path) - if err != nil && !os.IsNotExist(err) { - return Manifest{}, fmt.Errorf("inspect specialist file: %w", err) + writeReplacement := storage.writeReplacement + if writeReplacement == nil { + writeReplacement = writeSpecialistReplacement } - if err == nil && info.Mode()&os.ModeSymlink != 0 { - return Manifest{}, fmt.Errorf("refusing to overwrite symlink specialist file: %s", path) + if err := writeReplacement(path, content); err != nil { + var cleanupErr *fsutil.CommittedReplacementCleanupError + if errors.As(err, &cleanupErr) { + manifest.Warnings = append(manifest.Warnings, fmt.Sprintf("specialist was updated, but replacement backup %s could not be removed: %v", cleanupErr.BackupPath, cleanupErr.Cause)) + return manifest, nil + } + return Manifest{}, err } + return manifest, nil } - flags := os.O_WRONLY | os.O_CREATE - if input.Overwrite { - flags |= os.O_TRUNC - } else { - flags |= os.O_EXCL - } - file, err := os.OpenFile(path, flags, 0o600) + file, err := os.OpenFile(path, os.O_WRONLY|os.O_CREATE|os.O_EXCL, 0o600) if err != nil { if os.IsExist(err) { return Manifest{}, fmt.Errorf("specialist already exists: %s", manifest.Metadata.Name) @@ -98,12 +106,95 @@ func (storage *Storage) Create(input CreateInput) (Manifest, error) { return manifest, nil } +func writeSpecialistReplacement(path string, content string) error { + return writeSpecialistReplacementWith(path, content, nil, syncSpecialistDir) +} + +func writeSpecialistReplacementWith(path string, content string, rename func(string, string) error, syncDir func(string) error) (err error) { + temp, err := os.CreateTemp(filepath.Dir(path), ".specialist-*.tmp") + if err != nil { + return fmt.Errorf("create temporary specialist file: %w", err) + } + tempPath := temp.Name() + // The temporary file never survives this function, on any path: a successful + // replace has already consumed it, and every failure — including the Windows + // partial-replace recoveries in fsutil — discards it here. Recovery from a + // failed overwrite is therefore always about the ORIGINAL (which fsutil either + // rolls back or names in its error), never about this file, and no error text + // should tell an operator to go looking for it. + defer func() { + _ = temp.Close() + _ = os.Remove(tempPath) + }() + + if err := temp.Chmod(0o600); err != nil { + return fmt.Errorf("set temporary specialist file permissions: %w", err) + } + info, err := os.Lstat(path) + if err != nil && !os.IsNotExist(err) { + return fmt.Errorf("inspect specialist file: %w", err) + } + if err == nil { + if info.Mode()&os.ModeSymlink != 0 { + return fmt.Errorf("refusing to overwrite symlink specialist file: %s", path) + } + if runtime.GOOS != "windows" { + // The old in-place overwrite retained manually configured Unix modes. + // Preserve that behavior when publishing a replacement inode. + if err := temp.Chmod(info.Mode().Perm()); err != nil { + return fmt.Errorf("preserve specialist file permissions: %w", err) + } + } + } + if _, err := temp.WriteString(content); err != nil { + return fmt.Errorf("write temporary specialist file: %w", err) + } + if err := temp.Sync(); err != nil { + return fmt.Errorf("sync temporary specialist file: %w", err) + } + if err := temp.Close(); err != nil { + return fmt.Errorf("close temporary specialist file: %w", err) + } + + // On Unix, the same-directory rename publishes the complete file atomically. + // On Windows, ReplaceFileW is used to preserve the destination DACL rather + // than publishing the temporary file's inherited DACL. ReplaceFileW can + // briefly leave the destination name absent; specialistFilesMu prevents + // Zero-managed loads in this process from observing that window, but external + // processes and editors are not synchronized. + if err := fsutil.ReplaceWithRetry(tempPath, path, rename); err != nil { + return fmt.Errorf("replace specialist file: %w", err) + } + // The replacement is committed at this point. As in the sessions store, + // opening/fsyncing the parent directory only improves crash durability and + // must not turn a successful overwrite into an API failure. + _ = syncDir(filepath.Dir(path)) + return nil +} + +func syncSpecialistDir(path string) error { + if runtime.GOOS == "windows" { + return nil + } + dir, err := os.Open(path) + if err != nil { + return nil + } + if err := dir.Sync(); err != nil { + _ = dir.Close() + return err + } + return dir.Close() +} + func (storage *Storage) Delete(input DeleteInput) (string, error) { location := normalizeWritableLocation(input.Location) path, err := storage.path(input.Name, location) if err != nil { return "", err } + specialistFilesMu.Lock() + defer specialistFilesMu.Unlock() if err := os.Remove(path); err != nil { if os.IsNotExist(err) { return "", fmt.Errorf("specialist not found: %s", strings.TrimSpace(input.Name)) diff --git a/internal/specialist/storage_dacl_windows_test.go b/internal/specialist/storage_dacl_windows_test.go new file mode 100644 index 000000000..777d1cb85 --- /dev/null +++ b/internal/specialist/storage_dacl_windows_test.go @@ -0,0 +1,85 @@ +//go:build windows + +package specialist + +import ( + "os" + "path/filepath" + "strings" + "testing" + + "golang.org/x/sys/windows" +) + +// TestStorageCreateForceKeepsWindowsDACL is the regression test for the Windows +// DACL-preservation change: the replacement is a freshly created +// temporary file, so publishing it with a plain rename would hand the destination +// the directory's inherited DACL and drop the restrictive one an explicitly +// locked-down specialist had — exposing its system prompt to anyone the directory +// grants access to. temp.Chmod(0o600) cannot express that on Windows (Go only +// maps the owner-write bit there), so the replacement primitive has to carry the +// descriptor over. +func TestStorageCreateForceKeepsWindowsDACL(t *testing.T) { + userDir := t.TempDir() + path := filepath.Join(userDir, "safe.md") + if err := os.WriteFile(path, []byte("old content"), 0o600); err != nil { + t.Fatal(err) + } + // A protected, owner-only DACL: what an operator restricting one specialist + // inside a group-readable directory would end up with. + restricted, err := windows.SecurityDescriptorFromString("D:P(A;;FA;;;OW)") + if err != nil { + t.Skipf("cannot build a test security descriptor: %v", err) + } + dacl, _, err := restricted.DACL() + if err != nil { + t.Skipf("cannot read the test DACL: %v", err) + } + if err := windows.SetNamedSecurityInfo( + path, + windows.SE_FILE_OBJECT, + windows.DACL_SECURITY_INFORMATION|windows.PROTECTED_DACL_SECURITY_INFORMATION, + nil, nil, dacl, nil, + ); err != nil { + t.Skipf("cannot apply a restrictive DACL on this filesystem: %v", err) + } + want := specialistDACL(t, path) + if !strings.Contains(want, "(A;;FA;;;OW)") { + t.Skipf("the restrictive DACL did not take effect on this filesystem: %q", want) + } + + storage := NewStorage(Paths{UserDir: userDir}) + manifest, err := storage.Create(CreateInput{ + Name: "safe", + Description: "Safe", + SystemPrompt: "new content", + Overwrite: true, + }) + if err != nil { + t.Fatalf("Create returned error: %v", err) + } + data, err := os.ReadFile(path) + if err != nil { + t.Fatal(err) + } + if got, want := string(data), FormatMarkdown(manifest); got != want { + t.Fatalf("file content = %q, want %q", got, want) + } + if got := specialistDACL(t, path); got != want { + t.Fatalf("DACL after overwrite = %q, want the destination's own %q", got, want) + } + assertNoTemporarySpecialistFiles(t, userDir) +} + +func specialistDACL(t *testing.T, path string) string { + t.Helper() + sd, err := windows.GetNamedSecurityInfo(path, windows.SE_FILE_OBJECT, windows.DACL_SECURITY_INFORMATION) + if err != nil { + t.Skipf("cannot read the security descriptor of %s: %v", path, err) + } + text := sd.String() + if index := strings.Index(text, "D:"); index >= 0 { + return text[index:] + } + return text +} diff --git a/internal/specialist/storage_test.go b/internal/specialist/storage_test.go index fe4c41a9c..691e6be10 100644 --- a/internal/specialist/storage_test.go +++ b/internal/specialist/storage_test.go @@ -1,10 +1,16 @@ package specialist import ( + "errors" "os" "path/filepath" + "runtime" "strings" + "syscall" "testing" + "time" + + "github.com/Gitlawb/zero/internal/fsutil" ) func TestStorageCreateWritesValidManifestAndDeleteRemovesIt(t *testing.T) { @@ -88,4 +94,243 @@ func TestStorageCreateForceRejectsSymlink(t *testing.T) { if string(data) != "outside" { t.Fatalf("symlink target was modified: %q", string(data)) } + assertNoTemporarySpecialistFiles(t, userDir) +} + +func TestStorageCreateForceReplacesFileAndPreservesMode(t *testing.T) { + userDir := t.TempDir() + path := filepath.Join(userDir, "safe.md") + if err := os.WriteFile(path, []byte("old content"), 0o644); err != nil { + t.Fatal(err) + } + storage := NewStorage(Paths{UserDir: userDir}) + + manifest, err := storage.Create(CreateInput{ + Name: "safe", + Description: "Safe", + SystemPrompt: "new content", + Overwrite: true, + }) + if err != nil { + t.Fatalf("Create returned error: %v", err) + } + data, err := os.ReadFile(path) + if err != nil { + t.Fatal(err) + } + if got, want := string(data), FormatMarkdown(manifest); got != want { + t.Fatalf("file content = %q, want %q", got, want) + } + info, err := os.Stat(path) + if err != nil { + t.Fatal(err) + } + if got := info.Mode().Perm(); runtime.GOOS != "windows" && got != 0o644 { + t.Fatalf("file permissions = %o, want the existing mode 644", got) + } + assertNoTemporarySpecialistFiles(t, userDir) +} + +func TestStorageCreateForceSerializesConcurrentLoad(t *testing.T) { + userDir := t.TempDir() + storage := NewStorage(Paths{UserDir: userDir}) + if _, err := storage.Create(CreateInput{Name: "safe", Description: "old"}); err != nil { + t.Fatal(err) + } + + gapOpen := make(chan struct{}) + finishReplacement := make(chan struct{}) + storage.writeReplacement = func(path, content string) error { + backup := path + ".test-backup" + if err := os.Rename(path, backup); err != nil { + return err + } + close(gapOpen) + <-finishReplacement + if err := os.WriteFile(path, []byte(content), 0o600); err != nil { + return err + } + return os.Remove(backup) + } + + writeDone := make(chan error, 1) + go func() { + _, err := storage.Create(CreateInput{Name: "safe", Description: "new", Overwrite: true}) + writeDone <- err + }() + <-gapOpen + defer func() { + select { + case <-finishReplacement: + default: + close(finishReplacement) + } + }() + + loadDone := make(chan LoadResult, 1) + loadErr := make(chan error, 1) + loadStarted := make(chan struct{}) + go func() { + close(loadStarted) + result, err := Load(LoadOptions{Paths: Paths{UserDir: userDir}}) + if err != nil { + loadErr <- err + return + } + loadDone <- result + }() + <-loadStarted + select { + case err := <-loadErr: + t.Fatalf("Load returned during replacement: %v", err) + case result := <-loadDone: + t.Fatalf("Load returned during replacement gap: %#v", result.Specialists) + case <-time.After(25 * time.Millisecond): + } + + close(finishReplacement) + if err := <-writeDone; err != nil { + t.Fatalf("Create returned error: %v", err) + } + select { + case err := <-loadErr: + t.Fatal(err) + case result := <-loadDone: + manifest, ok := Find(result, "safe") + if !ok || manifest.Metadata.Description != "new" { + t.Fatalf("loaded specialist = %#v, want replacement", manifest) + } + case <-time.After(time.Second): + t.Fatal("Load did not resume after replacement") + } +} + +func TestStorageCreateReturnsManifestWarningAfterCommittedCleanupFailure(t *testing.T) { + userDir := t.TempDir() + backupPath := filepath.Join(userDir, ".zero-replace-old.backup") + cleanupErr := &os.PathError{Op: "remove", Path: backupPath, Err: syscall.Errno(32)} + storage := NewStorage(Paths{UserDir: userDir}) + storage.writeReplacement = func(path, content string) error { + if err := os.WriteFile(path, []byte(content), 0o600); err != nil { + return err + } + return &fsutil.CommittedReplacementCleanupError{BackupPath: backupPath, Cause: cleanupErr} + } + + manifest, err := storage.Create(CreateInput{Name: "safe", Description: "Safe", Overwrite: true}) + if err != nil { + t.Fatalf("Create returned error after committed replacement: %v", err) + } + if len(manifest.Warnings) != 1 || !strings.Contains(manifest.Warnings[0], backupPath) || !strings.Contains(manifest.Warnings[0], cleanupErr.Error()) { + t.Fatalf("warnings = %#v, want backup path and cleanup problem", manifest.Warnings) + } + if data, readErr := os.ReadFile(manifest.FilePath); readErr != nil || string(data) != FormatMarkdown(manifest) { + t.Fatalf("committed specialist content = %q, error = %v", data, readErr) + } +} + +func TestWriteSpecialistReplacementRetriesTransientWindowsRename(t *testing.T) { + if runtime.GOOS != "windows" { + t.Skip("rename retries are Windows-specific") + } + dir := t.TempDir() + path := filepath.Join(dir, "safe.md") + if err := os.WriteFile(path, []byte("old content"), 0o600); err != nil { + t.Fatal(err) + } + attempts := 0 + err := writeSpecialistReplacementWith(path, "new content", func(src, dst string) error { + attempts++ + if attempts == 1 { + return syscall.Errno(32) // ERROR_SHARING_VIOLATION + } + return os.Rename(src, dst) + }, func(string) error { return nil }) + if err != nil { + t.Fatalf("writeSpecialistReplacementWith returned error: %v", err) + } + if attempts != 2 { + t.Fatalf("rename attempts = %d, want 2", attempts) + } + data, err := os.ReadFile(path) + if err != nil { + t.Fatal(err) + } + if got := string(data); got != "new content" { + t.Fatalf("file content = %q, want %q", got, "new content") + } + assertNoTemporarySpecialistFiles(t, dir) +} + +func TestWriteSpecialistReplacementIgnoresDirectorySyncErrorAfterCommit(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "safe.md") + syncErr := errors.New("sync failed") + called := false + err := writeSpecialistReplacementWith(path, "new content", nil, func(got string) error { + called = true + if got != dir { + t.Fatalf("sync directory = %q, want %q", got, dir) + } + return syncErr + }) + if err != nil { + t.Fatalf("committed replacement returned sync error: %v", err) + } + if !called { + t.Fatal("directory sync was not attempted") + } + if data, readErr := os.ReadFile(path); readErr != nil || string(data) != "new content" { + t.Fatalf("committed content = %q, error = %v", data, readErr) + } + assertNoTemporarySpecialistFiles(t, dir) +} + +func TestSyncSpecialistDirIgnoresOpenFailure(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("directory sync is not attempted on Windows") + } + missing := filepath.Join(t.TempDir(), "missing") + if err := syncSpecialistDir(missing); err != nil { + t.Fatalf("directory open failure should be best-effort, got: %v", err) + } +} + +// TestWriteSpecialistReplacementKeepsTheOriginalAfterFailedReplace pins the caller +// side of Windows partial-failure rollback: fsutil restores the original at dst, +// reports the ReplaceFileW error, and leaves src for this function's deferred +// temporary-file cleanup. +func TestWriteSpecialistReplacementKeepsTheOriginalAfterFailedReplace(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "safe.md") + if err := os.WriteFile(path, []byte("old content"), 0o600); err != nil { + t.Fatal(err) + } + replaceErr := errors.New("ReplaceFileW could not rename the replacement") + err := writeSpecialistReplacementWith(path, "new content", func(_, _ string) error { + return replaceErr + }, func(string) error { return nil }) + + if !errors.Is(err, replaceErr) { + t.Fatalf("writeSpecialistReplacementWith error = %v, want it to report %v", err, replaceErr) + } + data, readErr := os.ReadFile(path) + if readErr != nil { + t.Fatalf("the original destination was lost after failed replacement: %v", readErr) + } + if got := string(data); got != "old content" { + t.Fatalf("file content = %q, want the original bytes", got) + } + assertNoTemporarySpecialistFiles(t, dir) +} + +func assertNoTemporarySpecialistFiles(t *testing.T, dir string) { + t.Helper() + matches, err := filepath.Glob(filepath.Join(dir, ".specialist-*.tmp")) + if err != nil { + t.Fatal(err) + } + if len(matches) != 0 { + t.Fatalf("temporary specialist files remain: %v", matches) + } }