diff --git a/App/AppDelegate.swift b/App/AppDelegate.swift index 471690c..3d35799 100644 --- a/App/AppDelegate.swift +++ b/App/AppDelegate.swift @@ -383,6 +383,16 @@ final class AppDelegate: NSObject, NSApplicationDelegate { /// (audit trail: Track-D D-1; semantics unchanged after D-5/D-6.) var memorySearchAvailable: Bool = false + /// Audit 2026-05-12 / M-2 (Issue #13): per-process sentinel TurnID for + /// memory-mutation / memory-retrieval events that don't have a real + /// per-turn TurnID (the extraction orchestrator runs after `.turnEnd` + /// and isn't pinned to a specific turn). Generated once at delegate + /// construction; `installAgent` writes the matching row into + /// `replay.sqlite/turns` so `events.turn_id REFERENCES turns(turn_id)` + /// resolves. Pre-fix the sink synthesized a fake TurnID per call which + /// broke the FK on every emission. + let memorySentinelTurnID: TurnID = TurnID.fresh() + /// Background memory-extraction orchestrator. Drains a bounded /// `AsyncChannel(capacity: 32, .dropOldest)` one job at a time so a /// stalled 32B-model extraction never back-pressures `.turnEnd` @@ -1554,12 +1564,33 @@ final class AppDelegate: NSObject, NSApplicationDelegate { .deletingLastPathComponent() .appendingPathComponent("jarvis.db") - // 2. MemoryStore. Hard-block on failure per D-5/D-6 — see method + // 2a. Embedder. Constructed BEFORE MemoryStore so both the writer + // side (Issue #12: populating `facts_vec` after every fact + // INSERT) and the reader side (HybridSearch) share one + // instance. Embedder construction is permissive — only fails + // on a non-loopback URL — but if the constructor somehow + // throws, we proceed with a nil embedder so the store still + // opens (degraded mode: FTS5-only search). Loopback host is + // hardcoded; the OllamaEmbeddingClient.assertLoopback check + // will never reject `127.0.0.1`. + let memoryEmbedder: OllamaEmbeddingClient? + do { + memoryEmbedder = try OllamaEmbeddingClient( + baseURL: URL(string: "http://127.0.0.1:11434")! + ) + } catch { + systemLogger?.warning( + "installMemory: embedder init failed — facts_vec writes will be skipped (degraded mode): \(String(describing: error))" + ) + memoryEmbedder = nil + } + + // 2b. MemoryStore. Hard-block on failure per D-5/D-6 — see method // docstring above for the policy + the prior Track-D D-1 era // that this reversed. let store: MemoryStore do { - store = try MemoryStore(databaseURL: dbURL) + store = try MemoryStore(databaseURL: dbURL, embedder: memoryEmbedder) } catch { let description = String(describing: error) systemLogger?.critical( @@ -1580,9 +1611,17 @@ final class AppDelegate: NSObject, NSApplicationDelegate { let searchAvailable = true // 3. Wire the replay sink. `store` is non-optional after D-5/D-6 - // (we'd have terminated above if init had failed). + // (we'd have terminated above if init had failed). Issue #13: + // the sink attributes every memory event to the per-process + // sentinel TurnID (memorySentinelTurnID); `installAgent` writes + // the matching `turns` row after `beginSession`. Until that row + // exists, sink emissions still flow but fail the FK and get + // dropped via the batch-split path — chain stays uncorrupted. if let log = replayLog { - await store.setReplayLog(AppDelegateMemoryReplaySink(replayLog: log)) + await store.setReplayLog(AppDelegateMemoryReplaySink( + replayLog: log, + sentinelTurnID: memorySentinelTurnID + )) } else { systemLogger?.warning("installMemory: replayLog absent — memory mutation rows won't persist") } @@ -1635,18 +1674,19 @@ final class AppDelegate: NSObject, NSApplicationDelegate { // `MemoryStore.forgetFact`. (audit trail: Track-D D-2.) let forgetDispatcher: any ForgetFactDispatching = ForgetFactStoreAdapter(store: store) var searchDispatcher: (any HybridSearchDispatching)? = nil - if searchAvailable { - do { - let embedder = try OllamaEmbeddingClient( - baseURL: URL(string: "http://127.0.0.1:11434")! - ) - let hybrid = HybridSearch(store: store, embedder: embedder) - searchDispatcher = HybridSearchAdapter(hybrid: hybrid) - } catch { - systemLogger?.warning( - "installMemory: embedder init failed — search_memory not registered: \(String(describing: error))" - ) - } + if searchAvailable, let embedder = memoryEmbedder { + // Audit 2026-05-12 / M-1 (Issue #12): the SAME embedder + // instance powers both writer-side (facts_vec inserts) and + // reader-side (HybridSearch.searchFacts) so the vector space + // stays consistent. Pre-fix this constructed a second + // OllamaEmbeddingClient for search only; the writer side + // never had one, which is why facts_vec was empty. + let hybrid = HybridSearch(store: store, embedder: embedder) + searchDispatcher = HybridSearchAdapter(hybrid: hybrid) + } else { + systemLogger?.warning( + "installMemory: search_memory not registered — embedder absent (degraded mode)" + ) } // Register INTO the shared `inProcessToolRegistry` — never @@ -1814,6 +1854,28 @@ final class AppDelegate: NSObject, NSApplicationDelegate { ) return } + + // Audit 2026-05-12 / M-2 (Issue #13): write the memory-sentinel turn + // row so subsequent memory-mutation / memory-retrieval events emit + // against a turn_id that exists in `turns`. Without this row, every + // memory event would FK-fail and (pre-Issue-#13-batch-split-fix) take + // every batch-sibling down with it. The sentinel never `endTurn`s — + // its `ended_at` stays NULL for the life of the session. + do { + try await replayLog.startTurn( + turnId: memorySentinelTurnID, + sessionId: sessionId, + retryOf: nil, + turnNonce: "memory-sentinel", + source: .text, + provider: "memory-system", + modelId: "memory-system" + ) + } catch { + systemLogger?.warning( + "installAgent: memory-sentinel startTurn failed — memory events will FK-fail and be dropped via batch-split: \(String(describing: error))" + ) + } // Plan 10-02b / B-01: source the tool catalog lazily from BOTH // the in-process registry (memory + self-knowledge tools) AND // the stdio MCPClient (mcp-time / mcp-clipboard / @@ -3045,25 +3107,33 @@ struct DormantVoiceBusEmitter: BusOutboundEmitter { /// Bridges `Memory.MemoryReplaySink` → `Replay.ReplayLog`. /// /// The Memory module deliberately does not import `Replay.ReplayLog` directly -/// (07-02 design); AppDelegate is the only place that bridges the two. The -/// adapter forwards a recorded `ReplayEvent` from MemoryStore.applyOp into the -/// on-disk replay log. The log records are TurnID-keyed; the orchestrator -/// records `ReplayEvent.memoryMutation` against an Int64 turnId hash, but the -/// replay log indexes on TurnID. This adapter generates a synthetic TurnID -/// stub from the Int64 hash — once AgentOrchestrator wiring lands the real -/// turn-id correlation will be in place. +/// (07-02 design); AppDelegate is the only place that bridges the two. +/// +/// Audit 2026-05-12 / M-2 (Issue #13): pre-fix this sink synthesized a fake +/// TurnID per call (`"memory-trigger-"`) from the Int64 triggerTurnId +/// the Memory module passed in. That synthetic TurnID had no row in the +/// replay-DB's `turns` table, so EVERY emission fired the foreign-key +/// constraint on `events.turn_id REFERENCES turns(turn_id)`. The whole +/// batch transaction rolled back, the log re-buffered, and consecutive +/// failures climbed to chronic-critical within minutes. Real turn events +/// queued behind the poison were silently lost. +/// +/// New design: the sink holds a single optional **sentinel TurnID** that +/// `installAgent` writes into the `turns` table once per session (a turn +/// row with `ended_at=NULL`, `stop_reason=NULL`, source = `.text`). Memory +/// emissions use this sentinel — the FK resolves, the events land, and the +/// Int64 triggerTurnId travels in the JSON payload for forensic queries. struct AppDelegateMemoryReplaySink: MemoryReplaySink { let replayLog: ReplayLog + let sentinelTurnID: TurnID func record(_ event: ReplayEvent, forTriggerTurnId triggerTurnId: Int64) { - // The replay log's `record(_:for:)` is async; fire-and-forget through - // a detached Task so the synchronous MemoryReplaySink contract stays - // intact. The Int64 trigger-turn-id is encoded as a TurnID's UUID - // string deterministically (FNV-1a hash → reverse stub) so replay - // queries can correlate later. - let synthetic = TurnID(rawValue: "memory-trigger-\(triggerTurnId)") - Task.detached { [replayLog] in - await replayLog.record(event, for: synthetic) + // Fire-and-forget through a detached Task so the synchronous + // MemoryReplaySink contract stays intact. The Int64 triggerTurnId + // is already encoded into the event's JSON payload by MemoryStore; + // we attribute the row to the sentinel turn so the FK resolves. + Task.detached { [replayLog, sentinelTurnID] in + await replayLog.record(event, for: sentinelTurnID) } } } diff --git a/packages/AgentCore/Sources/AgentCore/ContextBuilder.swift b/packages/AgentCore/Sources/AgentCore/ContextBuilder.swift index 10361c8..1f156b1 100644 --- a/packages/AgentCore/Sources/AgentCore/ContextBuilder.swift +++ b/packages/AgentCore/Sources/AgentCore/ContextBuilder.swift @@ -70,7 +70,11 @@ extension ContextBuilder { get_self_state, list_camera_devices, get_time, get_clipboard, \ run_applescript) over giving the user generic instructions like \ "open System Settings" or "I don't have access to your hardware." \ - You are not a text-only assistant. + You are not a text-only assistant. \ + You also have a long-term memory store. When the user references \ + something they may have told you before, or asks "do you remember…", \ + call search_memory FIRST. Never claim to remember without searching. \ + Use forget_fact when the user explicitly asks to forget something. """ /// Compose the full system prompt for one turn. diff --git a/packages/AgentCore/Tests/AgentCoreTests/CacheHintsEligibilityTests.swift b/packages/AgentCore/Tests/AgentCoreTests/CacheHintsEligibilityTests.swift index 3faec3b..53e4807 100644 --- a/packages/AgentCore/Tests/AgentCoreTests/CacheHintsEligibilityTests.swift +++ b/packages/AgentCore/Tests/AgentCoreTests/CacheHintsEligibilityTests.swift @@ -135,4 +135,31 @@ final class CacheHintsEligibilityTests: XCTestCase { "ContextBuilder.systemPrompt(for: .empty) must start with selfAwarePreamble (D-12 — preamble FIRST, then per-turn enrichments)." ) } + + // MARK: - Audit 2026-05-12 / M-3 — proactive memory recall discoverability + // + // Issue #14: agent never called `search_memory` in production because the + // preamble didn't name it — model preference is shaped by the preamble's + // explicit "always prefer calling those tools" rule. Without a name in the + // preamble, the tool falls into "tools the agent technically has access to + // but won't think to use." Regression guard: the preamble MUST cue + // `search_memory` and `forget_fact` so proactive recall is alive. + + /// test_preambleNamesMemoryTools — Issue #14 regression guard. + /// + /// Asserts both memory tools are named in the preamble. Without naming, + /// the model has no preamble-level nudge to call them, so the live + /// 2026-05-12 session reproduced the failure: user asks "what is my dog's + /// name?" and the agent answers from the void without searching. + func test_preambleNamesMemoryTools() { + let preamble = ContextBuilder.selfAwarePreamble + XCTAssertTrue( + preamble.contains("search_memory"), + "Issue #14: selfAwarePreamble must name `search_memory` so the model is cued to call it when the user references prior context." + ) + XCTAssertTrue( + preamble.contains("forget_fact"), + "Issue #14: selfAwarePreamble must name `forget_fact` alongside `search_memory` — the read/write pair of the memory surface." + ) + } } diff --git a/packages/Memory/Sources/Memory/HybridSearch.swift b/packages/Memory/Sources/Memory/HybridSearch.swift index f365e52..49b2867 100644 --- a/packages/Memory/Sources/Memory/HybridSearch.swift +++ b/packages/Memory/Sources/Memory/HybridSearch.swift @@ -31,8 +31,18 @@ public actor HybridSearch { public func searchFacts(query: String, k: Int = 10, triggerTurnId: Int64) async throws -> [FactRef] { guard !query.isEmpty else { return [] } + // Audit 2026-05-12 / M-5 (Issue #16): FTS5 MATCH grammar rejects raw + // natural-language punctuation. Sanitize before binding to the + // positional parameter; short-circuit if no usable tokens remain + // (binding an empty MATCH is itself a syntax error). Embedding still + // runs on the raw query so semantic recall isn't degraded by the + // FTS-safe rewrite. + guard let safeQuery = MemoryQueries.sanitizeFTS5Query(query) else { + logger.debug("searchFacts: sanitize returned no tokens; short-circuit queryLen=\(query.count) k=\(k) triggerTurnId=\(triggerTurnId)") + return [] + } let embedding = try await embedder.embed(query) - let rows = try await store.runHybridSearchSQL(query: query, embedding: embedding, k: k) + let rows = try await store.runHybridSearchSQL(query: safeQuery, embedding: embedding, k: k) let now = Int64(Date().timeIntervalSince1970 * 1000) var refs: [FactRef] = [] refs.reserveCapacity(rows.count) diff --git a/packages/Memory/Sources/Memory/MemoryExtractionOrchestrator.swift b/packages/Memory/Sources/Memory/MemoryExtractionOrchestrator.swift index b10b536..e91a7a8 100644 --- a/packages/Memory/Sources/Memory/MemoryExtractionOrchestrator.swift +++ b/packages/Memory/Sources/Memory/MemoryExtractionOrchestrator.swift @@ -119,13 +119,19 @@ public actor MemoryExtractionOrchestrator { assistantText: job.assistantText, priorActiveFacts: priorFacts ) - // Stable, but currently best-effort: TurnID is a UUID-string - // wrapper; we hash it to Int64 for the DB column. Plan 07-03's - // search path can use the hash to correlate facts back to turns. - let turnIdInt: Int64 = stableHash(job.turnId.rawValue) + // Audit 2026-05-12 / M-4 (Issue #15): `facts.source_turn_id` is + // no longer derived from an FNV-1a hash of `TurnID.rawValue`. + // The hash was Phase-7-era and conflated two disjoint number + // systems (TurnID UUID strings vs. turns.id INTEGER rowid), + // storing a value that joined nothing. We now pass 0 as a + // sentinel meaning "no correlated turn rowid"; fact-to-turn + // correlation is best done via `valid_from` timestamp proximity + // until a future change threads the real `turns.id` rowid back + // from `appendTurn`. Schema comment was updated to match. + let sentinelSourceTurnId: Int64 = 0 for op in ops { do { - try await applyOp(op, turnIdInt) + try await applyOp(op, sentinelSourceTurnId) } catch { logger.error("memory applyOp failed: \(error)") } @@ -134,20 +140,4 @@ public actor MemoryExtractionOrchestrator { logger.error("memory extraction failed for turnId=\(job.turnId.rawValue): \(error)") } } - - /// Stable 64-bit hash of an arbitrary string. We use FNV-1a so the - /// hash is identical across process launches (Swift's built-in - /// `String.hashValue` is randomized per process and can't be persisted - /// to a SQLite column for correlation). - private func stableHash(_ s: String) -> Int64 { - var hash: UInt64 = 0xcbf29ce484222325 - let prime: UInt64 = 0x100000001b3 - for byte in s.utf8 { - hash ^= UInt64(byte) - hash = hash &* prime - } - // Fold to signed Int64 — drop the sign bit so the value stays - // positive (SQLite Int64 column indexes nicer when always positive). - return Int64(hash & 0x7FFFFFFFFFFFFFFF) - } } diff --git a/packages/Memory/Sources/Memory/MemoryQueries.swift b/packages/Memory/Sources/Memory/MemoryQueries.swift index 255f892..604b0f9 100644 --- a/packages/Memory/Sources/Memory/MemoryQueries.swift +++ b/packages/Memory/Sources/Memory/MemoryQueries.swift @@ -115,4 +115,60 @@ public enum MemoryQueries { ORDER BY created_at DESC LIMIT ?; """ + + /// Audit 2026-05-12 / M-5 (Issue #16) — FTS5-safe query rewriter. + /// + /// `hybridSearchSQL` binds the query text directly to `facts_fts MATCH ?`. + /// FTS5's MATCH grammar accepts terms, AND/OR/NOT/NEAR, column filters, + /// and phrase quoting — but raw natural language with `?`, `'`, `:`, `-`, + /// `*`, or other reserved characters throws `fts5: syntax error` at the + /// SQLite layer. Live probe before this fix: + /// sqlite3 jarvis.db "SELECT rowid FROM facts_fts + /// WHERE facts_fts MATCH 'what is my dog name?'" + /// → Error: stepping, fts5: syntax error near "?" + /// + /// Rewriter strategy (simplest defensible): + /// 1. Replace anything that isn't a unicode letter or digit with a + /// space. This drops `?`, `'`, `:`, `-`, `*`, `(`, `)`, `"`, etc. + /// 2. Split on whitespace. + /// 3. Wrap each remaining token in double quotes, escaping any + /// embedded `"` per FTS5 grammar (doubled `""`). The class-1 + /// filter strips `"` characters, so step (3) escape is belt-and- + /// suspenders against future relaxation of the class filter. + /// 4. Join tokens with spaces. + /// + /// Turns `"what is my dog's name?"` into + /// `"what" "is" "my" "dog" "s" "name"` — a FTS5-safe phrase-OR query. + /// Returns nil if no usable tokens remain (caller short-circuits to + /// empty results rather than binding an empty MATCH that would itself + /// be a syntax error). + /// + /// Long-term we want a real query AST; this is the minimum fix that + /// stops the footgun #14 (preamble add) is about to wire up. + public static func sanitizeFTS5Query(_ raw: String) -> String? { + // Step 1: replace non-alphanumeric (per Unicode) with space. + let scalarsKept = raw.unicodeScalars.map { scalar -> Character in + if CharacterSet.alphanumerics.contains(scalar) { + return Character(scalar) + } + return " " + } + let scrubbed = String(scalarsKept) + + // Step 2: split on whitespace, drop empties. + let tokens = scrubbed + .split(whereSeparator: { $0.isWhitespace }) + .map { String($0) } + .filter { !$0.isEmpty } + + guard !tokens.isEmpty else { return nil } + + // Step 3 + 4: double-quote each token (escape any internal " per + // FTS5 — doubled), join with spaces. + let quoted = tokens.map { token -> String in + let escaped = token.replacingOccurrences(of: "\"", with: "\"\"") + return "\"\(escaped)\"" + } + return quoted.joined(separator: " ") + } } diff --git a/packages/Memory/Sources/Memory/MemorySchema.swift b/packages/Memory/Sources/Memory/MemorySchema.swift index 100a21c..cf05d36 100644 --- a/packages/Memory/Sources/Memory/MemorySchema.swift +++ b/packages/Memory/Sources/Memory/MemorySchema.swift @@ -32,18 +32,17 @@ public enum MemorySchema { // Facts table — temporal validity + supersede chain + soft delete. // - // `source_turn_id` is a CORRELATION TAG, NOT a foreign key. The - // memory-extraction writer (`MemoryExtractionOrchestrator.process`) - // FNV-1a-hashes the originating `TurnID` UUID to an Int64 because - // `TurnID` is a string-backed UUID and the writer needs an integer - // column. The hash space and `turns.id` (AUTOINCREMENT) never - // collide by design — they're disjoint number systems — so any - // `REFERENCES turns(id)` on this column would fail every insert - // with SQLITE_CONSTRAINT_FOREIGNKEY (code 19). The constraint was - // present in the original schema and silently broke fact persistence - // for the entire history of the project until the 2026-05-12 dogfood - // surfaced it. Reintroducing the FK without first wiring the writer - // to use real `turns.id` rowids would re-break persistence. + // `source_turn_id` is a CORRELATION TAG, NOT a foreign key, and as + // of audit 2026-05-12 / M-4 (Issue #15) it is effectively unused. + // The pre-fix writer FNV-1a-hashed the originating `TurnID` UUID to + // an Int64, but the hash space and `turns.id` (AUTOINCREMENT) were + // disjoint number systems — joining `facts.source_turn_id` against + // `turns.id` returned nothing on every query. Reintroducing a real + // `REFERENCES turns(id)` on this column would require threading the + // real rowid from `appendTurn` through the extraction pipeline; until + // that lands, the writer passes 0 as a sentinel meaning "no + // correlated turn rowid." Fact-to-turn correlation is via + // `valid_from` timestamp proximity. // // `superseded_by REFERENCES facts(id)` IS a real FK — the supersede // chain uses `priorFact.id` from a previous `applyOp` return value, diff --git a/packages/Memory/Sources/Memory/MemoryStore.swift b/packages/Memory/Sources/Memory/MemoryStore.swift index 3930fad..9c849e4 100644 --- a/packages/Memory/Sources/Memory/MemoryStore.swift +++ b/packages/Memory/Sources/Memory/MemoryStore.swift @@ -64,10 +64,26 @@ public actor MemoryStore { private let conn: SQLiteConnection private let logger: Logger + /// Audit 2026-05-12 / M-1 (Issue #12): writer-side embedder. When + /// non-nil, `insertNewFact` runs the embedder against the subject/ + /// predicate/object triple and writes a matching `facts_vec` row after + /// the facts row is committed. When nil, the writer skips the vec + /// insert entirely — degraded-but-searchable mode (FTS5 still works). + /// Embedder failures during the vec write are logged but never roll + /// back the facts row, per audit recommendation: "log + leave the FTS + /// row (degraded but searchable) rather than rolling back the fact row." + private let embedder: (any EmbeddingProviding)? + /// Open jarvis.db at databaseURL. Creates parent dir if absent. /// Registers vec0 as auto-extension (once per process). Applies pragmas /// + schema migration + vec_version sanity probe. Throws on any failure. - public init(databaseURL: URL) throws { + /// + /// `embedder` (audit 2026-05-12 / Issue #12): optional embedding + /// provider used by `applyOp` to populate `facts_vec` alongside `facts`. + /// When nil, facts_vec is never written; hybrid search degrades to + /// FTS5-only ranking (no semantic recall). Production wires + /// `OllamaEmbeddingClient` here in `installMemory`; tests inject stubs. + public init(databaseURL: URL, embedder: (any EmbeddingProviding)? = nil) throws { // Ensure parent directory exists. let parent = databaseURL.deletingLastPathComponent() try FileManager.default.createDirectory(at: parent, withIntermediateDirectories: true) @@ -81,6 +97,7 @@ public actor MemoryStore { self.conn = try SQLiteConnection.open(at: databaseURL) self.logger = Logger(label: Self.logChannel) + self.embedder = embedder // 1. Pragmas (WAL + synchronous + busy_timeout + foreign_keys + temp_store). for sql in MemorySchema.pragmas { @@ -128,14 +145,18 @@ public actor MemoryStore { /// Apply a single mem0 operation. The ONLY emission site for /// `ReplayEvent.memoryMutation` (MEM-06). + /// + /// Audit 2026-05-12 / M-1 (Issue #12): now `async` because `insertNewFact` + /// awaits the optional embedder to populate `facts_vec` alongside `facts`. + /// Pre-fix this was sync and `facts_vec` was never written. @discardableResult - public func applyOp(_ op: MemoryOp, sourceTurnId: Int64) throws -> Fact? { + public func applyOp(_ op: MemoryOp, sourceTurnId: Int64) async throws -> Fact? { switch op { case .noop: logger.debug("memory NOOP for turnId=\(sourceTurnId)") return nil case .add(let subject, let predicate, let object, _): - return try insertNewFact( + return try await insertNewFact( subject: subject, predicate: predicate, object: object, @@ -143,7 +164,7 @@ public actor MemoryStore { sourceTurnId: sourceTurnId ) case .update(let supersedes, let subject, let predicate, let object, _): - return try insertNewFact( + return try await insertNewFact( subject: subject, predicate: predicate, object: object, @@ -160,7 +181,7 @@ public actor MemoryStore { object: String, supersedesFactId: Int64?, sourceTurnId: Int64 - ) throws -> Fact { + ) async throws -> Fact { let now = Int64(Date().timeIntervalSince1970 * 1000) do { try conn.beginTransaction() @@ -228,6 +249,13 @@ public actor MemoryStore { createdAt: now ) + // Audit 2026-05-12 / M-1 (Issue #12): write the matching + // facts_vec row so semantic search has a cohort. Done AFTER + // commit so embedder failure (network, model unavailable, + // dimension mismatch) leaves the facts row + FTS5 row intact — + // degraded-but-searchable per audit recommendation. + await writeFactsVecRow(for: fact) + recordMemoryMutation( op: supersedesFactId == nil ? "ADD" : "UPDATE", fact: fact, @@ -245,6 +273,38 @@ public actor MemoryStore { } } + /// Audit 2026-05-12 / M-1 (Issue #12): populate `facts_vec` with the + /// embedding for a freshly-inserted fact. Best-effort — embedder errors + /// and SQLite errors are logged but never thrown; the facts row stays + /// (FTS5 search still works, vec cohort is empty for this fact). + /// + /// No-op when the store was constructed without an embedder (the test + /// surface for that path is `MemoryStoreVecWriteTests`). + private func writeFactsVecRow(for fact: Fact) async { + guard let embedder else { return } + let text = "\(fact.subject) \(fact.predicate) \(fact.object)" + let embedding: [Float] + do { + embedding = try await embedder.embed(text) + } catch { + logger.warning("writeFactsVecRow: embedder failed for factId=\(fact.id): \(error). facts_vec row absent; FTS5 row remains.") + return + } + guard embedding.count == MemoryConstants.embeddingDim else { + logger.warning("writeFactsVecRow: embedder returned wrong dim (\(embedding.count) vs expected \(MemoryConstants.embeddingDim)) for factId=\(fact.id); skipping vec insert.") + return + } + let blob = embedding.withUnsafeBufferPointer { Data(buffer: $0) } + do { + try conn.exec( + "INSERT INTO facts_vec(fact_id, embedding) VALUES (?, ?);", + bindings: [.int(fact.id), .blob(blob)] + ) + } catch { + logger.warning("writeFactsVecRow: facts_vec insert failed for factId=\(fact.id): \(error). FTS5 row remains.") + } + } + private func recordMemoryMutation( op: String, fact: Fact, diff --git a/packages/Memory/Tests/MemoryTests/HybridSearchTests.swift b/packages/Memory/Tests/MemoryTests/HybridSearchTests.swift index fd16990..8a5c167 100644 --- a/packages/Memory/Tests/MemoryTests/HybridSearchTests.swift +++ b/packages/Memory/Tests/MemoryTests/HybridSearchTests.swift @@ -42,6 +42,91 @@ final class HybridSearchTests: XCTestCase { ) } + // MARK: - Audit 2026-05-12 / M-5 — FTS5 query sanitization (Issue #16) + // + // `hybridSearchSQL` binds the query string directly to `facts_fts MATCH ?`. + // Real user queries like "what is my dog's name?" contain `?`, `'`, and + // other FTS5-reserved characters that throw `fts5: syntax error` at the + // SQLite layer. Sanitization in HybridSearch.searchFacts is the smallest + // safe rewrite: strip punctuation, split on whitespace, double-quote each + // term, join with spaces. Turns "what is my dog's name?" into + // `"what" "is" "my" "dog" "s" "name"`. FTS5-safe phrase-OR query. + + func testSearchFactsSanitizesPunctuationBeforeFTS5() async throws { + let embedder = MockEmbedder() + let store = MockStore() + await store.setRows([]) + let search = HybridSearch(store: store, embedder: embedder) + + _ = try await search.searchFacts( + query: "what is my dog's name?", + k: 5, + triggerTurnId: 1 + ) + + let captured = await store.capturedQuery + XCTAssertNotNil(captured, "store must receive a sanitized query") + let safeQuery = captured ?? "" + // Must NOT contain raw FTS5-reserved punctuation that breaks MATCH. + XCTAssertFalse(safeQuery.contains("?"), + "Issue #16: sanitized query must not contain raw `?` — FTS5 syntax error") + XCTAssertFalse(safeQuery.contains("'"), + "Issue #16: sanitized query must not contain raw `'` — FTS5 syntax error") + // Must preserve the meaningful tokens so search remains useful. + XCTAssertTrue(safeQuery.contains("dog"), + "Sanitization must preserve `dog` token") + XCTAssertTrue(safeQuery.contains("name"), + "Sanitization must preserve `name` token") + } + + func testSearchFactsSanitizesQuotesAndOperators() async throws { + let embedder = MockEmbedder() + let store = MockStore() + await store.setRows([]) + let search = HybridSearch(store: store, embedder: embedder) + + // FTS5 operators (NEAR, AND, OR, NOT, *, :, parens) + embedded quote. + _ = try await search.searchFacts( + query: "remember when I said \"hi\" *or* something?", + k: 5, + triggerTurnId: 1 + ) + + let captured = await store.capturedQuery + XCTAssertNotNil(captured) + let safeQuery = captured ?? "" + // The embedded raw quote must be escaped (FTS5 doubles internal " + // inside a phrase) so we don't ship an unterminated phrase to SQLite. + // The wrapper double-quotes EACH token so any token containing a raw + // " gets the inner " doubled per FTS5 grammar. + // Sanity: must still be non-empty (we didn't strip everything). + XCTAssertFalse(safeQuery.isEmpty, "sanitized query must not be empty") + // Star alone gets stripped (it's a prefix operator without a term); + // useful tokens like `remember`, `said`, `hi`, `something` survive. + XCTAssertTrue(safeQuery.contains("remember")) + XCTAssertTrue(safeQuery.contains("something")) + } + + func testSearchFactsEmptyAfterSanitizeReturnsEmpty() async throws { + let embedder = MockEmbedder() + let store = MockStore() + await store.setRows([]) + let search = HybridSearch(store: store, embedder: embedder) + + // All-punctuation query → after stripping, no terms remain. + // Must short-circuit: never call store (would bind empty MATCH which + // is itself an FTS5 syntax error). + let refs = try await search.searchFacts( + query: "??!?...", + k: 5, + triggerTurnId: 1 + ) + XCTAssertEqual(refs.count, 0) + let captured = await store.capturedQuery + XCTAssertNil(captured, + "Issue #16: when sanitization yields no terms, search must short-circuit and never bind an empty MATCH to FTS5.") + } + func testSearchFactsCallsEmbedderOnce() async throws { let embedder = MockEmbedder() let store = MockStore() diff --git a/packages/Memory/Tests/MemoryTests/MemoryExtractionOrchestratorTests.swift b/packages/Memory/Tests/MemoryTests/MemoryExtractionOrchestratorTests.swift index 7e2097f..8eb072c 100644 --- a/packages/Memory/Tests/MemoryTests/MemoryExtractionOrchestratorTests.swift +++ b/packages/Memory/Tests/MemoryTests/MemoryExtractionOrchestratorTests.swift @@ -301,6 +301,52 @@ final class MemoryExtractionOrchestratorTests: XCTestCase { await orchestrator.shutdown() } + /// Audit 2026-05-12 / M-4 (Issue #15) — `MemoryExtractionOrchestrator.process` + /// must NOT pass an FNV-1a hash of the TurnID UUID as `sourceTurnId`. + /// Pre-fix that value was a meaningless 64-bit hash that joined nothing + /// in the `turns` table. The sentinel 0 is the cleanest signal that + /// fact-to-turn correlation is "not yet wired" without storing nonsense. + /// (Audit option 1: drop the hash; future change may thread the real + /// `turns.id` rowid via `appendTurn` → `flushPair` → `ExtractionJob`.) + func testProcessUsesSentinelSourceTurnIdNotFNVHash() async throws { + let provider = TimedMockProvider() + provider.argsJSON = try JSONSerialization.data(withJSONObject: ["ops": [ + ["op": "ADD", "subject": "user", "predicate": "has_dog_named", "object": "Toby"] + ]]) + let extractor = MemoryExtractor(provider: provider) + let spy = SpyApplyOp() + let orchestrator = MemoryExtractionOrchestrator( + extractor: extractor, + applyOp: { op, turnId in spy.record(op, sourceTurnId: turnId) } + ) + await orchestrator.start() + + // A real-shape TurnID — the kind that produced + // `7747877241524896942` (FNV hash) in today's live DB pre-fix. + let realTurnId = TurnID(rawValue: "B0E2A6FC-6B0A-4F92-A3F1-D2D8AC8C0D52") + await orchestrator.enqueue(ExtractionJob( + turnId: realTurnId, + userText: "My dog's name is Toby.", + assistantText: "Got it — Toby." + )) + try await Task.sleep(nanoseconds: 200_000_000) + + XCTAssertEqual(spy.calls.count, 1) + let sourceTurnId = spy.calls.first?.sourceTurnId ?? -1 + XCTAssertEqual( + sourceTurnId, 0, + "Issue #15: orchestrator must pass sentinel 0 — not an FNV hash — as sourceTurnId." + ) + // Defense in depth: the FNV-1a hash of this UUID would be a huge + // positive Int64; assert we never see one. + XCTAssertLessThan( + sourceTurnId, 1_000_000, + "Issue #15: sourceTurnId must not look like an FNV hash." + ) + + await orchestrator.shutdown() + } + /// Process applies returned ops to the store via the callback. func testProcessAppliesOpsToStore() async throws { let provider = TimedMockProvider() diff --git a/packages/Memory/Tests/MemoryTests/MemoryStoreVecWriteTests.swift b/packages/Memory/Tests/MemoryTests/MemoryStoreVecWriteTests.swift new file mode 100644 index 0000000..98f06d8 --- /dev/null +++ b/packages/Memory/Tests/MemoryTests/MemoryStoreVecWriteTests.swift @@ -0,0 +1,134 @@ +import XCTest +import Foundation +import AgentCore +import Replay +@testable import Memory + +/// Audit 2026-05-12 / M-1 (Issue #12) — facts_vec write-side coverage. +/// +/// The schema declares `facts_vec` (sqlite-vec virtual table) and the read +/// path's RRF SQL joins against it, but before this fix no producer ever +/// wrote into it. Result: the `vc` CTE in `hybridSearchSQL` returned zero +/// rows on every call and RRF degenerated to FTS5-only ranking. Semantic +/// recall ("dog" → "Toby" via embedding similarity) silently didn't work +/// despite vec0 being loaded and the embedder client being healthy. +/// +/// Test plan: +/// - applyOp(.add) with a stub embedder injected → assert exactly one +/// facts_vec row exists for the new fact's id. +/// - applyOp(.add) with NO embedder injected → fact still persists with +/// FTS5 row; facts_vec row absent (degraded-but-searchable mode). +/// - applyOp(.add) with an embedder that throws → fact still persists; +/// embedder failure must not roll back the facts row. +final class MemoryStoreVecWriteTests: XCTestCase { + + /// Stub embedder returning a constant 768-dim vector — no call recording, + /// no scripting. (Per project convention: Stub*, not Mock*.) + actor StubEmbedder: EmbeddingProviding { + let vector: [Float] + init(value: Float = 0.1) { + self.vector = Array(repeating: value, count: MemoryConstants.embeddingDim) + } + func embed(_ input: String) async throws -> [Float] { + return vector + } + } + + /// Stub embedder that always throws — used to verify the writer's + /// degraded-mode discipline (FTS row stays, vec row absent, fact still + /// persisted). + actor ThrowingEmbedder: EmbeddingProviding { + struct Boom: Error {} + func embed(_ input: String) async throws -> [Float] { + throw Boom() + } + } + + // MARK: - Tests + + func testApplyOpADDWritesFactsVecRowWhenEmbedderInjected() async throws { + let tmp = try Self.tempDB() + let store = try MemoryStore(databaseURL: tmp, embedder: StubEmbedder()) + + let fact = try await store.applyOp( + .add(subject: "user", predicate: "has_dog_named", object: "Toby", embedding: nil), + sourceTurnId: 0 + ) + XCTAssertNotNil(fact) + guard let fact else { return } + + // facts row count = 1, facts_vec row count = 1 (parity invariant). + let factsCount = try await store.queryRowCount("SELECT id FROM facts") + XCTAssertEqual(factsCount, 1) + + let vecCount = try await store.queryRowCount( + "SELECT fact_id FROM facts_vec WHERE fact_id = \(fact.id)" + ) + XCTAssertEqual( + vecCount, 1, + "Issue #12: facts_vec must contain exactly one row matching the new fact's id." + ) + } + + func testApplyOpADDPersistsFactWhenNoEmbedderInjected() async throws { + let tmp = try Self.tempDB() + // No embedder → facts row persists, facts_vec stays empty (degraded + // but searchable via FTS5 + recentActiveFacts). + let store = try MemoryStore(databaseURL: tmp, embedder: nil) + + let fact = try await store.applyOp( + .add(subject: "user", predicate: "has_dog_named", object: "Toby", embedding: nil), + sourceTurnId: 0 + ) + XCTAssertNotNil(fact) + guard let fact else { return } + + let factsCount = try await store.queryRowCount("SELECT id FROM facts") + XCTAssertEqual(factsCount, 1) + + let vecCount = try await store.queryRowCount( + "SELECT fact_id FROM facts_vec WHERE fact_id = \(fact.id)" + ) + XCTAssertEqual( + vecCount, 0, + "Without an embedder, facts_vec must NOT be populated — degraded but searchable mode." + ) + } + + func testApplyOpADDPersistsFactWhenEmbedderThrows() async throws { + let tmp = try Self.tempDB() + let store = try MemoryStore(databaseURL: tmp, embedder: ThrowingEmbedder()) + + // The embedder will throw — the fact INSERT must still succeed and + // be searchable via FTS5. Audit recommendation: "log + leave the FTS + // row (degraded but searchable) rather than rolling back the fact + // row." + let fact = try await store.applyOp( + .add(subject: "user", predicate: "has_dog_named", object: "Toby", embedding: nil), + sourceTurnId: 0 + ) + XCTAssertNotNil(fact) + guard let fact else { return } + + let factsCount = try await store.queryRowCount("SELECT id FROM facts") + XCTAssertEqual(factsCount, 1, + "Embedder failure must not roll back the facts row.") + + let vecCount = try await store.queryRowCount( + "SELECT fact_id FROM facts_vec WHERE fact_id = \(fact.id)" + ) + XCTAssertEqual( + vecCount, 0, + "When the embedder throws, facts_vec row must be absent (FTS row remains)." + ) + } + + // MARK: - helpers + + private static func tempDB() throws -> URL { + let tmp = FileManager.default.temporaryDirectory + .appendingPathComponent("memoryvecwrite-\(UUID().uuidString)", isDirectory: true) + try FileManager.default.createDirectory(at: tmp, withIntermediateDirectories: true) + return tmp.appendingPathComponent("jarvis.db") + } +} diff --git a/packages/Replay/Sources/Replay/ReplayLog.swift b/packages/Replay/Sources/Replay/ReplayLog.swift index f473331..9459e6e 100644 --- a/packages/Replay/Sources/Replay/ReplayLog.swift +++ b/packages/Replay/Sources/Replay/ReplayLog.swift @@ -218,45 +218,99 @@ public actor ReplayLog { try conn.commit() consecutiveFlushFailures = 0 } catch { - // ME-05: rollback the in-flight transaction, then re-buffer the - // batch at the FRONT of `pending` so the next flush attempt can - // retry. Pre-fix this silently dropped events on transaction - // failure, violating OBS-02's durability claim. try? conn.rollback() - // Capture the turn_ids so a forensic reader can identify which - // turns lost which events (the reviewer's required minimum). - let turnIds = Set(batch.map { $0.turnId.rawValue }) - .sorted().joined(separator: ",") - // Redact the error message in case sqlite3_errmsg ever echoes - // any caller-supplied bytes; defense-in-depth alongside HI-01. - let errStr = JarvisLogging.Redact.apply(String(describing: error)) - - consecutiveFlushFailures += 1 - let escalated = consecutiveFlushFailures >= Self.flushFailureEscalationThreshold - - // Re-buffer at the head; subsequent records append after. - pending.insert(contentsOf: batch, at: 0) - - if escalated { - // Chronic failure — surface as critical so the dev overlay - // / structured-log scrapers can flag it. Don't `fatalError`: - // OBS-02 says replay is best-effort and must never propagate - // into orchestrator turn semantics. - logger.critical("replay flush failing chronically (data-loss risk)", metadata: [ - "batch_size": "\(batch.count)", - "turn_ids": "\(turnIds)", - "consecutive_failures": "\(consecutiveFlushFailures)", - "error": "\(errStr)", - ]) - } else { - logger.error("replay flush failed; batch re-queued", metadata: [ - "batch_size": "\(batch.count)", - "turn_ids": "\(turnIds)", - "consecutive_failures": "\(consecutiveFlushFailures)", - "error": "\(errStr)", - ]) + // Audit 2026-05-12 / M-2 (Issue #13) — batch-splitting defense. + // + // Pre-fix: a single FK-violation in the batch (poisoned + // turn_id) took every other event in the same transaction + // down on rollback. The log re-buffered the whole batch and + // retried forever, escalating to CRITICAL after 3 attempts + // and burying real turn events behind a single bad row. + // + // New strategy on batch failure: retry each event in its OWN + // transaction. Good events persist; only the actually-bad + // rows are dropped. We log each drop at error level with the + // turn_id so a forensic reader can identify which row broke. + // Best-effort per OBS-02: replay never propagates into + // orchestrator semantics. + let initialError = error + var droppedCount = 0 + var droppedTurnIds: [String] = [] + for e in batch { + do { + try conn.beginTransaction() + try writeRow( + sql: "INSERT INTO events(turn_id, ts, monotonic_ns, kind, payload_bytes) VALUES (?, ?, ?, ?, ?);", + bindings: [ + .text(e.turnId.rawValue), + .int(e.ts), + .int(e.mono), + .text(e.kind), + .blob(e.payload), + ] + ) + try conn.commit() + } catch { + try? conn.rollback() + droppedCount += 1 + droppedTurnIds.append(e.turnId.rawValue) + } } + + if droppedCount == 0 { + // The single-row retry pass persisted everything — the + // batch transaction must have failed for a reason that + // disappeared between attempts (e.g., transient SQLite + // BUSY). Treat as success. + consecutiveFlushFailures = 0 + return + } + + if droppedCount == batch.count { + // Every row failed individually too — this isn't a single + // poisoned event, it's a chronic disk/WAL fault. Restore + // the original pre-fix behavior: re-buffer at the head + // for retry, count toward escalation. This preserves + // ME-05 durability for genuine I/O faults. + let turnIds = Set(batch.map { $0.turnId.rawValue }) + .sorted().joined(separator: ",") + let errStr = JarvisLogging.Redact.apply(String(describing: initialError)) + consecutiveFlushFailures += 1 + let escalated = consecutiveFlushFailures >= Self.flushFailureEscalationThreshold + pending.insert(contentsOf: batch, at: 0) + if escalated { + logger.critical("replay flush failing chronically (data-loss risk)", metadata: [ + "batch_size": "\(batch.count)", + "turn_ids": "\(turnIds)", + "consecutive_failures": "\(consecutiveFlushFailures)", + "error": "\(errStr)", + ]) + } else { + logger.error("replay flush failed; batch re-queued", metadata: [ + "batch_size": "\(batch.count)", + "turn_ids": "\(turnIds)", + "consecutive_failures": "\(consecutiveFlushFailures)", + "error": "\(errStr)", + ]) + } + return + } + + // Partial — some events persisted, some were dropped (the + // bounded-blast-radius case the batch split is designed for). + // Reset the consecutive-failure counter because the log is + // not stuck; surface each dropped row so a forensic reader + // can see which events died and why. + consecutiveFlushFailures = 0 + let droppedIds = Set(droppedTurnIds).sorted().joined(separator: ",") + let errStr = JarvisLogging.Redact.apply(String(describing: initialError)) + logger.error("replay batch-split: dropped poisoned rows; siblings persisted", metadata: [ + "batch_size": "\(batch.count)", + "dropped_count": "\(droppedCount)", + "dropped_turn_ids": "\(droppedIds)", + "error": "\(errStr)", + ]) } } diff --git a/packages/Replay/Tests/ReplayTests/ReplayLogTests.swift b/packages/Replay/Tests/ReplayTests/ReplayLogTests.swift index ef08143..8081ccc 100644 --- a/packages/Replay/Tests/ReplayTests/ReplayLogTests.swift +++ b/packages/Replay/Tests/ReplayTests/ReplayLogTests.swift @@ -316,4 +316,63 @@ final class ReplayLogTests: XCTestCase { let s = String(data: payload, encoding: .utf8) XCTAssertEqual(s, secret, "replay log is the authoritative byte record; redaction belongs to the viewer (P8)") } + + // MARK: - Audit 2026-05-12 / M-2 (Issue #13) — batch-splitting defense + // + // Live failure mode pre-fix: a single poisoned event (FK violation from + // a synthetic memory-trigger- turn_id) took down every other event + // in the same flush transaction. The replay log re-buffered the whole + // batch, retried, failed again, and at 3 consecutive failures escalated + // to CRITICAL. By the end of the day batch_size had grown to 31+ and + // consecutive_failures was 23+. Real turn events queued behind the + // poison were silently lost. + // + // Defense layer 2 (audit recommendation): when a batch transaction + // fails, ReplayLog now retries events individually so one poisoned row + // can only take itself down, not 30 siblings. + + func test_L10_batchSplitOnFKFailurePreservesOtherEvents() async throws { + let log = try ReplayLog(databaseURL: dbURL) + let session = try await log.beginSession(appVersion: "v", buildSHA: "s") + let goodTurn = TurnID.fresh() + try await log.startTurn( + turnId: goodTurn, sessionId: session, retryOf: nil, + turnNonce: "n", source: .text, + provider: "anthropic", modelId: "claude-opus-4-7" + ) + + // Two good events + one poison (synthetic turn_id not in `turns`). + // The whole batch will go through `flushPending` on endTurn. + await log.record(.textDelta("hello"), for: goodTurn) + await log.record(.textDelta("world"), for: goodTurn) + let poisonTurn = TurnID(rawValue: "memory-trigger-7747877241524896942") + await log.record(.memoryMutation(Data("{\"op\":\"ADD\"}".utf8)), for: poisonTurn) + await log.record(.textDelta("after"), for: goodTurn) + + await log.endTurn(goodTurn, stopReason: "end_turn") + + let conn = try SQLiteConnection.open(at: dbURL) + defer { try? conn.close() } + // The three good events + the turn_end event must persist even + // though the poison row could not be written. + let goodCount: Int64 = try conn.query( + "SELECT COUNT(*) FROM events WHERE turn_id=?;", + bindings: [.text(goodTurn.rawValue)], + map: { $0.columnInt(at: 0) } + ).first ?? 0 + XCTAssertEqual( + goodCount, 4, + "Issue #13: batch-splitting must preserve good events when one row in the batch FK-fails (3 textDelta + 1 turn_end)." + ) + + let poisonCount: Int64 = try conn.query( + "SELECT COUNT(*) FROM events WHERE turn_id=?;", + bindings: [.text(poisonTurn.rawValue)], + map: { $0.columnInt(at: 0) } + ).first ?? 0 + XCTAssertEqual( + poisonCount, 0, + "Poisoned row must NOT persist (FK violation) — but it also must not take its batch-siblings down." + ) + } }