fix(memory): enforce authoritative revision integrity - #1303
Conversation
Teingi
left a comment
Teingi
left a comment
There was a problem hiding this comment.
Thanks for tightening the Memory integrity boundary. The original issue is real and the main reproducer is addressed, but the three cases below still allow revisions outside the documented contract. Requesting changes until these integrity gaps are closed.
| ) | ||
| ).mappings() | ||
| stored_rows = tuple(rows) | ||
| if any(str(row["entry_version_id"]) in new_by_id for row in stored_rows): |
There was a problem hiding this comment.
[P1] Make version-ID uniqueness database-enforced
Could we make this uniqueness check database-enforced? The SELECT and later INSERT are not atomic. On OceanBase, two transactions can both observe no row and insert the same entry_version_id for different memory_artifact_id values, because the current primary key and unique constraints include the Memory ID. Both commits can therefore succeed, leaving an ID that this code treats as scope-global duplicated. Please add a database-level UNIQUE (scope_id, entry_version_id) with an explicit migration for existing tables, translate the resulting integrity error, and cover it with a coordinated two-connection race where both reads complete before either insert.
| canonical_base: Memory | None, | ||
| new_by_entry: Mapping[str, MemoryEntryVersion], | ||
| ) -> None: | ||
| previous_ids = tuple( |
There was a problem hiding this comment.
[P1] Validate history already referenced by the base
This only collects predecessors for value.entry_versions, so an invalid version already referenced by canonical_base is never rechecked. I reproduced this by changing a persisted v1 row to previous_version_id="does-not-exist" and then appending an unrelated entry: the commit succeeds and the new Revision still references the broken history. Because older releases could already persist this state, please either validate the base-referenced chain here or provide a one-time audit/migration plus database constraints that make this assumption safe.
| and change.from_entry_version_id == previous.entry_version_id | ||
| and change.to_entry_version_id == current.entry_version_id | ||
| and current.entry_version_id != previous.entry_version_id | ||
| and current.state == previous.state |
There was a problem hiding this comment.
[P2] Reject semantic revisions of inactive entries
This accepts a revise whenever the state is unchanged, including inactive -> inactive. A direct SPI writer can therefore deactivate an entry containing “black tea” and commit an inactive revision containing “green tea”, although the Memory contract requires reactivation before a semantic revision. organize(normalize) still needs a narrow exception, so please allow an inactive revision only when the new and predecessor canonical content bytes are identical and the change is normalization; otherwise require reactivation first.
| except (TypeError, ValueError) as error: | ||
| raise _InvalidMemoryCommitError("vector") from error | ||
| if canonical != projection.embedding or expected_hash != projection.embedding_content_hash: | ||
| raise _InvalidMemoryCommitError("vector") |
There was a problem hiding this comment.
[P1] 不要用精确浮点相等判断单位向量已经规范化
MemoryService._embed_texts() 已先执行一次 canonical_embedding()；这里再次归一化后再做 tuple 精确比较。单位归一化不是位级幂等的：公开 MemoryService.remember() 探针让模型返回 (0.2407121489724894, -0.9705965492093231)，服务层得到 (0.24071214897248938, -0.9705965492093231)，这里二次归一化又把首项改成 0.24071214897248935，于是合法的有限非零向量被抛为 _InvalidMemoryCommitError("vector")，整个 Memory 写入失败。现有向量测试只使用 (1, 0, 0) 这类轴向量，无法覆盖该舍入路径。请改为校验维度、有限性和带容差的单位范数（或确保只规范化一次并复用结果），并补一个非轴向量的公开 MemoryService.remember() 回归测试。
3 participants
Which issue or RFC does this PR close?
Closes #1297.
Rationale for this change
MemoryUnitOfWork.commit()is the final persistence trust boundary, but it previously accepted incomplete or internally inconsistent revisions. A malformed direct SPI writer could persist illegal predecessor links or body/hash mismatches, and search could then return text that authoritative entry reads rejected.What changes are included in this PR?
entries(),expand(), projection reads, and every FTS/vector candidate against the exact authoritative revision, manifest, entry row, and current head projection.Are there any user-facing changes?
Malformed commits made through the public Memory backend SPI now fail with
MemoryBackendConfigurationErrorinstead of becoming durable state. Valid service workflows and the documented no-vector degradation remain unchanged. There are no API or persisted-format migrations.How was this change tested?
.venv/Scripts/prek.exe run -a.venv/Scripts/ty.exe check.venv/Scripts/python.exe -m pytest tests/builtin/artifacts/memory tests/builtin/persistence/test_memory.py tests/builtin/persistence/test_memory_integrity.py tests/e2e/test_memory_search_concurrency.py tests/e2e/test_builtin_runtime.py -q(39 passed, 5 skipped)612 passed, 14 skipped; 8 unrelated platform failures remain in pre-existing symlink/POSIX-mode,python3command, encoding, and path-separator assertions.AI usage statement
OpenAI Codex (GPT-5) was used to inspect the issue and codebase, implement the fix, design regression tests, and review the final change.