Repository navigation
Python: fix(core): keep distinct non-ASCII memory topics in separate files - #9227
Yufeng He (he-yufeng) wants to merge 5 commits into
Conversation
…files _slugify_topic strips everything outside [a-z0-9], so two unrelated topics can collapse onto one filename: 旅行计划 and 饮食偏好 both fall through to the memory-topic fallback, café and cafè both become caf. The second write then reads back as the first topic with merged memories, and deleting one removes the shared file under both names. Keep the readable slug byte-identical when the derivation is lossless (slug == normalized.lower(), so existing safe ASCII stems are untouched) and suffix a short sha256 digest of the normalized topic otherwise, the same collision-resistance standard _storage_key_segment already applies to owner and source segments in this file. get_topic and delete_topic fall back to the pre-digest path when the new file is absent, and write_topic absorbs a legacy-named file into the new name, so stores written by older versions stay readable and migrate on first rewrite. Fixes microsoft#9205
GitHub Actions
Inactive
GitHub Actions
Inactive
agent-framework-automation
Bot
added
the
python
LI (ktz03)
left a comment
LI (ktz03)
left a comment
There was a problem hiding this comment.
Tested 61ed9bc18f6941b6e5ad90cfc851c12acac73ec4 through the public memory tools and MemoryFileStore with real temporary Markdown files. Fresh CJK/accented pairs are separated correctly, and safe ASCII stems retain their filenames. Same-topic rewrites also read and migrate files generated by the previous implementation.
There is still a blocking identity check missing from the legacy fallback and cleanup paths. A legacy filename is only a possible match; it can belong to a different topic:
store.write_topic(session, record("café", "accent fact"), source_id="memory")
store.write_topic(session, record("caf", "ASCII fact"), source_id="memory")
store.delete_topic(session, source_id="memory", topic="café")
# Both files are removed; the independent ASCII topic "caf" is now missing.Reversing the two writes removes caf.md during write_topic("café") itself. The public write_memory tool also still merges caf followed by café into one topic because get_topic("café") accepts caf.md without checking its stored title. The same issue appears during upgrade: a file generated by the previous implementation for café, followed by a write to distinct cafè, absorbs the new fact into café and leaves cafè unreadable.
Please verify that a fallback file belongs to the requested logical topic before reading, migrating, or deleting it, and add both write-order/delete controls alongside the valid same-topic migration tests. This should preserve explicit lookup by slug while preventing the fallback from treating an unrelated topic as a legacy alias.
Validation scope: Windows/Python 3.14.3, 16 public compatibility scenarios, including four upgrade cases using files generated in separate processes by the unchanged implementation. Cached dependencies; no full core suite, remote model, or other-platform validation.
AI assistance: AI-assisted review and reproduction.
The pre-digest fallback path treats any legacy-named file as an alias for the requested topic. The slug strips accents and non-ASCII text, so a same-stem file can belong to a different topic: after writing both "café" and "caf", deleting "café" removed "caf.md" too, writing "café" absorbed "caf.md", and a pre-digest file for "café" answered lookups for "cafè". A legacy-named file now counts as a fallback alias only when the topic recorded inside the file matches the requested topic; get_topic, delete_topic, and the write_topic absorb all gate on it. Same-topic migration of stores written by older versions is unchanged. Verified with the full agent-framework-core suite: 6728 tests, 0 failures; ruff and pyright clean on the changed files. Signed-off-by: Yufeng He <40085740+he-yufeng@users.noreply.github.com>
GitHub Actions
Inactive
|
Thanks for the careful review, and for running the real-file compatibility matrix. Your three scenarios all reproduced as genuine data loss, and the fix is in 864a5b5. The gap was that the legacy fallback treated any legacy-named file as an alias for the requested topic. The slug derivation is lossy by design (accents and non-ASCII fold away), so a same-stem file can belong to a different topic. The fix gates every legacy-path use on identity: a legacy-named file is a fallback alias only when the topic recorded in its
Your repros are pinned as regression tests: Verification: full |
LI (ktz03)
left a comment
LI (ktz03)
left a comment
There was a problem hiding this comment.
The legacy identity checks in 864a5b5cc75c54aaa7afd0fe2f00915c6e780cd8 address the earlier read/write/delete scenarios. Both write orders now preserve caf separately from café, and the valid same-topic migration controls still work.
There is another upgrade path that fails before a rewrite can migrate the file. list_topics() parses an existing caf.md headed # café and returns the new slug caf-850f7dc4. MemoryContextProvider.before_run() then selects the topic and calls get_topic(topic=entry.slug). That lookup searches for caf-850f7dc4.md, while the only persisted file is still caf.md; the legacy candidate cannot be derived from the already-digested slug. The result is FileNotFoundError, so a relevant prompt aborts during memory preparation.
Minimal reproduction using the actual previous layout and file contents:
import asyncio
from pathlib import Path
from tempfile import TemporaryDirectory
from agent_framework import (
AgentSession, MemoryContextProvider, MemoryFileStore, Message, SessionContext,
)
async def main():
with TemporaryDirectory() as root:
legacy = Path(root) / "memory/alice/memory/topics/caf.md"
legacy.parent.mkdir(parents=True)
legacy.write_text(
"# café\n\nUpdated: 2026-10-09T02:00:00Z\nSessions: -\n\n"
"## Summary\nlegacy fact\n\n## Memories\n- legacy fact\n",
encoding="utf-8",
)
session = AgentSession(session_id="one")
session.state["owner"] = "alice"
store = MemoryFileStore(root, owner_state_key="owner")
assert store.get_topic(session, source_id="memory", topic="café").memories == ["legacy fact"]
provider = MemoryContextProvider(store=store, recent_turns=0, max_extractions=0)
context = SessionContext(
session_id=session.session_id,
input_messages=[Message(role="user", contents=["legacy fact"])],
)
await provider.before_run(agent=None, session=session, context=context, state={})
asyncio.run(main())
# FileNotFoundError: No memory topic named 'caf-850f7dc4' was found for this owner.Please cover the complete existing-file → index → selected-topic injection path before a write, keeping the legacy heading identity checks. The index pointers should also resolve to persisted records before migration. Broadly trusting a legacy filename again would reintroduce the issue fixed in the previous revision.
I generated the legacy files with the unchanged implementation in separate processes. café, 旅行计划, and Travel Plans all pass the same automatic injection on the baseline and fail on this head; project-a remains a normal control. Explicit same-topic rewrites migrate the three files and make injection work again. The earlier 16 compatibility scenarios were also repeated successfully. Scope: Windows/Python 3.14.3, public provider/store/tool calls and real temporary files with cached dependencies; no full core suite, external model/service, or other OS validation.
AI assistance: AI-assisted review and reproduction.
list_topics parses a pre-digest file such as caf.md headed "# café" and returns the current digested slug, but the file on disk still uses the legacy name. before_run then calls get_topic(entry.slug) for the selected entry, where neither the digested path nor a legacy candidate derivable from that slug exists, so memory preparation dies with FileNotFoundError. Consolidation silently skips the topic the same way. A record whose file still sits at the pre-digest name for its topic is now keyed by the on-disk stem, so MEMORY.md pointers and get_topic(entry.slug) resolve to the persisted file before a rewrite migrates it. The heading identity check on the fallback paths is unchanged: a same-stem file owned by a different topic still does not alias. Verified with the full agent-framework-core suite: 8483 tests, 0 failures; ruff and pyright clean on the changed files. Signed-off-by: Yufeng He <40085740+he-yufeng@users.noreply.github.com>
GitHub Actions
Inactive
|
Both upgrade paths are covered in 1ba70db, thanks for the precise repro. The break was in The fix re-keys such records by the stem that actually exists on disk, only when the stem is exactly the pre-digest derivation of the parsed topic. Your repro is pinned as |
| if (path := self._legacy_topic_path(session, source_id=source_id, topic=value)) is not None | ||
| } | ||
| for legacy_path in legacy_paths: | ||
| if legacy_path.exists() and self._topic_file_matches(legacy_path, topic=record.topic): |
There was a problem hiding this comment.
Yufeng He (@he-yufeng) This cleanup can delete the file it just wrote when an explicit custom slug matches the legacy derived stem. For example, topic "café" with slug "caf" writes topics/caf.md, then derives the same path as legacy and unlinks it. Please exclude the current topic_path from legacy cleanup (and cover this explicit-slug case) so a valid public slug override cannot lose the topic.
There was a problem hiding this comment.
Done in 7579c98. The legacy-absorb set now excludes the path just written, so topic "café" with explicit slug "caf" keeps topics/caf.md instead of unlinking it in the same call. Covered by test_memory_file_store_write_with_explicit_slug_survives_legacy_cleanup, which fails against the previous code (the file is gone right after the write) and passes now; the full test_harness_memory.py file is green (37 passed) with ruff check and format clean.
Writing a topic whose explicit slug collides with the legacy stem re-derived from the topic deleted the file it had just written: the legacy path set was filtered against the topic-derived path only, not against the actual target. Skip the written path when absorbing pre-digest files and cover the explicit-slug case.
GitHub Actions
Active
| def _slugify_topic(topic: str) -> str: | ||
| normalized = _normalize_topic(topic) | ||
| slug = re.sub(r"[^a-z0-9]+", "-", normalized.lower()).strip("-") | ||
| if slug == normalized.lower(): |
There was a problem hiding this comment.
Yufeng He (@he-yufeng) Lowercasing is still treated as lossless here, so distinct preserved topics such as ProjectX and projectx both use projectx.md; the second write merges the first record and deleting either topic deletes the shared file. Please treat case folding as lossy and add a case-only collision regression.
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
MAF Automated Review — Iteration 1
Result: Findings reported
Scope: full PR (4 commit(s)): 61ed9bc18f69, 864a5b5cc75c, 1ba70dbd4afd, 7579c982e40f
Model: gpt-5.6-sol
Overview
The review found 4 verified inline finding(s).
Reviewed the supplied pull-request change set across correctness, security/reliability, architecture, and failure behavior.
4 verified findings remained after source verification (2 high, 2 medium) across 1 file. Details are attached to the affected lines below.
Affected areas: python/packages/core/agent_framework/_harness/_memory.py
| # punctuation): two distinct topics can share it. Suffix a digest of the | ||
| # normalized topic so byte-distinct topics get distinct files. | ||
| digest = hashlib.sha256(normalized.encode("utf-8")).hexdigest()[:8] | ||
| return f"{slug}-{digest}" if slug else f"memory-topic-{digest}" |
There was a problem hiding this comment.
The generated name shares the same namespace as literal safe topics: café and the literal topic caf-850f7dc4 both resolve to caf-850f7dc4.md. The second write therefore overwrites the first, and reads or deletes can target the other topic. Please make lossy-derived names disjoint from literal topic names (or otherwise detect and allocate around occupied identities) so both topics can coexist.
| # The readable slug is a lossy projection (non-ASCII, accents, case, | ||
| # punctuation): two distinct topics can share it. Suffix a digest of the | ||
| # normalized topic so byte-distinct topics get distinct files. | ||
| digest = hashlib.sha256(normalized.encode("utf-8")).hexdigest()[:8] |
There was a problem hiding this comment.
Eight hex characters provide only 32 bits of collision resistance, so same-prefix collisions are practical; for example, 旅行丂浮 and 旅行丅召 both become memory-topic-2c95bf3b.md. A second write then replaces the first topic and either name can read or delete the shared file. Please retain enough of the digest to make collisions infeasible, consistent with the full SHA-256 storage-key fallback used elsewhere in the package.
| # punctuation): two distinct topics can share it. Suffix a digest of the | ||
| # normalized topic so byte-distinct topics get distinct files. | ||
| digest = hashlib.sha256(normalized.encode("utf-8")).hexdigest()[:8] | ||
| return f"{slug}-{digest}" if slug else f"memory-topic-{digest}" |
There was a problem hiding this comment.
_slugify_topic adds nine uncapped bytes to losy slugs, so previously writable long topics can now raise ENAMETOLONG; any fix must preserve distinct, idempotent slugs and legacy-safe names while reserving space for .md.tmp.<pid>.
| return MemoryTopicRecord.from_markdown(topic_path.read_text(encoding="utf-8"), fallback_topic=topic) | ||
| for topic_path in self._topic_read_candidates(session, source_id=source_id, topic=topic): | ||
| if topic_path.exists(): | ||
| return MemoryTopicRecord.from_markdown(topic_path.read_text(encoding="utf-8"), fallback_topic=topic) |
There was a problem hiding this comment.
When this loads a record written with the documented stable slug override, parsing derives a new slug from the topic instead of retaining the on-disk stem. For example, topic="café", slug="caf" reloads with slug="caf-850f7dc4"; the next merge writes that path, removes caf.md, and callers can no longer use the stable caf slug. Please persist or otherwise recover explicit slug metadata so reload/rewrite preserves overrides while still distinguishing files that genuinely need legacy migration.
| if not topic_path.exists(): | ||
| raise FileNotFoundError(f"No memory topic named '{topic}' was found for this owner.") | ||
| return MemoryTopicRecord.from_markdown(topic_path.read_text(encoding="utf-8"), fallback_topic=topic) | ||
| for topic_path in self._topic_read_candidates(session, source_id=source_id, topic=topic): |
There was a problem hiding this comment.
Yufeng He (@he-yufeng) there is still an upgrade-path alias when the old legacy stem is also the requested new topic name. If an existing pre-digest file is topics/caf.md headed # café, then write_memory(topic="caf", ...) treats that primary path as the caf record before checking stored-topic identity, appends the fact to the café record, and migrates it instead of creating a distinct caf topic. Please handle a primary-path hit whose stored topic does not match the requested topic when that file is an unmigrated legacy file, for example by migrating the legacy record first and then writing the requested topic separately.
There was a problem hiding this comment.
Done in 5898505. _merge_memory now calls the new MemoryStore.migrate_misfiled_legacy_topic hook before resolving the record: when the primary path holds an unmigrated pre-digest file whose stored topic differs from the requested one (your caf.md/café case), that record is migrated to its derived name first and the requested topic is then written as a separate record.
I kept this strictness in the write path only. Reads by name-or-slug stay loose on purpose: the index keys unmigrated files by their on-disk stem, so get_topic("caf") resolving the legacy caf.md is the contract the index and provider rely on. As a sibling guard, delete_topic now skips a misfiled legacy file instead of deleting the other topic's data through this name, raising FileNotFoundError when nothing owned by the requested topic exists.
Regressions: writing caf against a legacy caf.md holding café creates a distinct caf record and preserves café under its derived name, and deleting caf in that layout raises FileNotFoundError with the café file untouched.
| # The readable slug is a lossy projection (non-ASCII, accents, case, | ||
| # punctuation): two distinct topics can share it. Suffix a digest of the | ||
| # normalized topic so byte-distinct topics get distinct files. | ||
| digest = hashlib.sha256(normalized.encode("utf-8")).hexdigest()[:8] |
There was a problem hiding this comment.
Yufeng He (@he-yufeng) the suffix uses only the first 8 hex chars of SHA-256, leaving 32 bits of collision resistance, so distinct non-ASCII topics can still share a file. For example, 丮仑一一 and 仺价一一 both have an empty readable slug and SHA-256 prefix 36749dfe, so both map to memory-topic-36749dfe.md; the second write would merge or clobber the first. Please use the full SHA-256 digest or an injective/longer storage-key encoding.
There was a problem hiding this comment.
Done in 5898505. _slugify_topic now suffixes the full 64-hex-char SHA-256 of the normalized topic instead of the 8-char prefix, so your 丮仑一一/仺价一一 pair (both memory-topic-36749dfe before) lands in distinct files. Verified your example locally: the prefixes do collide at 36749dfe, the full digests differ.
The regression test pins the prefix collision and asserts the two slugs are now distinct full-digest names. Safe stems stay byte-identical and idempotent as before.
GitHub Actions
Active
|
Thank you for the contribution. This pull request has had several outstanding correctness findings around memory-topic filename collisions, filename limits, and custom-slug restoration for more than three days without recent progress, so we are closing it for now to keep the review queue current. Please reopen it when you are ready to continue and address the outstanding items, and we will be happy to take another look. |
Motivation & Context
MemoryFileStorederives a topic's filename with_slugify_topic, which strips every character outside[a-z0-9]. Two unrelated topics can therefore collapse onto one file:旅行计划and饮食偏好both fall through to thememory-topicfallback,caféandcafèboth becomecaf. The second write reads back as the first topic with merged memories, anddelete_memory_topicon either name removes the shared file. Silent data loss for any non-ASCII deployment, reproduced against current main in #9205.Description & Review Guide
_slugify_topicnow returns its existing output unchanged when the derivation is lossless (slug == normalized.lower()), so every safe ASCII stem keeps the exact filename older versions wrote. When the projection discarded information (non-ASCII, accents, case, punctuation), it appends an 8-char sha256 digest of the normalized topic:café->caf-850f7dc4,旅行计划->memory-topic-f9d6fe13. This is the same collision-resistance standard_storage_key_segmentalready applies to owner and source segments in this file.get_topic/delete_topicfall back to the pre-digest path when the new file is absent, andwrite_topicabsorbs a legacy-named file into the new name so a migrated topic never lists twice.MemoryFileStore(_legacy_topic_path,_topic_read_candidates) and on whether keeping the readable prefix plus digest is the right trade against a full_storage_key_segmentswitch (that helper would base32-encode any non-safe topic, losing the readable stems this store exposes in tool output and index pointer lines).Related Issue
Fixes #9205
AI Assistance
AI assistance details: implementation and tests were drafted with AI assistance; I verified the reproduction, the fix, and the full core test suite locally.
Contribution Checklist
breaking changelabel (or add "[BREAKING]" to the title prefix, before or after any language prefix) — a workflow keeps the label and title prefix in sync automatically.