diff --git a/.abcd/work/issues/open/iss-82-rules-inject-savestate-inline-atomic-write.md b/.abcd/work/issues/open/iss-82-rules-inject-savestate-inline-atomic-write.md new file mode 100644 index 00000000..e4e9e972 --- /dev/null +++ b/.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. \ No newline at end of file diff --git a/.abcd/work/issues/open/iss-79-storeoriginal-inline-atomic-write.md b/.abcd/work/issues/resolved/iss-79-storeoriginal-inline-atomic-write.md similarity index 82% rename from .abcd/work/issues/open/iss-79-storeoriginal-inline-atomic-write.md rename to .abcd/work/issues/resolved/iss-79-storeoriginal-inline-atomic-write.md index 372fb32e..6b8a75a7 100644 --- a/.abcd/work/issues/open/iss-79-storeoriginal-inline-atomic-write.md +++ b/.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. \ No newline at end of file diff --git a/CHANGELOG.md b/CHANGELOG.md index 07f399ac..7854ddb6 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/internal/core/memory/ingest.go b/internal/core/memory/ingest.go index 62b61067..a0572577 100644 --- a/internal/core/memory/ingest.go +++ b/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 diff --git a/internal/core/memory/memory_test.go b/internal/core/memory/memory_test.go index 64ac0939..01df43b6 100644 --- a/internal/core/memory/memory_test.go +++ b/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") diff --git a/internal/core/memory/writer.go b/internal/core/memory/writer.go index 42877fc2..e3040570 100644 --- a/internal/core/memory/writer.go +++ b/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:]) -} diff --git a/internal/fsutil/canonical_test.go b/internal/fsutil/canonical_test.go index a0d0394e..549b078e 100644 --- a/internal/fsutil/canonical_test.go +++ b/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 ")) + } +}