From d73402fbf2eb9ea2c6ee02a4e19bb85796f3d578 Mon Sep 17 00:00:00 2001 From: REPPL <77722411+REPPL@users.noreply.github.com> Date: Sun, 12 Jul 2026 18:18:42 +0100 Subject: [PATCH] refactor: consolidate atomic-write primitives onto fsutil (iss-32) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four divergent temp-file+rename copies and a duplicate isRealDir are routed through the canonical internal/fsutil, behind a detector: - fsutil gains WriteFileAtomicPreserveMode (stat existing mode, default 0644, delegate to WriteFileAtomic). - ahoy marker.go writeFileAtomic + store.go isRealDir deleted; callers use fsutil.WriteFileAtomicPreserveMode / fsutil.IsRealDir. - capture roots.go writeFileAtomic deleted; callers use fsutil. - memory durableWrite replaced by a one-line writeStringAtomic adapter over fsutil.WriteFileAtomic; orphaned tmpCounter var + sync/atomic import removed. - TestNoNonCanonicalAtomicWritePrimitives flags any named redefinition outside fsutil; a new fsutil crash-safety suite covers the primitive (fsutil previously had zero tests). Behaviour: the ahoy and capture writers GAIN a parent-directory fsync (a durability strengthening; their copies never did it). Memory pages are now written at an explicit 0644 chmod where the old writer left the mode subject to the process umask (0644 under the default 022; formerly 0600 under a 077 umask) — deterministic and consistent with every other abcd store file. Symlink-refusal and same-directory atomic rename are unchanged; os.CreateTemp's random name is stronger than the old fixed temp path. security-reviewed PASS, ruthless-reviewed PROMOTE. A fifth inline copy (memory/ingest.go storeOriginal), outside this issue's named corpus, is tracked as iss-79. Assisted-by: Claude:claude-opus-4-8 --- ...ss-79-storeoriginal-inline-atomic-write.md | 11 ++ .../iss-32-atomic-write-consolidation.md | 1 + CHANGELOG.md | 9 ++ internal/core/ahoy/apply.go | 4 +- internal/core/ahoy/detect.go | 6 +- internal/core/ahoy/gitignore.go | 6 +- internal/core/ahoy/marker.go | 52 +------ internal/core/ahoy/store.go | 9 +- internal/core/capture/roots.go | 38 ----- internal/core/capture/workflow.go | 6 +- internal/core/memory/coverage.go | 2 +- internal/core/memory/lint.go | 4 +- internal/core/memory/writer.go | 55 +++----- internal/fsutil/canonical_test.go | 57 ++++++++ internal/fsutil/fsutil.go | 28 ++-- internal/fsutil/fsutil_test.go | 133 ++++++++++++++++++ 16 files changed, 274 insertions(+), 147 deletions(-) create mode 100644 .abcd/work/issues/open/iss-79-storeoriginal-inline-atomic-write.md rename .abcd/work/issues/{open => resolved}/iss-32-atomic-write-consolidation.md (61%) create mode 100644 internal/fsutil/canonical_test.go create mode 100644 internal/fsutil/fsutil_test.go diff --git a/.abcd/work/issues/open/iss-79-storeoriginal-inline-atomic-write.md b/.abcd/work/issues/open/iss-79-storeoriginal-inline-atomic-write.md new file mode 100644 index 00000000..372fb32e --- /dev/null +++ b/.abcd/work/issues/open/iss-79-storeoriginal-inline-atomic-write.md @@ -0,0 +1,11 @@ +--- +schema_version: 1 +id: "iss-79" +slug: "storeoriginal-inline-atomic-write" +severity: "minor" +category: "tech-debt" +source: "agent-finding" +found_during: "2026-07-12 /abcd:run iss-32 review" +--- + +fifth divergent atomic write: internal/core/memory/ingest.go storeOriginal (~:806) is an inline temp(.memtmp,O_EXCL)+fsync+rename durable write that iss-32's consolidation left untouched — it is inline, not a named func, so TestNoNonCanonicalAtomicWritePrimitives (name-based) cannot catch it, and it was outside iss-32's named 4-copy corpus. It differs from canonical fsutil.WriteFileAtomic: no parent-dir fsync, no explicit chmod. Consolidate it onto fsutil.WriteFileAtomic(target, material.rawBytes, 0644) (keeping the pre-existing sources-dir symlink guard), OR extend the canonical-primitive detector to catch inline temp+rename sequences. Safe today (has its own symlink guard); this is a completeness/consistency follow-up. \ No newline at end of file diff --git a/.abcd/work/issues/open/iss-32-atomic-write-consolidation.md b/.abcd/work/issues/resolved/iss-32-atomic-write-consolidation.md similarity index 61% rename from .abcd/work/issues/open/iss-32-atomic-write-consolidation.md rename to .abcd/work/issues/resolved/iss-32-atomic-write-consolidation.md index 886b9aa6..361e9422 100644 --- a/.abcd/work/issues/open/iss-32-atomic-write-consolidation.md +++ b/.abcd/work/issues/resolved/iss-32-atomic-write-consolidation.md @@ -7,6 +7,7 @@ category: "tech-debt" source: "agent-finding" found_during: "2026-07-08 multi-agent review" found_at: "internal/fsutil/fsutil.go" +resolution: "Consolidated the four named atomic-write/real-dir copies onto internal/fsutil (one-canonical-primitive), armed behind TestNoNonCanonicalAtomicWritePrimitives + a new fsutil crash-safety suite (fsutil had zero tests). Deleted ahoy writeFileAtomic+isRealDir, capture writeFileAtomic; memory durableWrite -> thin writeStringAtomic adapter over fsutil.WriteFileAtomic. Added fsutil.WriteFileAtomicPreserveMode. ruthless PROMOTE, security PASS (symlink-refusal preserved, O_EXCL->CreateTemp stronger, no mode downgrade). A 5th inline copy (memory/ingest.go storeOriginal), outside this issue's named corpus, is filed as iss-79." --- atomic-write consolidation: four independent temp-file+rename implementations with divergent durability — internal/fsutil.WriteFileAtomic (file fsync + parent-dir fsync), ahoy marker.go writeFileAtomic (no parent fsync, mode-preserving), capture roots.go writeFileAtomic (no parent fsync despite a doc comment claiming durability), memory writer.go durableWrite (parent fsync, O_EXCL naming); isRealDir is likewise duplicated (ahoy store.go vs fsutil). internal/fsutil itself has zero tests. Detector (per one-canonical-primitive): a lint flagging private redefinitions of the canonical names (writeFileAtomic, isRealDir) outside internal/fsutil, plus a fsutil crash-safety test suite; consolidation then drains behind the armed lint. Acceptance corpus: the three non-canonical copies and the duplicate isRealDir. \ No newline at end of file diff --git a/CHANGELOG.md b/CHANGELOG.md index 0dd499f1..0e30fff2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -129,6 +129,15 @@ called out in a **Breaking** section. named intent EXISTS and points back at this spec (bidirectional agreement). - The issue ledger moved from `.abcd/development/activity/issues` to `.abcd/work/issues` (the committed shared-working tier). +- The atomic-write and real-directory primitives are consolidated onto + `internal/fsutil` (iss-32): the ahoy, capture, and memory store writers no + longer keep their own divergent temp-file+rename copies. Two observable + effects of routing through the canonical primitive: the ahoy and capture + writers now fsync the parent directory after the rename (a crash-durability + strengthening they previously lacked), and memory pages are written at a + fixed `0644` (an explicit chmod, where the old writer left the mode subject to + the process umask). A `TestNoNonCanonicalAtomicWritePrimitives` guard keeps a + fifth copy from reappearing. ### Removed diff --git a/internal/core/ahoy/apply.go b/internal/core/ahoy/apply.go index 21a92a2d..79521a97 100644 --- a/internal/core/ahoy/apply.go +++ b/internal/core/ahoy/apply.go @@ -4,6 +4,8 @@ import ( "fmt" "os" "path/filepath" + + "github.com/REPPL/abcd-cli/internal/fsutil" "sort" "strings" "time" @@ -281,7 +283,7 @@ func (a *applyCtx) stepHistory() { } repoDir := filepath.Join(root, sha) transcripts := filepath.Join(repoDir, "transcripts") - if a.approved[SafeAutocreate] && !isRealDir(transcripts) { + if a.approved[SafeAutocreate] && !fsutil.IsRealDir(transcripts) { if err := os.MkdirAll(transcripts, 0o755); err == nil { a.note(transcripts) } diff --git a/internal/core/ahoy/detect.go b/internal/core/ahoy/detect.go index 7afd92ca..c9f698dc 100644 --- a/internal/core/ahoy/detect.go +++ b/internal/core/ahoy/detect.go @@ -4,6 +4,8 @@ import ( "os" "os/exec" "path/filepath" + + "github.com/REPPL/abcd-cli/internal/fsutil" "sort" "github.com/REPPL/abcd-cli/internal/core/identity" @@ -83,7 +85,7 @@ func classify(cwd string, id RepoIdentity, idx *historyIndex) (FolderKind, map[s registered := indexHasRoot(idx, id.RootSHA) signals["index_registered"] = registered - abcdDir := isRealDir(filepath.Join(cwd, ".abcd")) + abcdDir := fsutil.IsRealDir(filepath.Join(cwd, ".abcd")) signals["abcd_dir"] = abcdDir markerFired := false @@ -259,7 +261,7 @@ func detectHistoryStore(rootSHA string) []Gap { return gaps } repoDir := filepath.Join(root, rootSHA) - if !isRealDir(filepath.Join(repoDir, "transcripts")) { + if !fsutil.IsRealDir(filepath.Join(repoDir, "transcripts")) { gaps = append(gaps, Gap{ ID: "history.transcripts_missing", Category: SafeAutocreate, Scope: "repo", Title: "history transcripts/ dir missing", diff --git a/internal/core/ahoy/gitignore.go b/internal/core/ahoy/gitignore.go index b5dc8752..c2ed9477 100644 --- a/internal/core/ahoy/gitignore.go +++ b/internal/core/ahoy/gitignore.go @@ -4,6 +4,8 @@ import ( "bytes" "os" "path/filepath" + + "github.com/REPPL/abcd-cli/internal/fsutil" "strings" ) @@ -170,7 +172,7 @@ func applyVisibilityBlock(cwd, visibility string) (bool, error) { if absent { eol := "\n" body := strings.Join(canonicalGitignoreBlock(visibility), eol) + eol - if err := writeFileAtomic(path, []byte(body)); err != nil { + if err := fsutil.WriteFileAtomicPreserveMode(path, []byte(body)); err != nil { return false, err } return true, nil @@ -195,7 +197,7 @@ func applyVisibilityBlock(cwd, visibility string) (bool, error) { if bytes.Equal(newBytes, raw) { return false, nil // byte-identical — preserve mtime } - if err := writeFileAtomic(path, newBytes); err != nil { + if err := fsutil.WriteFileAtomicPreserveMode(path, newBytes); err != nil { return false, err } return true, nil diff --git a/internal/core/ahoy/marker.go b/internal/core/ahoy/marker.go index d4d4b6fb..198e112e 100644 --- a/internal/core/ahoy/marker.go +++ b/internal/core/ahoy/marker.go @@ -4,7 +4,8 @@ import ( "bytes" _ "embed" "os" - "path/filepath" + + "github.com/REPPL/abcd-cli/internal/fsutil" "regexp" ) @@ -108,7 +109,7 @@ func installMarkerFile(targetPath string) (wrote bool, ok bool) { if absent { body := append(append([]byte{}, synth...), eol...) - if err := writeFileAtomic(targetPath, body); err != nil { + if err := fsutil.WriteFileAtomicPreserveMode(targetPath, body); err != nil { return false, false } return true, true @@ -117,7 +118,7 @@ func installMarkerFile(targetPath string) (wrote bool, ok bool) { matches := markerBlockRe.FindAllIndex(existing, -1) if len(matches) == 0 { body := composeMarkerInsertion(existing, synth, eol) - if err := writeFileAtomic(targetPath, body); err != nil { + if err := fsutil.WriteFileAtomicPreserveMode(targetPath, body); err != nil { return false, false } return true, true @@ -127,7 +128,7 @@ func installMarkerFile(targetPath string) (wrote bool, ok bool) { return false, true // current — no write, mtime preserved } body := composeMarkerReplacement(existing, matches, synth) - if err := writeFileAtomic(targetPath, body); err != nil { + if err := fsutil.WriteFileAtomicPreserveMode(targetPath, body); err != nil { return false, false } return true, true @@ -201,7 +202,7 @@ func removeMarkerFile(targetPath string) (wrote bool, ok bool) { return false, true // no block — untouched } body := composeMarkerRemoval(existing, matches) - if err := writeFileAtomic(targetPath, body); err != nil { + if err := fsutil.WriteFileAtomicPreserveMode(targetPath, body); err != nil { return false, false } return true, true @@ -296,44 +297,3 @@ func concat(parts ...[]byte) []byte { } return out } - -// writeFileAtomic writes data to path via a temp file + rename, preserving the -// existing file's mode when present. Parent dirs are created as needed. -func writeFileAtomic(path string, data []byte) error { - dir := filepath.Dir(path) - if err := os.MkdirAll(dir, 0o755); err != nil { - return err - } - mode := os.FileMode(0o644) - if fi, err := os.Stat(path); err == nil { - mode = fi.Mode().Perm() - } - tmp, err := os.CreateTemp(dir, ".abcd-tmp-*") - if err != nil { - return err - } - tmpName := tmp.Name() - if _, err := tmp.Write(data); err != nil { - tmp.Close() - os.Remove(tmpName) - return err - } - if err := tmp.Sync(); err != nil { - tmp.Close() - os.Remove(tmpName) - return err - } - if err := tmp.Close(); err != nil { - os.Remove(tmpName) - return err - } - if err := os.Chmod(tmpName, mode); err != nil { - os.Remove(tmpName) - return err - } - if err := os.Rename(tmpName, path); err != nil { - os.Remove(tmpName) - return err - } - return nil -} diff --git a/internal/core/ahoy/store.go b/internal/core/ahoy/store.go index 531d8156..c7a1258b 100644 --- a/internal/core/ahoy/store.go +++ b/internal/core/ahoy/store.go @@ -5,6 +5,8 @@ import ( "os" "os/exec" "path/filepath" + + "github.com/REPPL/abcd-cli/internal/fsutil" "strings" "github.com/REPPL/abcd-cli/internal/core" @@ -116,11 +118,6 @@ func isDir(p string) bool { return err == nil && fi.IsDir() } -func isRealDir(p string) bool { - fi, err := os.Lstat(p) - return err == nil && fi.IsDir() && fi.Mode()&os.ModeSymlink == 0 -} - // --------------------------------------------------------------------------- // ~/.abcd/history store // --------------------------------------------------------------------------- @@ -293,7 +290,7 @@ func writeJSON(path string, v any) error { return err } data = append(data, '\n') - return writeFileAtomic(path, data) + return fsutil.WriteFileAtomicPreserveMode(path, data) } // writeConfig persists a config map deterministically. diff --git a/internal/core/capture/roots.go b/internal/core/capture/roots.go index 7787cc07..e4e4235b 100644 --- a/internal/core/capture/roots.go +++ b/internal/core/capture/roots.go @@ -113,41 +113,3 @@ func readWithChecksum(path string) (string, string, error) { } return string(data), sha256Hex(data), nil } - -// writeFileAtomic writes data to path via a temp file + fsync + rename, -// preserving the existing file mode (default 0644). -func writeFileAtomic(path string, data []byte) error { - dir := filepath.Dir(path) - mode := os.FileMode(0o644) - if fi, err := os.Stat(path); err == nil { - mode = fi.Mode().Perm() - } - tmp, err := os.CreateTemp(dir, ".abcd-iss-tmp-*") - if err != nil { - return err - } - tmpName := tmp.Name() - if _, err := tmp.Write(data); err != nil { - tmp.Close() - os.Remove(tmpName) - return err - } - if err := tmp.Sync(); err != nil { - tmp.Close() - os.Remove(tmpName) - return err - } - if err := tmp.Close(); err != nil { - os.Remove(tmpName) - return err - } - if err := os.Chmod(tmpName, mode); err != nil { - os.Remove(tmpName) - return err - } - if err := os.Rename(tmpName, path); err != nil { - os.Remove(tmpName) - return err - } - return nil -} diff --git a/internal/core/capture/workflow.go b/internal/core/capture/workflow.go index 068ae45c..b5dfe8f7 100644 --- a/internal/core/capture/workflow.go +++ b/internal/core/capture/workflow.go @@ -4,6 +4,8 @@ import ( "fmt" "os" "path/filepath" + + "github.com/REPPL/abcd-cli/internal/fsutil" "sort" "strconv" "strings" @@ -110,7 +112,7 @@ func commitCapture(req CaptureRequest, issID, slug, placeholder string) (Capture if checksum != emptyChecksum { return CaptureResult{}, fmt.Errorf("%w: placeholder %s changed since reservation", ErrChecksumMismatch, placeholder) } - if err := writeFileAtomic(placeholder, []byte(content)); err != nil { + if err := fsutil.WriteFileAtomicPreserveMode(placeholder, []byte(content)); err != nil { return CaptureResult{}, err } return CaptureResult{ID: issID, Slug: slug, Path: placeholder, Status: StateOpen}, nil @@ -204,7 +206,7 @@ func commitTransition(src, dst, newContent, expected string) error { if current != expected { return fmt.Errorf("%w: %s changed since it was read", ErrChecksumMismatch, src) } - if err := writeFileAtomic(dst, []byte(newContent)); err != nil { + if err := fsutil.WriteFileAtomicPreserveMode(dst, []byte(newContent)); err != nil { return err } if err := os.Remove(src); err != nil && !os.IsNotExist(err) { diff --git a/internal/core/memory/coverage.go b/internal/core/memory/coverage.go index e4b03634..2eb3f552 100644 --- a/internal/core/memory/coverage.go +++ b/internal/core/memory/coverage.go @@ -561,7 +561,7 @@ func writeCoverageIndex(path string, result coverageResult, budget quotationBudg if err := os.MkdirAll(dirOf(path), 0o755); err != nil { return nil, err } - if err := durableWrite(path, marshalIndentNoEscape(payload)); err != nil { + if err := writeStringAtomic(path, marshalIndentNoEscape(payload)); err != nil { return nil, err } return payload, nil diff --git a/internal/core/memory/lint.go b/internal/core/memory/lint.go index dbc5245e..3d017b10 100644 --- a/internal/core/memory/lint.go +++ b/internal/core/memory/lint.go @@ -464,10 +464,10 @@ func Lint(req LintRequest) (LintResult, error) { "generated_at": generatedAt, "store_path": mem, } - if err := durableWrite(filepath.Join(reportDir, "report.json"), marshalIndentNoEscape(reportFields)); err != nil { + if err := writeStringAtomic(filepath.Join(reportDir, "report.json"), marshalIndentNoEscape(reportFields)); err != nil { return LintResult{}, err } - if err := durableWrite(filepath.Join(reportDir, "report.md"), renderLintReportMD(reportFields)); err != nil { + if err := writeStringAtomic(filepath.Join(reportDir, "report.md"), renderLintReportMD(reportFields)); err != nil { return LintResult{}, err } diff --git a/internal/core/memory/writer.go b/internal/core/memory/writer.go index 13c35456..42877fc2 100644 --- a/internal/core/memory/writer.go +++ b/internal/core/memory/writer.go @@ -5,9 +5,10 @@ import ( "path/filepath" "sort" "strings" - "sync/atomic" "syscall" "time" + + "github.com/REPPL/abcd-cli/internal/fsutil" ) // writer.go — the atomic single-writer for .abcd/memory/ (ADR-13 §1–3). EVERY @@ -33,8 +34,6 @@ type WriteReport struct { WrotePages []string `json:"wrote_pages"` } -var tmpCounter uint64 - type renderedWrite struct { write PageWrite text string @@ -171,7 +170,7 @@ func writePagesLocked(repoRoot string, rendered []renderedWrite, merge RegistryM var wrote []string for _, r := range rendered { - if err := durableWrite(filepath.Join(mem, r.write.Filename), r.text); err != nil { + if err := writeStringAtomic(filepath.Join(mem, r.write.Filename), r.text); err != nil { return WriteReport{}, err } wrote = append(wrote, r.write.Filename) @@ -189,7 +188,7 @@ func writePagesLocked(repoRoot string, rendered []renderedWrite, merge RegistryM if err != nil { return WriteReport{}, err } - if err := durableWrite(SourcesIndexPath(repoRoot), SerializeRegistry(registry)); err != nil { + if err := writeStringAtomic(SourcesIndexPath(repoRoot), SerializeRegistry(registry)); err != nil { return WriteReport{}, err } } @@ -290,35 +289,13 @@ func logEvent(w PageWrite, stamp string) string { // Six-step durable write + tri-state read // --------------------------------------------------------------------------- -func durableWrite(path, content string) error { - tmp := path + "." + itoa(os.Getpid()) + "." + itoa(int(atomic.AddUint64(&tmpCounter, 1)-1)) + ".memtmp" - f, err := os.OpenFile(tmp, os.O_WRONLY|os.O_CREATE|os.O_EXCL, 0o644) - if err != nil { - return err - } - writeErr := func() error { - if _, err := f.WriteString(content); err != nil { - return err - } - if err := f.Sync(); err != nil { - return err - } - return f.Close() - }() - if writeErr != nil { - f.Close() - os.Remove(tmp) - return writeErr - } - if err := os.Rename(tmp, path); err != nil { - os.Remove(tmp) - return err - } - if dir, err := os.Open(filepath.Dir(path)); err == nil { - dir.Sync() - dir.Close() - } - return nil +// writeStringAtomic durably writes string content through the canonical +// fsutil primitive at the store's fixed 0644 mode — a thin string adapter, not +// a second implementation (iss-32: one-canonical-primitive). fsutil handles the +// temp-file, fsync, rename, and parent-dir fsync, matching the durability this +// store previously carried in its own copy. +func writeStringAtomic(path, content string) error { + return fsutil.WriteFileAtomic(path, []byte(content), 0o644) } // triStateRead: (content, present, error). Absent -> ("", false, nil); a @@ -375,7 +352,7 @@ func ensureSkeleton(mem string) ([]string, error) { return nil, err } if !present { - if err := durableWrite(path, s.render()); err != nil { + if err := writeStringAtomic(path, s.render()); err != nil { return nil, err } created = append(created, s.name) @@ -412,12 +389,12 @@ func backfillLegacy(mem string) ([]string, error) { continue } newRegion := region + "source:\n class: " + backfillSourceClass + "\n" - if err := durableWrite(path, joinFileFrontmatter(newRegion, body)); err != nil { + if err := writeStringAtomic(path, joinFileFrontmatter(newRegion, body)); err != nil { return nil, err } } else { newRegion := "source:\n class: " + backfillSourceClass + "\n" - if err := durableWrite(path, joinFileFrontmatter(newRegion, "\n"+text)); err != nil { + if err := writeStringAtomic(path, joinFileFrontmatter(newRegion, "\n"+text)); err != nil { return nil, err } } @@ -528,7 +505,7 @@ func reconcile(mem string) ([]string, error) { return nil, err } if !present || sha256Hex(current) != sha256Hex(d.content) { - if err := durableWrite(path, d.content); err != nil { + if err := writeStringAtomic(path, d.content); err != nil { return nil, err } rewritten = append(rewritten, d.name) @@ -547,7 +524,7 @@ func appendLog(mem string, events []string) error { if present { base = current } - return durableWrite(path, base+strings.Join(events, "")) + return writeStringAtomic(path, base+strings.Join(events, "")) } // --------------------------------------------------------------------------- diff --git a/internal/fsutil/canonical_test.go b/internal/fsutil/canonical_test.go new file mode 100644 index 00000000..a0d0394e --- /dev/null +++ b/internal/fsutil/canonical_test.go @@ -0,0 +1,57 @@ +package fsutil + +import ( + "os" + "path/filepath" + "regexp" + "strings" + "testing" +) + +// nonCanonicalPrimitiveRe matches a private redefinition of a durable-write or +// real-dir primitive — the exact names iss-32 consolidates. The canonical home +// is internal/fsutil (exported WriteFileAtomic / IsRealDir); any lowercase +// redefinition elsewhere is a divergent copy. +var nonCanonicalPrimitiveRe = regexp.MustCompile(`func\s+(writeFileAtomic|durableWrite|isRealDir)\b`) + +// TestNoNonCanonicalAtomicWritePrimitives is the one-canonical-primitive +// detector: no package under internal/ (other than fsutil) may declare its own +// named atomic-write or real-dir primitive. It matches top-level function +// declarations by name (not inline temp+rename sequences), which is what the +// consolidation removes. It walks the internal/ tree from this package's +// directory; before the consolidation it flags four copies (ahoy +// marker.go/store.go, capture roots.go, memory writer.go). +func TestNoNonCanonicalAtomicWritePrimitives(t *testing.T) { + internalRoot := filepath.Join("..") // internal/fsutil -> internal/ + var offenders []string + err := filepath.WalkDir(internalRoot, func(path string, d os.DirEntry, err error) error { + if err != nil { + return err + } + if d.IsDir() { + // The canonical home is exempt; it defines the real thing. + if filepath.Base(path) == "fsutil" { + return filepath.SkipDir + } + return nil + } + if !strings.HasSuffix(d.Name(), ".go") || strings.HasSuffix(d.Name(), "_test.go") { + return nil + } + data, err := os.ReadFile(path) + if err != nil { + return err + } + for _, m := range nonCanonicalPrimitiveRe.FindAllStringSubmatch(string(data), -1) { + offenders = append(offenders, path+": func "+m[1]) + } + return nil + }) + if err != nil { + t.Fatalf("walk internal/: %v", err) + } + if len(offenders) > 0 { + t.Fatalf("non-canonical atomic-write/real-dir primitives (route through internal/fsutil):\n %s", + strings.Join(offenders, "\n ")) + } +} diff --git a/internal/fsutil/fsutil.go b/internal/fsutil/fsutil.go index 6d0db09a..c29df995 100644 --- a/internal/fsutil/fsutil.go +++ b/internal/fsutil/fsutil.go @@ -1,13 +1,13 @@ // Package fsutil holds the durable-write and path-safety primitives shared by -// the ~/.abcd store writers. It is transport-agnostic: no stdout, no os.Exit, -// no CLI knowledge. +// the ~/.abcd and repo .abcd store writers. It is transport-agnostic: no +// stdout, no os.Exit, no CLI knowledge. // -// It exists so the six-step atomic write and the "is this a real directory, -// not a symlink" check live in ONE place. ahoy's marker.go/store.go carry -// their own unexported copies (writeFileAtomic, isRealDir) predating this -// package; those are the flagged consolidation target — a follow-up -// behaviour-preserving refactor should route them through here rather than -// keep a divergent copy. +// It is the single home for the atomic temp-file+fsync+rename write and the +// "is this a real directory, not a symlink" check: the ahoy, capture, and +// memory store writers all route through WriteFileAtomic / +// WriteFileAtomicPreserveMode / IsRealDir rather than keep divergent copies +// (the one-canonical-primitive invariant, guarded by +// TestNoNonCanonicalAtomicWritePrimitives). package fsutil import ( @@ -60,6 +60,18 @@ func WriteFileAtomic(path string, data []byte, perm os.FileMode) error { return nil } +// WriteFileAtomicPreserveMode is WriteFileAtomic that keeps the target's +// existing permission bits when it already exists, defaulting to 0644 for a new +// file. It is the canonical form for the store writers that rewrite a file in +// place and must not silently reset its mode. +func WriteFileAtomicPreserveMode(path string, data []byte) error { + perm := os.FileMode(0o644) + if fi, err := os.Stat(path); err == nil { + perm = fi.Mode().Perm() + } + return WriteFileAtomic(path, data, perm) +} + // syncParent fsyncs the directory so a crash right after the rename cannot lose // it. Some filesystems refuse a directory fsync; that is tolerated. func syncParent(dir string) { diff --git a/internal/fsutil/fsutil_test.go b/internal/fsutil/fsutil_test.go new file mode 100644 index 00000000..127f329d --- /dev/null +++ b/internal/fsutil/fsutil_test.go @@ -0,0 +1,133 @@ +package fsutil + +import ( + "os" + "path/filepath" + "testing" +) + +func TestWriteFileAtomicCreatesWithPerm(t *testing.T) { + dir := t.TempDir() + p := filepath.Join(dir, "sub", "f.txt") // parent dir does not exist yet + if err := WriteFileAtomic(p, []byte("hello"), 0o600); err != nil { + t.Fatalf("write: %v", err) + } + got, err := os.ReadFile(p) + if err != nil { + t.Fatalf("read: %v", err) + } + if string(got) != "hello" { + t.Fatalf("content = %q, want hello", got) + } + fi, err := os.Stat(p) + if err != nil { + t.Fatal(err) + } + if fi.Mode().Perm() != 0o600 { + t.Fatalf("perm = %o, want 600", fi.Mode().Perm()) + } +} + +func TestWriteFileAtomicOverwrites(t *testing.T) { + dir := t.TempDir() + p := filepath.Join(dir, "f.txt") + if err := WriteFileAtomic(p, []byte("first"), 0o644); err != nil { + t.Fatal(err) + } + if err := WriteFileAtomic(p, []byte("second"), 0o644); err != nil { + t.Fatal(err) + } + got, _ := os.ReadFile(p) + if string(got) != "second" { + t.Fatalf("content = %q, want second", got) + } + // No temp files linger after a successful write. + entries, _ := os.ReadDir(dir) + if len(entries) != 1 { + t.Fatalf("expected exactly one file, got %d: %v", len(entries), entries) + } +} + +// TestWriteFileAtomicReplacesSymlink proves the leaf symlink is REPLACED, not +// written through: a pre-planted symlink at path must not clobber its target. +func TestWriteFileAtomicReplacesSymlink(t *testing.T) { + dir := t.TempDir() + victim := filepath.Join(dir, "victim.txt") + if err := os.WriteFile(victim, []byte("do-not-touch"), 0o644); err != nil { + t.Fatal(err) + } + link := filepath.Join(dir, "link.txt") + if err := os.Symlink(victim, link); err != nil { + t.Fatal(err) + } + if err := WriteFileAtomic(link, []byte("new"), 0o644); err != nil { + t.Fatalf("write: %v", err) + } + // The symlink is now a real file with the new content... + fi, err := os.Lstat(link) + if err != nil { + t.Fatal(err) + } + if fi.Mode()&os.ModeSymlink != 0 { + t.Fatalf("path is still a symlink; the write followed it") + } + // ...and the victim was not written through. + got, _ := os.ReadFile(victim) + if string(got) != "do-not-touch" { + t.Fatalf("symlink target was clobbered: %q", got) + } +} + +func TestWriteFileAtomicPreserveMode(t *testing.T) { + dir := t.TempDir() + + // New file defaults to 0644. + fresh := filepath.Join(dir, "fresh.txt") + if err := WriteFileAtomicPreserveMode(fresh, []byte("x")); err != nil { + t.Fatal(err) + } + if fi, _ := os.Stat(fresh); fi.Mode().Perm() != 0o644 { + t.Fatalf("new file perm = %o, want 644", fi.Mode().Perm()) + } + + // Existing file keeps its mode across a rewrite. + kept := filepath.Join(dir, "kept.txt") + if err := WriteFileAtomic(kept, []byte("a"), 0o600); err != nil { + t.Fatal(err) + } + if err := WriteFileAtomicPreserveMode(kept, []byte("b")); err != nil { + t.Fatal(err) + } + if fi, _ := os.Stat(kept); fi.Mode().Perm() != 0o600 { + t.Fatalf("rewritten file perm = %o, want 600 (preserved)", fi.Mode().Perm()) + } +} + +func TestIsRealDir(t *testing.T) { + dir := t.TempDir() + realDir := filepath.Join(dir, "d") + if err := os.Mkdir(realDir, 0o755); err != nil { + t.Fatal(err) + } + file := filepath.Join(dir, "f") + if err := os.WriteFile(file, []byte("x"), 0o644); err != nil { + t.Fatal(err) + } + linkToDir := filepath.Join(dir, "ld") + if err := os.Symlink(realDir, linkToDir); err != nil { + t.Fatal(err) + } + + if !IsRealDir(realDir) { + t.Errorf("real dir reported as not-real") + } + if IsRealDir(file) { + t.Errorf("file reported as real dir") + } + if IsRealDir(linkToDir) { + t.Errorf("symlink-to-dir reported as real dir (must lstat, not follow)") + } + if IsRealDir(filepath.Join(dir, "missing")) { + t.Errorf("missing path reported as real dir") + } +}