Skip to content

Python: give memory topic files an injective filename stem - #9724

Open
Manohar Paturi (ManoharPaturi) wants to merge 1 commit into
microsoft:mainfrom
ManoharPaturi:fix/memory-topic-slug-collision
Open

Manohar Paturi (ManoharPaturi) wants to merge 1 commit into
microsoft:mainfrom
ManoharPaturi:fix/memory-topic-slug-collision

Conversation

@ManoharPaturi

@ManoharPaturi Manohar Paturi (ManoharPaturi) commented Oct 26, 2026 •

Copy link
Copy Markdown
Contributor

Motivation & Context

MemoryFileStore keyed every topic file by _slugify_topic(topic), which is many to one: C ++ and C # both slug to c. write_topic overwrites the slug path unconditionally, so writing a second topic that shares a slug silently destroyed the first topic's memories. Reproduced on main: after writing both topics, get_topic("C ++") returned the C # memory and the C ++ memory was gone. The store's own path components for owner, session, source, and scope all use the injective _storage_key_segment derivation for exactly this reason; the topic filename was the one mapping left below that standard.

Description & Review Guide

  • What are the major changes? The topic filename stem is now the readable slug plus an 8 character digest of the normalized topic, so distinct normalized topics never share a file. Writes path by the record's normalized topic, and reads resolve the digest stem first.
  • What is the impact of these changes? Topics that share a slug coexist with their own memories. Reads keep working for stores written before this change through two fallbacks: the legacy slug-only filename, then a slug-prefix scan that also serves lookups made by slug (several tests look topics up by record.slug). When a slug-only lookup is ambiguous because several topics share that slug, the file whose recorded topic matches the lookup wins, else the first stem in sorted order keeps the choice deterministic.
  • What do you want reviewers to focus on? The fallback chain in _existing_topic_path, and the regression test writing C ++ then C # and reading both back, which fails on main.

Related Issue

Fixes #9723

AI Assistance

  • No material AI assistance was used.
  • This is an AI-assisted contribution. I reviewed, understood, and verified all submitted content and accept responsibility for it.

Contribution Checklist

  • The code builds clean without any errors or warnings
  • All unit tests pass, and I have added new tests where possible
  • The PR follows the Contribution Guidelines
  • This PR links to an agreed issue with no competing open PR, or the Related Issue section documents a trivial-change or repository-automation exception.
  • This is not a breaking change. If it is a breaking change, add the breaking change label (or add "[BREAKING]" to the title prefix, before or after any language prefix) — a workflow keeps the label and title prefix in sync automatically.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Manohar Paturi (@ManoharPaturi) Four material issues remain in the memory topic storage changes:

  1. In python/packages/core/agent_framework/_harness/_memory.py:811-825, glob(f"{prefix}*.md") accepts longer unrelated slugs and returns the first match. A missing topic can therefore read, merge into, or delete another topic; colliding names such as C++ and C# can also merge. Please validate exact topic/slug identity rather than accepting arbitrary prefixes.
  2. In _memory.py:884-894, updating a legacy filename writes a new digest file without retiring the matching legacy file. Both remain visible, lookup can return stale content, and deletion can resurrect the legacy record. Please implement a one-authoritative-record migration/update lifecycle.
  3. In _memory.py:783-784, index links and provider selection still use the old nonunique slug instead of the actual persisted filename/exact topic identity. Collision-safe files can therefore produce broken links and duplicate the wrong topic during injection. Please use the persisted storage identity consistently.
  4. In _memory.py:886, writing from record.topic ignores the public MemoryTopicRecord(slug=...) override. Please preserve explicit slug lookup compatibility while introducing collision-safe storage identities.

Please address all four before merge.

Distinct topics can share a readable slug, so a slug-only stem let the second topic silently overwrite the first. The stem now appends a digest of the normalized topic, and the storage layer keeps one authoritative record: lookups validate every candidate against the record it actually holds instead of accepting a filename prefix, so a missing topic can never read, merge into, or delete a longer slug; rewriting a topic that lived at a legacy slug-only file retires the legacy file; index pointer links and provider lookups use the persisted stem; and an explicit slug override is persisted in the record and stays findable by that slug, without letting lookups for other names bleed into the overridden record.

Signed-off-by: Manohar Paturi <186662190+ManoharPaturi@users.noreply.github.com>
@ManoharPaturi

Copy link
Copy Markdown
Contributor Author

All four addressed in 8d4eb01, verified end to end against the store before pushing.

  1. Exact identity: _existing_topic_path no longer accepts a filename prefix. Every glob candidate is read and validated against the record it actually holds, first by normalized topic, and a missing topic such as coll under Collection now raises FileNotFoundError instead of reading the longer slug. Colliding C++ and C# each resolve to their own record, and looking up a bare C that matches neither record finds nothing rather than an arbitrary same-slug file.
  2. One authoritative record: write_topic resolves the previous path for the topic before writing and retires it when it differs, so rewriting a topic that lived at a legacy slug-only file leaves exactly the digest-stemmed file. A test seeds a legacy file, rewrites, and asserts the legacy file is gone and only the new stem remains.
  3. Persisted identity everywhere: index entries are built from record.storage_stem, so every pointer line links a file that exists, with a test asserting all links resolve when two topics share a slug, and injection consumes those same entries.
  4. Slug override: the explicit slug is persisted in the record file and used for the storage stem, so get_topic by that slug resolves the overridden record after a reload, while lookups for other names cannot bleed into it (a guard test asserts machine does not find the ml-notes record).

The five new tests fail on the previous head. The memory suite passes at 40 tests, ruff check and format are clean.

@ManoharPaturi

Copy link
Copy Markdown
Contributor Author

Eduard van Valkenburg (@eavanvalkenburg) the PR body now carries the required Contribution Checklist with the non-breaking item confirmed, matching the template. Everything from your review is addressed in the latest push and the thread replies have the verification details.

This branch was previously deployed

1 inactive deployment
github-app-auth — 8d4eb109 Deployed Oct 27, 2026 by ManoharPaturi via add_label #26286
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

python Usage: [Issues, PRs], Target: Python

Projects

None yet

Development

Successfully merging this pull request may close these issues.

.NET: Python: MemoryFileStore topic slug collisions silently overwrite another topic's memories

2 participants