[Manohar Paturi (ManoharPaturi)]

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.

[Eduard van Valkenburg (eavanvalkenburg)]

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.

[Manohar Paturi (ManoharPaturi)]

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.

[Manohar Paturi (ManoharPaturi)]

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.

[Eduard van Valkenburg (eavanvalkenburg)]

Manohar Paturi (@ManoharPaturi) Two additional cases at 8d4eb109 need attention:

1. In python/packages/core/agent_framework/_harness/_memory.py:854-857, default-slug reloads now fail. A normally written Travel Preferences record still reports slug="travel-preferences", but reloading through get_topic(..., record.slug) raises FileNotFoundError. Consolidation still reloads by that slug at line 1666 and catches the exception, silently omitting the topic; the forced consolidation probe processes 1 topic on base and 0 on this head. Please preserve unambiguous default-slug compatibility or make consolidation use the exact topic/persisted identity, without restoring ambiguous prefix matching.

2. In _memory.py:143-146, the 8-hex-character digest still permits silent overwrite. Both 主题乚丢一 and 主题亊不七 become memory-topic-74043f1d.md. Writing them with different memories leaves one record, and reading the first topic returns the second topic's fact. Please use a collision-resistant storage identity and validate the existing record's topic before replacing it, with a regression for this pair.

[Manohar Paturi (ManoharPaturi)]

Both addressed in cef2b8e, with your two probes reproduced on the previous head first.

1. Default slug reloads work again. A slug lookup now answers when exactly one record claims the slug, so Travel Preferences reloads through get_topic(record.slug). The rule only fires when the lookup string is itself a slug, because a topic name that merely slugifies into the family must not borrow a neighbor's file: your two CJK topics both slugify to memory-topic, and my first cut of the slug rule let the second write retire the first topic exactly that way, which is why the guard also checks the lookup shape. Consolidation reloads by record.topic now regardless, so the forced probe reaches every topic through exact identity. The regression drives the consolidate tool end to end with a Travel Preferences topic and asserts it comes back consolidated; on the previous head the probe skips it and the test fails.

2. Digest collisions can no longer silently overwrite. The stem digest widened to 64 bits, so your pair separates at sixteen hex characters, and write_topic now reads the occupant of an existing stem before reusing it, extending the digest when a different topic already lives there. One regression writes your exact pair and asserts both files survive with isolated reads, and a second seeds one topic's record at another topic's primary stem to prove the write side extends without depending on hash luck. Records also carry the stem they were read from or written to, so index pointer lines keep naming files that exist after an extension.

The four new tests fail on the previous head and pass now. The memory suite passes at 44 tests, the neighboring harness suites pass at 366, and ruff check and format are clean.

One correction to the verification note above: ruff and the suites were clean, but the same push tripped Package Checks, which runs pyright over the package and rejected the store assigning the record's private stem field from outside its class. That is fixed in f1fd7fa with the stamp routed through a setter, and pyright now reports the file clean, so the earlier "clean" claim should be read as ruff only, not the full check suite.

[Manohar Paturi (ManoharPaturi)]

The Package Checks failure was pyright flagging the store stamping the record's private on disk stem field from outside its class, three sites. Fixed in f1fd7fa: the stamp now goes through a small setter so the assignment happens inside the record. Verified with pyright against the core package config, where the file now reports clean, and the memory suite still passes at 44 tests. The merge gatekeeper was only mirroring that failure.

[Eduard van Valkenburg (eavanvalkenburg)]

Candidate stems are probed without first bounding their encoded filename length. For a sufficiently long readable slug, the longest candidate plus .md exceeds the filesystem component limit, so Path.exists() raises ENAMETOOLONG before lookup can reach a valid persisted or legacy filename. This can make both existing long topics and newly written topics unreadable, unwritable, or undeletable. Please bound generated candidates before touching the filesystem and resolve valid persisted/legacy stems without first probing oversized names. Add regressions for a long legacy-topic lookup and index-stem reload.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota. Try requesting a review at a lower effort level.

[Manohar Paturi (ManoharPaturi)]

Fixed in 0a7bbf5, with the failure reproduced on the previous head first.

The write path is where it bites hardest: on the previous head, writing a topic whose slug makes the primary candidate plus .md overshoot the component limit raises ENAMETOOLONG (errno 63 here) before any valid name is reached, so the topic is unwritable. Generated candidates now cap the readable slug part so the longest candidate, digest and extension included, stays within 255 bytes, and every filesystem probe skips oversized names instead of raising, so a valid persisted or legacy stem is still reached.

Regressions cover both sides: a legacy slug only file at 240 characters stays readable, and a topic with a ~470 character slug writes, reloads by topic and by its index stem, with the persisted stem asserted within the limit and every index link resolving. The write regression fails on the previous head with ENAMETOOLONG and passes now; the memory suite is at 46 tests, and pyright reports the file clean.

[Eduard van Valkenburg (eavanvalkenburg)]

/review

[github-actions]

MAF Automated Review — Iteration 1

Result: Findings reported
Scope: full PR (4 commit(s)): 8d4eb109881a, cef2b8e95aac, f1fd7fa1b0f1, 0a7bbf53decc
Model: gpt-5.6-sol

Overview

The PR replaces non-injective slug-only filenames with bounded digest stems, validates digest occupants, preserves index links to persisted stems, and adds broad collision and compatibility coverage. Those guards address fresh same-slug collisions and overlong filenames, but the unchecked exact-file fallback can still delete an unrelated legacy topic during migration. Consolidation also regresses for the newly persisted explicit-slug records because it reloads them through the default topic-derived filename family.

Reviewed the supplied pull-request change set across correctness, security/reliability, architecture, and failure behavior.
2 verified findings remained after source verification (1 high, 1 medium) across 1 file. Details are attached to the affected lines below.

Affected areas: python/packages/core/agent_framework/_harness/_memory.py

[github-actions]

This exact-file fallback does not verify which topic the file contains. On an upgraded store where legacy c.md holds C++, writing C# resolves that unrelated file as previous_path and unlinks it after creating the new C# file; deletion has the same cross-topic effect. Please preserve persisted-stem reads, but require destructive write/delete resolution to verify that a slug-only occupant has the requested normalized topic before it can be retired or removed.

[github-actions]

An explicit-slug record such as topic Machine Learning with slug ml-notes is stored under ml-notes-<digest>.md, but reloading by record.topic searches the machine-learning-* family. Consolidation therefore catches FileNotFoundError and silently skips every such record, whereas the previous slug-based reload found it. Please reload through the known persisted stem (or otherwise resolve that exact file) while retaining identity-safe behavior for colliding default slugs.

[Eduard van Valkenburg (eavanvalkenburg)]

Manohar Paturi (@ManoharPaturi) The new filename scheme still has two topic-identity gaps. First, write_topic uses an alias-capable lookup for the previous record without verifying the record’s actual topic, so writing C++ followed by c—or API followed by api—can delete or merge the other topic despite distinct digest filenames. Second, consolidation reloads by record.topic, but lookup searches only the filename family derived from that topic, so a record stored under a custom slug can be skipped and later duplicated. Please separate exact-topic resolution from explicit slug aliases, validate topic ownership before merge/retirement, and use one canonical identity for internal reload and cleanup. Add lifecycle and consolidation regressions for both cases.