fix(memory): v0.1 CRIT cluster — facts_vec / replay correlation / FTS5 sanitize / search_memory preamble #153

2026-05-12T21:45:47Z

KofTwentyTwo commented

2026-05-12 21:45:47 +00:00

(Migrated from github.com)

Summary

Closes the Memory CRITICAL/HIGH cluster from the 2026-05-12 audit. All five issues are addressed by one commit each.

#13 (CRITICAL) — replay log poisoned by synthetic memory-trigger-<hash> turn_ids. AppDelegateMemoryReplaySink now attributes every memory event to a per-process sentinel TurnID that installAgent writes into replay.sqlite/turns after beginSession. ReplayLog.flushPending gains a batch-split fallback so one bad row can no longer take its 30 batch-siblings down. Real turn events queued behind a poison row now persist.
#15 (HIGH) — MemoryExtractionOrchestrator.process no longer FNV-1a-hashes the TurnID UUID into facts.source_turn_id. The hash joined nothing in the turns table. We pass 0 as a sentinel meaning "no correlated turn rowid"; the schema comment documents the trade-off (audit option 1).
#12 (CRITICAL) — MemoryStore.init accepts an optional embedder: any EmbeddingProviding?. insertNewFact runs the embedder against subject predicate object after the facts row commits and writes the matching facts_vec row. Failures log + leave the FTS row (degraded but searchable). AppDelegate.installMemory shares one OllamaEmbeddingClient between writer + reader sides.
#16 (HIGH) — MemoryQueries.sanitizeFTS5Query strips non-alphanumeric chars, splits on whitespace, double-quotes each token. HybridSearch.searchFacts calls it before binding to MATCH ?. Embedding still runs on the raw query so semantic recall isn't degraded. Short-circuits to empty results when no tokens remain.
#14 (CRITICAL) — ContextBuilder.selfAwarePreamble now names search_memory and forget_fact with the cue from SearchMemoryTool.toolDescription. Length still well under the 4096-char cache-eligibility boundary; the existing test_preambleDoesNotEnableCacheUnintentionally still passes.

Each fix has a TDD-style regression test:

