refactor: route storeOriginal through fsutil.WriteFileAtomic (iss-79)

.abcd/work/issues/open/iss-82-rules-inject-savestate-inline-atomic-write.md

@@ -0,0 +1,12 @@
+---
+schema_version: 1
+id: "iss-82"
+slug: "rules-inject-savestate-inline-atomic-write"
+severity: "minor"
+category: "tech-debt"
+source: "user-observation"
+found_during: "2026-07-12 iss-79 correctness review"
+found_at: "internal/core/rules/inject.go"
+---
+
+Sixth divergent inline atomic write, and a detector-coverage gap: internal/core/rules/inject.go SaveState open-codes a durable write as os.CreateTemp + Write + Close + os.Rename with NO fsync and no parent-dir fsync -- weaker crash-safety than even the storeOriginal inline write iss-79 consolidated. It is not routed through fsutil.WriteFileAtomic, and the iss-79 detector (TestNoInlineAtomicWriteSequences) keys on os.O_EXCL + os.Rename so it does NOT flag the CreateTemp+Rename idiom. Fix: route SaveState through fsutil.WriteFileAtomic, and broaden the canonical-primitive detector to also flag inline os.CreateTemp + os.Rename sequences (watch it flag inject.go, then drain). Deferred from iss-79 to keep that change scoped to storeOriginal. Acceptance corpus: inject.go SaveState; the broadened detector must flag it before the fix and pass after. Found during iss-79 correctness review.

.abcd/work/issues/open/iss-79-storeoriginal-inline-atomic-write.md → .abcd/work/issues/resolved/iss-79-storeoriginal-inline-atomic-write.md

@@ -6,6 +6,7 @@ severity: "minor"
 category: "tech-debt"
 source: "agent-finding"
 found_during: "2026-07-12 /abcd:run iss-32 review"
+resolution: "storeOriginal routed through fsutil.WriteFileAtomic behind an extended inline-atomic-write detector (os.O_EXCL+os.Rename); CreateTemp+Rename idiom gap (inject.go SaveState) filed as iss-82"
 ---
 
 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.

CHANGELOG.md

@@ -20,6 +20,12 @@ called out in a **Breaking** section.
   authoritative, and flagging a single file would imply the others are fine. The
   collision itself is resolved (the later claimant renumbered to `itd-83`); the
   underlying minting scheme is tracked as `iss-80`.
+- `memory ingest --keep-original` writes the stored source copy through the
+  canonical `fsutil.WriteFileAtomic` (temp + fsync + **chmod + parent-directory
+  fsync**) instead of an inline temp+rename that omitted both — the fifth
+  divergent atomic write the `iss-32` consolidation left untouched. The
+  one-canonical-primitive detector now also flags inline `os.O_EXCL`+`os.Rename`
+  sequences, not just named primitives (iss-79).
 
 ### Added
 

internal/core/memory/ingest.go

@@ -14,6 +14,8 @@ import (
 	"syscall"
 	"time"
 	"unicode/utf8"
+
+	"github.com/REPPL/abcd-cli/internal/fsutil"
 )
 
 // ingest.go — the ingest flow from 07-memory.md §1:
@@ -802,25 +804,13 @@ func storeOriginal(repoRoot string, material sourceMaterial, contentHash string)
 	} else if fi.Mode()&os.ModeSymlink != 0 || !fi.IsDir() {
 		return "", newIngestError("sources dir is a symlink or non-directory: %s", sourcesRelPath)
 	}
+	// The sources dir is guaranteed a real directory by the guard above; route
+	// the durable write through the canonical primitive (temp + fsync + chmod +
+	// rename + parent-dir fsync) rather than an inline copy (iss-79 /
+	// one-canonical-primitive). os.Rename does not follow a leaf symlink, so a
+	// pre-planted target symlink is replaced, not written through.
 	target := filepath.Join(sourcesDir, contentHash+material.ext)
-	tmp := target + "." + itoa(os.Getpid()) + ".memtmp"
-	f, err := os.OpenFile(tmp, os.O_WRONLY|os.O_CREATE|os.O_EXCL, 0o644)
-	if err != nil {
-		return "", err
-	}
-	if _, err := f.Write(material.rawBytes); err != nil {
-		f.Close()
-		os.Remove(tmp)
-		return "", err
-	}
-	if err := f.Sync(); err != nil {
-		f.Close()
-		os.Remove(tmp)
-		return "", err
-	}
-	f.Close()
-	if err := os.Rename(tmp, target); err != nil {
-		os.Remove(tmp)
+	if err := fsutil.WriteFileAtomic(target, material.rawBytes, 0o644); err != nil {
 		return "", err
 	}
 	return filepath.Join(".abcd", "memory", "sources", contentHash+material.ext), nil

internal/core/memory/memory_test.go

@@ -489,6 +489,48 @@ func TestIngestKeepOriginalFailureStillReportsIngest(t *testing.T) {
 	}
 }
 
