correx/docs/reviews/2026-05-18-cas-steps-1-8-review.md

CAS Artifact Store — Steps 1–8 Final Review

Plan: docs/plans/2026-05-17-cas-artifact-store.md Date: 2026-05-18 Scope: Steps 1–8 (step 9 cloud sync deferred to a later epic) Build status: :infrastructure:artifacts-cas:check green, 21/21 tests, detekt + kover clean

Summary

Content-Addressed Store added as a new core interface (core:artifacts-store) plus a SQLite+segment-file implementation (infrastructure/artifacts-cas). Inference events now carry promptArtifactId / responseArtifactId; orchestrators put(...) bytes before emitting events. SqliteEventStore.append wraps its txn in artifactStore.flushBefore { ... } so segment data is fsynced before the event commit. Recovery tail-scan re-indexes orphan tail records on reopen, making the crash window between artifact write and event commit non-corrupting. A manual compactor + oldest-first disk-cap evictor round out the lifecycle; both gated by a maintenanceMutex so they cannot race each other.

CLAUDE.md invariants

All hold:

• Event log remains sole source of truth; segment + index are derivable via TailScanner.recover().
• No new EventPayload subclasses introduced; InferenceStartedEvent / InferenceCompletedEvent already registered in core/events/.../serialization/Serialization.kt.
• No cross-core impl deps. core:artifacts-store depends only on core:events (for the ArtifactId = TypeId typealias).
• Apps → core → infrastructure DAG preserved.
• Polymorphic registration verified for the modified inference event classes.

Findings

MUST FIX

None.

SHOULD FIX

1. CasArtifactStore.open() discards the TailScanReport (infrastructure/artifacts-cas/.../CasArtifactStore.kt:87-91). If recovery truncates a corrupt tail or re-indexes orphan records, the caller has no signal. Log at INFO when truncatedAt != null or recoveredRecords > 0.

2. No end-to-end orchestrator→put→append→replay→get test. SessionOrchestratorIntegrationTest uses NoopArtifactStore plus a RecordingArtifactStore to assert put-then-emit ordering, but nothing wires the real CasArtifactStore + SqliteEventStore together to verify the crash-window invariant in situ. Add one test: put via real CAS, append an inference event, reopen, replay, assert get(responseArtifactId) returns the original bytes.

3. SqliteEventStore.allEvents() (new in this diff) lacks withContext(Dispatchers.IO). Called from LivenessScanner which already wraps the iteration, so currently safe — but for consistency the read methods on the store itself should dispatch internally.

4. ReplayOrchestrator.runInference super path still calls artifactStore.put for the prompt before checking strategy (core/kernel/.../ReplayOrchestrator.kt:178). For non-SkipInference strategies during replay this writes a new artifact for an event being re-derived, bloating the store with duplicates. Verify intent; if unintentional, gate put on a replay flag.

NICE TO HAVE

• SegmentWriter.append does not fsync per-record; durability comes from flushBefore. Document this contract in ArtifactStore.kt KDoc.
• Compactor.readLiveEntries builds liveHashHex from liveIds.value (hex) and compares to hash.toHex() per entry. A ByteArrayKey wrapper would avoid per-entry hex allocation.
• CasArtifactStore.evict does listDirectoryEntries under mutex to determine activeId. SegmentWriter already tracks currentSegmentId — expose a getter and skip the filesystem call.

Categories with no findings

• Polymorphic serialization registration
• Cross-core deps
• Resource hygiene (AutoCloseable, .use { } chains)
• Detekt antipatterns (Thread.sleep, bare try/catch, new @Suppress)

Per-step inventory

Step	Subject	Status
1	ArtifactId value class in core:events	reused existing TypeId typealias
2	core:artifacts-store interface module	done
3	infrastructure/artifacts-cas MVP (writer/reader/index/store)	done
4	DI wiring + flushBefore around SQLite commit	done
5	Event schema (promptArtifactId / responseArtifactId)	done
6	Recovery tail-scan	done
7	Compactor + LivenessScanner	done
8	Disk-cap eviction policy	done
9	Cloud sync	deferred (later epic)