Merge pull request #154 from clay-good/harden-memory-integrity-invariant

feat(memory): harden the memory-integrity invariant under concurrency + adversarial edits

Author: clay-good

openspec/specs/api/spec.md

@@ -2977,3 +2977,17 @@ The API SHALL support `CLI Command openlore drift` to [partial spec — file too
 - **GIVEN** A git repository with a pre-commit hook installed
 - **WHEN** openlore drift --uninstall-hook is executed
 - **THEN** The pre-commit hook is removed
+
+### Requirement: DecisionApiWritesUseAtomicCompareAndSwap
+
+The decision-writing API functions (`openloreRecordDecision`, `openloreConsolidateDecisions`,
+`openloreSyncDecisions`) SHALL persist through the shared atomic compare-and-swap store path
+(`updateDecisionStore`), so a write committed concurrently by another path (e.g. the MCP
+`record_decision` handler or a background consolidation) is never silently clobbered.
+`openloreRecordDecision` SHALL derive the decision id from the committed store's `sessionId`
+so repeated records within a session deduplicate correctly.
+
+#### Scenario: Concurrent API record and MCP record lose no decision
+- **GIVEN** an `openloreRecordDecision` call and a concurrent `record_decision` against the same store
+- **WHEN** both complete
+- **THEN** the persisted store contains both decisions — neither write is lost

openspec/specs/architecture/spec.md

@@ -38,6 +38,55 @@ The system SHALL implement security via: API key-based authentication for LLM pr
 - **WHEN** accessing protected resources
 - **THEN** access is denied
 
+### Requirement: DurableAtomicStorePersistence
+
+The persisted memory and decision stores (`.openlore/memory/notes.json`,
+`.openlore/decisions/pending.json`) SHALL be written atomically: a write goes to a
+uniquely-named temporary file, is `fsync`'d, and is moved into place with an atomic rename,
+after which the containing directory is `fsync`'d (best-effort) so the rename itself is
+durable. A crash or interruption mid-write leaves the previously committed store intact and
+never a partially written (torn) file. Each store SHALL carry a monotonic `sequence` field
+(defaults to `0` for legacy stores) that orders writes and lets external readers detect
+change. The read-modify-write SHALL be performed entirely inside a single per-store advisory
+lock so that the lock — not an optimistic sequence guard — is the serialization point:
+`mutate` always runs against the freshest on-disk store and a competing write cannot
+interleave. The advisory lock SHALL carry an ownership token and be released only by the
+writer that still owns it (so a hold stolen as stale is never freed out from under its new
+owner); a crashed holder's lock SHALL become stealable well before a waiter gives up, and a
+wait that times out SHALL fail loud rather than write unlocked. ALL writers of a given store
+(record, approve/reject, consolidation, sync, and HTTP-API equivalents) SHALL go through this
+single compare-and-swap path; a raw lock-free overwrite is prohibited because it would defeat
+the serialization. Implemented in `src/core/decisions/atomic-store.ts`; guarded by
+`atomic-store.test.ts`.
+
+#### Scenario: A crash mid-write preserves the prior store
+- **GIVEN** a store write interrupted between writing the temporary file and the rename
+- **WHEN** the store is next loaded
+- **THEN** the previously committed store is returned intact, with no torn or partial content
+
+#### Scenario: Save uses compare-and-swap on sequence
+- **GIVEN** a store loaded at sequence S
+- **WHEN** a save is attempted but the on-disk sequence is no longer S
+- **THEN** the save re-reads the current store, re-applies the pending change, and writes at
+  the new sequence instead of overwriting
+
+### Requirement: CorruptStoreQuarantineNotSilentEmpty
+
+When a persisted store fails validation on load, the system SHALL move the unreadable file
+aside to a quarantine path (`*.corrupt-<n>`) and emit a recoverable signal. The system SHALL
+NOT silently substitute an empty store for a corrupt one, because silently losing persisted
+memory presents absence as current fact and violates the authoritative-recall invariant. The
+quarantine suffix SHALL be derived from on-disk state (the next free index), not wall-clock
+time, to keep recovery reproducible. The claim on a quarantine path SHALL be atomic (it fails
+if the path already exists), so two concurrent loaders can never overwrite each other's
+quarantine file and lose preserved bytes.
+
+#### Scenario: A malformed store is quarantined, not silently emptied
+- **GIVEN** a store file that fails schema or JSON validation
+- **WHEN** the store is loaded
+- **THEN** the file is moved to `*.corrupt-<n>` and a recoverable signal is emitted, rather
+  than an empty store being returned silently
+
 ## System Diagram
 
 ```mermaid

openspec/specs/cli/spec.md

@@ -284,6 +284,49 @@ The system SHALL support a --preset navigation flag on the MCP server that expos
 
 The system SHALL serialize concurrent decision consolidation processes using a cross-process advisory file lock to prevent draft loss.
 
+> Decision recorded: 412817d2
+> Date: 2026-06-01
+
+### Requirement: GateReasonMachineIsAPureTotalClassifier
+
+The pre-commit gate's block-reason decision SHALL be computed by a pure, total
+classifier (`classifyGateState`, `src/core/decisions/gate-state.ts`) that maps the
+decision-store state (approved / verified / draft counts, consolidation recency,
+active count, git-repo + staged-source flags) to exactly one outcome: pass, or
+block with one of the canonical reasons (`verified`, `approved_not_synced`,
+`drafts_pending_consolidation`, `no_decisions_recorded`). The CLI gate command
+SHALL delegate the reason decision to this classifier and only build the
+user-facing payload around it. The classifier SHALL be **total** (every state maps
+to exactly one outcome), **deterministic / idempotent** (same state ⇒ same
+outcome), and **deadlock-free** (a blocking outcome always carries an actionable
+reason; a passing outcome never carries one), enforced by property tests
+(`gate-state.test.ts`).
+
+#### Scenario: Every gate state yields exactly one outcome
+
+- **GIVEN** any combination of decision-store counts, consolidation recency, and
+  git staged-source state
+- **WHEN** the gate reason is classified
+- **THEN** the result is either a clean pass or a block with exactly one canonical
+  reason — never blocked-without-reason and never passing-with-reason
+
+### Requirement: AllDecisionStoreWritersUseAtomicCompareAndSwap
+
+Every writer of the decision store (`pending.json`) — `record_decision`,
+`approve`/`reject`, consolidation, sync, and the HTTP API equivalents — SHALL
+persist through the single atomic compare-and-swap path
+(`updateDecisionStore` / `casUpdate`), so a write committed by one path is never
+silently clobbered by a stale snapshot from another. A raw, lock-free overwrite of
+the store by any production writer is prohibited.
+
+#### Scenario: Rapid record_decision under background consolidation loses no draft
+
+- **GIVEN** several `record_decision` calls in quick succession, each spawning a
+  background consolidation that also writes the store
+- **WHEN** all writes complete
+- **THEN** the persisted store contains every recorded decision — no write is lost
+  to a competing path on a different lock
+
 > Decision recorded: 412817d2
 > Date: 2026-06-01
 ### Requirement: AddSelecttestsMcpToolForCallgraphbasedTestImpactSelection

openspec/specs/mcp-handlers/spec.md

@@ -116,6 +116,72 @@ The system SHALL The recall tool SHALL partition returned memories into authorit
 
 > Decision recorded: dbe6a95e
 > Date: 2026-06-16
+
+### Requirement: AuthoritativeRecallInvariant
+
+The system SHALL guarantee, as a single named and test-enforced invariant, that **no
+memory whose freshness verdict is `drifted` or `orphaned` ever appears in an authoritative
+recall path unlabeled**. The authoritative recall paths are the `recall` tool and the
+memory (decision) section of `orient`. An `orphaned` memory SHALL be fully withheld from
+the authoritative set (surfaced only under `needsReanchoring` / `staleDecisions`); a
+`drifted` memory MAY remain in the authoritative set only when it carries an explicit
+`verify` label. This invariant is the operational definition of the project promise:
+*OpenLore never serves an unverified or stale fact as authoritative.* It SHALL be enforced
+by a property-based test (`memory-invariant.test.ts`) that generates arbitrary memories and
+arbitrary code mutations and asserts the property holds for every generated case.
+
+#### Scenario: A drifted memory is excluded from the authoritative set unlabeled
+
+- **GIVEN** a memory whose anchor verdict is `drifted`
+- **WHEN** `recall` or `orient` produces its response
+- **THEN** the memory does not appear in the authoritative set unlabeled; it is withheld or
+  carries an explicit verify/non-authoritative label
+
+#### Scenario: The invariant holds under generated mutation
+
+- **GIVEN** an arbitrary memory and an arbitrary mutation to the code it anchors
+- **WHEN** the authoritative recall path is computed
+- **THEN** the authoritative set contains only `fresh` memories and explicitly-labeled
+  `drifted` ones, never an `orphaned` memory
+
+### Requirement: FreshnessFailsSafeTowardDistrust
+
+The freshness computation (`anchorFreshness`, `hashSpan`) SHALL fail safe toward distrust:
+any ambiguity, hash collision, or boundary error SHALL bias the verdict toward `drifted` or
+`orphaned`, never toward a false `fresh`. A renamed, moved, or deleted symbol SHALL yield
+`orphaned` (or `drifted` only when a confident relocation is established). `hashSpan` SHALL
+slice spans by byte offset so multibyte UTF-8 boundaries hash correctly. A test that
+produces a false `fresh` SHALL be treated as a correctness failure; a false `orphaned` is
+acceptable. This is guarded by the adversarial suite (`anchor-adversarial.test.ts`).
+
+#### Scenario: A forced collision does not produce false fresh
+
+- **GIVEN** two distinct source spans
+- **WHEN** freshness is computed for a memory anchored to one after the other replaces it
+- **THEN** the verdict is `drifted` or `orphaned`, never `fresh` (distinct spans do not
+  collide on the truncated content hash; a collision would fail the suite loudly)
+
+#### Scenario: A multibyte span boundary hashes correctly
+
+- **GIVEN** an anchored span whose start or end falls on a multibyte UTF-8 boundary
+- **WHEN** `hashSpan` computes the content hash before and after an unrelated edit elsewhere
+- **THEN** the hash is byte-correct and stable, producing `fresh` only when the span bytes
+  are unchanged
+
+### Requirement: ConcurrentMemoryWriteSafety
+
+The `remember` and `record_decision` tools SHALL be safe under concurrent invocation: two
+concurrent writes to the same store SHALL NOT cause either write to be lost. On a write
+conflict the system SHALL re-read the current store and re-apply the pending
+append/upsert (compare-and-swap on a monotonic `sequence`), rather than overwrite the
+competing write.
+
+#### Scenario: Concurrent remember calls lose no write
+
+- **GIVEN** N concurrent `remember` calls against the same memory store
+- **WHEN** all calls complete
+- **THEN** the persisted store contains all N memories
+
 ### Requirement: DecisionsCarryStructuralAnchorsForSelfinvalidation
 
 The system SHALL resolve structural anchors against the call graph when recording a decision, falling back to file-level anchors when no analysis is available.

src/api/decisions.ts

@@ -20,7 +20,7 @@ import { createLLMService } from '../core/services/llm-service.js';
 import { isGitRepository, getChangedFiles, getFileDiff, getCommitMessages, resolveBaseRef, buildSpecMap } from '../core/drift/index.js';
 import {
   loadDecisionStore,
-  saveDecisionStore,
+  updateDecisionStore,
   upsertDecisions,
   patchDecision,
   makeDecisionId,
@@ -102,10 +102,15 @@ export async function openloreRecordDecision(options: RecordDecisionOptions): Pr
     syncedToSpecs: [],
   };
 
-  const updated = upsertDecisions(store, [decision]);
-  await saveDecisionStore(rootPath, updated);
+  // CAS upsert so concurrent writers never lose a draft; derive the id from the
+  // committed store's sessionId so repeated records in a session dedupe correctly.
+  let recordedId = id;
+  await updateDecisionStore(rootPath, (s) => {
+    recordedId = makeDecisionId(s.sessionId, domain, options.title);
+    return upsertDecisions(s, [{ ...decision, id: recordedId, sessionId: s.sessionId }]);
+  });
 
-  return { id };
+  return { id: recordedId };
 }
 
 // ============================================================================
@@ -182,12 +187,12 @@ export async function openloreConsolidateDecisions(
     : { verified: consolidated.map((d) => ({ ...d, status: 'verified' as const, confidence: 'medium' as const })), phantom: [], missing: [] };
   progress(onProgress, 'Verifying decisions', 'complete', `${verified.length} verified`);
 
-  let updatedStore = { ...store };
-  for (const id of supersededIds) {
-    updatedStore = patchDecision(updatedStore, id, { status: 'rejected' });
-  }
-  updatedStore = upsertDecisions(updatedStore, [...verified, ...phantom]);
-  await saveDecisionStore(rootPath, updatedStore);
+  // CAS persist onto the freshest store so a concurrently-recorded draft is kept.
+  const updatedStore = await updateDecisionStore(rootPath, (s) => {
+    let next = s;
+    for (const id of supersededIds) next = patchDecision(next, id, { status: 'rejected' });
+    return upsertDecisions(next, [...verified, ...phantom]);
+  });
 
   return { verified, phantom, missing, store: updatedStore };
 }