refactor: route storeOriginal through fsutil.WriteFileAtomic (iss-79) - #51
Merged
Merged
Conversation
memory ingest's storeOriginal was a fifth inline temp + os.O_EXCL + fsync + rename durable write that the iss-32 consolidation missed -- it was open-coded, not a named func, so the name-based canonical-primitive detector could not see it, and it diverged from fsutil.WriteFileAtomic by omitting the parent-directory fsync and the explicit chmod. storeOriginal now routes through fsutil.WriteFileAtomic(target, rawBytes, 0644), keeping the sources-dir symlink/non-directory guard ahead of the write. The orphaned itoa helper (its only caller was the removed temp-name line) is deleted. Detector-first: TestNoInlineAtomicWriteSequences extends the one-canonical- primitive check to flag any non-fsutil file pairing os.O_EXCL with os.Rename -- the inline-atomic-write signature the name-based regex missed (capture's allocator uses syscall.O_EXCL for a reservation with no rename, so it is correctly not flagged). Watched it flag ingest.go, then pass after the reroute. Added TestIngestKeepOriginalWritesSourceCanonically: --keep-original lands the source bytes with an explicit 0644 mode via the canonical primitive. Behaviour-preserving from the caller's view; strengthens crash-safety. Assisted-by: Claude:claude-opus-4-8
iss-79's storeOriginal reroute lands in this branch. The correctness review surfaced a sixth inline atomic write -- rules/inject.go SaveState (CreateTemp+ Rename, no fsync) -- which the os.O_EXCL-keyed detector does not catch; captured as iss-82 (route it + broaden the detector to the CreateTemp idiom) rather than widening this scoped change. Assisted-by: Claude:claude-opus-4-8
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
memory ingest'sstoreOriginalwas a fifth inline temp +os.O_EXCL+ fsync +rename durable write that the
iss-32consolidation missed — open-coded (not anamed func), so the name-based canonical-primitive detector could not see it, and
diverging from
fsutil.WriteFileAtomicby omitting the parent-directory fsyncand the explicit chmod.
How
storeOriginalnow routes throughfsutil.WriteFileAtomic(target, rawBytes, 0o644), keeping the sources-dir symlink/non-directoryos.Lstatguard ahead ofthe write. The orphaned
itoahelper (only caller was the removed temp-nameline) is deleted.
TestNoInlineAtomicWriteSequencesextends theone-canonical-primitive check to flag any non-
fsutilfile pairingos.O_EXCLwith
os.Rename— the inline-atomic-write signature the name-based regexmissed. (
capture's allocator usessyscall.O_EXCLfor a reservation with norename, so it is correctly not flagged.) Watched it flag
ingest.go, then passafter the reroute.
TestIngestKeepOriginalWritesSourceCanonically(success path: bytes +0644 mode via the canonical primitive).
Behaviour-preserving from the caller's view; strengthens crash-safety.
Review
Correctness: PROMOTE. Security: PROMOTE (target name is a hex hash +
safeExt,no traversal;
os.Renamekeeps leaf-symlink non-following; symlink-dir guardretained; adds chmod + parent-fsync hardening).
The correctness review found a sixth inline atomic write —
rules/inject.goSaveState(CreateTemp+Rename, no fsync) — which theos.O_EXCL-keyed detector does not catch. Filed as iss-82 (route it +broaden the detector to the
CreateTempidiom) rather than widening this scopedchange; the detector comment documents the gap.
Scope
internal/core/memory/{ingest,writer,memory_test}.go,internal/fsutil/ canonical_test.go, CHANGELOG, ledger (resolves iss-79, files iss-82). Fullmake preflightgreen (build, vet, gofmt, tests,-race).Part of drain #2. Chain: branched from
main; independent of iss-76/iss-73.Assisted-by: Claude:claude-opus-4-8