+// TestIngestKeepOriginalWritesSourceCanonically is the success-path regression
+// test for --keep-original: the stored original lands with the exact source bytes
+// and a 0644 mode, and its repo-relative path is reported. (The reroute itself is
+// pinned by the canonical-primitive detector; this locks the happy path.)
+func TestIngestKeepOriginalWritesSourceCanonically(t *testing.T) {
+	repo := t.TempDir()
+	content := "Rotate tokens every 24 hours."
+	src := writeSource(t, repo, "article.txt", content)
+
+	res, err := Ingest(IngestRequest{
+		RepoRoot:     repo,
+		Source:       src,
+		KeepOriginal: true,
+		Distiller:    oneTopicDistiller("topic", "auth", "tokens", "# Token rotation\nRotate tokens every 24 hours."),
+		Now:          fixedNow,
+	})
+	if err != nil {
+		t.Fatalf("ingest: %v", err)
+	}
+	if res.KeepOriginalError != "" {
+		t.Fatalf("unexpected keep-original error: %s", res.KeepOriginalError)
+	}
+	if res.KeptOriginal == "" {
+		t.Fatalf("KeptOriginal path must be reported on success")
+	}
+	stored := filepath.Join(repo, res.KeptOriginal)
+	fi, err := os.Stat(stored)
+	if err != nil {
+		t.Fatalf("stored original not written: %v", err)
+	}
+	if fi.Mode().Perm() != 0o644 {
+		t.Fatalf("stored original mode = %v, want 0644 (fsutil.WriteFileAtomic chmod)", fi.Mode().Perm())
+	}
+	got, err := os.ReadFile(stored)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if string(got) != content {
+		t.Fatalf("stored original bytes = %q, want %q", got, content)
+	}
+}
+
 // TestSplitFileFrontmatterCRLFParity proves the parser-parity instance: a
 // CRLF-terminated document must split identically to its LF twin. Before the
 // fix splitFileFrontmatter's exact-match closing delimiter ("---" != "---\r")

internal/core/memory/writer.go

@@ -546,25 +546,3 @@ func sortedUnion(a, b []string) []string {
 	sort.Strings(out)
 	return out
 }
-
-func itoa(n int) string {
-	if n == 0 {
-		return "0"
-	}
-	neg := n < 0
-	if neg {
-		n = -n
-	}
-	var b [20]byte
-	i := len(b)
-	for n > 0 {
-		i--
-		b[i] = byte('0' + n%10)
-		n /= 10
-	}
-	if neg {
-		i--
-		b[i] = '-'
-	}
-	return string(b[i:])
-}

internal/fsutil/canonical_test.go

@@ -55,3 +55,52 @@ func TestNoNonCanonicalAtomicWritePrimitives(t *testing.T) {
 			strings.Join(offenders, "\n  "))
 	}
 }
+
+// TestNoInlineAtomicWriteSequences is the second half of the one-canonical-
+// primitive detector (iss-79): the name-based check above cannot see a durable
+// write open-coded inline rather than in a named func — exactly how memory's
+// storeOriginal escaped the iss-32 consolidation. An inline atomic write is an
+// exclusive temp create (os.O_EXCL) whose temp is then renamed onto the target
+// (os.Rename); their co-occurrence in one non-fsutil file is the signature.
+// (capture's allocator uses syscall.O_EXCL for a reservation with no rename, so
+// it is correctly not matched — the os-package constant plus os.Rename is what
+// marks a hand-rolled WriteFileAtomic.) The fix routes such a site through
+// fsutil.WriteFileAtomic, which removes os.O_EXCL from the file.
+//
+// Known gap: this does not yet catch the os.CreateTemp+os.Rename idiom (a weaker
+// inline write, no fsync). rules/inject.go SaveState is one such site — broadening
+// the detector to that idiom and routing it is tracked in iss-82.
+func TestNoInlineAtomicWriteSequences(t *testing.T) {
+	internalRoot := filepath.Join("..")
+	var offenders []string
+	err := filepath.WalkDir(internalRoot, func(path string, d os.DirEntry, err error) error {
+		if err != nil {
+			return err
+		}
+		if d.IsDir() {
+			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
+		}
+		src := string(data)
+		if strings.Contains(src, "os.O_EXCL") && strings.Contains(src, "os.Rename(") {
+			offenders = append(offenders, path+": inline os.O_EXCL temp + os.Rename")
+		}
+		return nil
+	})
+	if err != nil {
+		t.Fatalf("walk internal/: %v", err)
+	}
+	if len(offenders) > 0 {
+		t.Fatalf("inline atomic-write sequences (route through fsutil.WriteFileAtomic):\n  %s",
+			strings.Join(offenders, "\n  "))
+	}
+}