test_preambleNamesMemoryTools (#14)
testSearchFactsSanitizesPunctuationBeforeFTS5 + testSearchFactsSanitizesQuotesAndOperators + testSearchFactsEmptyAfterSanitizeReturnsEmpty (#16)
testApplyOpADDWritesFactsVecRowWhenEmbedderInjected + testApplyOpADDPersistsFactWhenNoEmbedderInjected + testApplyOpADDPersistsFactWhenEmbedderThrows (#12)
testProcessUsesSentinelSourceTurnIdNotFNVHash (#15)
test_L10_batchSplitOnFKFailurePreservesOtherEvents (#13)

Verification

swift test --package-path packages/Memory — 98 tests, 10 skipped, 0 failures (skipped suite is env-gated real-models corpus).
swift test --package-path packages/Replay — 34 tests, 0 failures.
swift test --package-path packages/AgentCore — pre-existing tests + new preamble assertion all pass.
bash scripts/check-app-builds.sh — clean.
All 18 boundary gates green (for s in scripts/check-*.sh; do bash "$s"; done).

Notes for review

Empirical end-to-end verification of facts_vec writes and search round-trips needs the user's environment (local Ollama + nomic-embed-text + qwen2.5-coder:32b). The MemoryRegressionCorpusTests env-gated suite (#56/#58) is the right surface for that; default CI runs the stub-embedder tests in MemoryStoreVecWriteTests which prove the wire shape without touching the network.
MemoryStore.applyOp is now async throws (was throws). All call sites already used try await because MemoryStore is an actor, so the change is transparent.
AppDelegateMemoryReplaySink gains a sentinelTurnID: TurnID field; the matching turns row is inserted in installAgent right after beginSession. Until that row exists, sink emissions still flow but get dropped via the new batch-split path — the chain stays uncorrupted.

Test plan

User restarts the app and confirms ~/Library/Logs/Jarvis/replay.<date>.log no longer shows "replay flush failing chronically" or consecutive_failures climbing.
User asks "what is my dog's name?" — agent calls search_memory (visible in ~/Library/Logs/Jarvis/memory.<date>.log [memory.search] channel) BEFORE responding.
User confirms a new fact added via "remember X" results in both a facts row AND a facts_vec row (sqlite3 jarvis.db "SELECT COUNT(*) FROM facts; SELECT COUNT(*) FROM facts_vec_rowids;").
User confirms natural-language search queries with ?, ', etc. don't throw FTS5 syntax errors.

🤖 Generated with Claude Code

## Summary Closes the Memory CRITICAL/HIGH cluster from the 2026-05-12 audit. All five issues are addressed by one commit each. - **#13 (CRITICAL)** — replay log poisoned by synthetic `memory-trigger-<hash>` turn_ids. `AppDelegateMemoryReplaySink` now attributes every memory event to a per-process **sentinel TurnID** that `installAgent` writes into `replay.sqlite/turns` after `beginSession`. `ReplayLog.flushPending` gains a **batch-split** fallback so one bad row can no longer take its 30 batch-siblings down. Real turn events queued behind a poison row now persist. - **#15 (HIGH)** — `MemoryExtractionOrchestrator.process` no longer FNV-1a-hashes the TurnID UUID into `facts.source_turn_id`. The hash joined nothing in the `turns` table. We pass `0` as a sentinel meaning "no correlated turn rowid"; the schema comment documents the trade-off (audit option 1). - **#12 (CRITICAL)** — `MemoryStore.init` accepts an optional `embedder: any EmbeddingProviding?`. `insertNewFact` runs the embedder against `subject predicate object` after the facts row commits and writes the matching `facts_vec` row. Failures log + leave the FTS row (degraded but searchable). `AppDelegate.installMemory` shares one `OllamaEmbeddingClient` between writer + reader sides. - **#16 (HIGH)** — `MemoryQueries.sanitizeFTS5Query` strips non-alphanumeric chars, splits on whitespace, double-quotes each token. `HybridSearch.searchFacts` calls it before binding to `MATCH ?`. Embedding still runs on the raw query so semantic recall isn't degraded. Short-circuits to empty results when no tokens remain. - **#14 (CRITICAL)** — `ContextBuilder.selfAwarePreamble` now names `search_memory` and `forget_fact` with the cue from `SearchMemoryTool.toolDescription`. Length still well under the 4096-char cache-eligibility boundary; the existing `test_preambleDoesNotEnableCacheUnintentionally` still passes. Each fix has a TDD-style regression test: - `test_preambleNamesMemoryTools` (#14) - `testSearchFactsSanitizesPunctuationBeforeFTS5` + `testSearchFactsSanitizesQuotesAndOperators` + `testSearchFactsEmptyAfterSanitizeReturnsEmpty` (#16) - `testApplyOpADDWritesFactsVecRowWhenEmbedderInjected` + `testApplyOpADDPersistsFactWhenNoEmbedderInjected` + `testApplyOpADDPersistsFactWhenEmbedderThrows` (#12) - `testProcessUsesSentinelSourceTurnIdNotFNVHash` (#15) - `test_L10_batchSplitOnFKFailurePreservesOtherEvents` (#13) ## Verification - `swift test --package-path packages/Memory` — 98 tests, 10 skipped, 0 failures (skipped suite is env-gated real-models corpus). - `swift test --package-path packages/Replay` — 34 tests, 0 failures. - `swift test --package-path packages/AgentCore` — pre-existing tests + new preamble assertion all pass. - `bash scripts/check-app-builds.sh` — clean. - All 18 boundary gates green (`for s in scripts/check-*.sh; do bash "$s"; done`). ## Notes for review - **Empirical end-to-end verification** of `facts_vec` writes and search round-trips needs the user's environment (local Ollama + `nomic-embed-text` + `qwen2.5-coder:32b`). The `MemoryRegressionCorpusTests` env-gated suite (#56/#58) is the right surface for that; default CI runs the stub-embedder tests in `MemoryStoreVecWriteTests` which prove the wire shape without touching the network. - `MemoryStore.applyOp` is now `async throws` (was `throws`). All call sites already used `try await` because `MemoryStore` is an actor, so the change is transparent. - `AppDelegateMemoryReplaySink` gains a `sentinelTurnID: TurnID` field; the matching `turns` row is inserted in `installAgent` right after `beginSession`. Until that row exists, sink emissions still flow but get dropped via the new batch-split path — the chain stays uncorrupted. ## Test plan - [ ] User restarts the app and confirms `~/Library/Logs/Jarvis/replay.<date>.log` no longer shows "replay flush failing chronically" or `consecutive_failures` climbing. - [ ] User asks "what is my dog's name?" — agent calls `search_memory` (visible in `~/Library/Logs/Jarvis/memory.<date>.log` `[memory.search]` channel) BEFORE responding. - [ ] User confirms a new fact added via "remember X" results in both a `facts` row AND a `facts_vec` row (`sqlite3 jarvis.db "SELECT COUNT(*) FROM facts; SELECT COUNT(*) FROM facts_vec_rowids;"`). - [ ] User confirms natural-language search queries with `?`, `'`, etc. don't throw FTS5 syntax errors. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: KofTwentyTwo/Jarvis#153