diff --git a/python/packages/core/AGENTS.md b/python/packages/core/AGENTS.md index 3acfc513d5..f47e4074fd 100644 --- a/python/packages/core/AGENTS.md +++ b/python/packages/core/AGENTS.md @@ -290,7 +290,7 @@ The vector store API is experimental under the shared `VECTOR_STORES` feature ID - **`FileMemoryProvider`** - `ContextProvider` that gives an agent a session-scoped, file-based memory backed by the same `AgentFileStore` abstraction. Adds tools (`file_memory_write`, `file_memory_read`, `file_memory_delete`, `file_memory_ls`, `file_memory_grep`, `file_memory_replace`, `file_memory_replace_lines`) plus default usage instructions. Port of the .NET `FileMemoryProvider`. - **Scoping** - Memories are isolated per session by default: each session writes under a working folder derived from `context.session_id`. Pass an explicit `scope` (e.g. a user id) to group memories across sessions, mirroring `FoundryMemoryProvider`'s `scope` arg. The scope (or session id) is an **opaque namespace key, not a path**: it is mapped to exactly one folder via the shared `agent_framework._filesystem._storage_key_segment` derivation, which is **injective** (except for a collision-resistant digest fallback past a length cap) — two byte-distinct values do not share a working folder, so a caller authorized for one identifier cannot reach another's memories. A multi-segment value such as `"tenants/alice"` therefore becomes a single encoded folder rather than a nested directory. The provider **fails closed** (`ValueError`) when neither a `scope` nor a `session_id` is available, because an empty working folder would be the shared store root. Applications should still canonicalize and authorize externally supplied identifiers themselves; the storage mapping is the storage-layer guarantee, not a substitute for that check. -- **Descriptions & index** - `file_memory_write` accepts an optional `description`, stored in a companion `_description.md` sidecar. After each write/delete the provider rebuilds a capped (50-entry) `memories.md` index, and `before_run` injects that index as a `user` context message so the model knows what memories exist. Sidecars and the index are internal files hidden from `file_memory_ls`/`file_memory_grep` and rejected as write targets. +- **Descriptions & index** - `file_memory_write` accepts an optional `description`, stored in a companion sidecar named `_description.md` (the whole memory file name is kept, so `notes.md` and `notes` get distinct sidecars). After each write/delete the provider rebuilds a capped (50-entry) `memories.md` index, and `before_run` injects that index as a `user` context message so the model knows what memories exist. Sidecars and the index are internal files hidden from `file_memory_ls`/`file_memory_grep` and rejected as write targets. Sidecars written by earlier versions used the extension-less stem (`notes_description.md`) and are not migrated. - **`DEFAULT_FILE_MEMORY_SOURCE_ID`** / **`DEFAULT_FILE_MEMORY_INSTRUCTIONS`** - Public defaults for the provider's source id and instruction banner. - **Harness wiring** - `create_harness_agent` includes the `FileMemoryProvider` by default; the `FileAccessProvider` is opt-in and added only when a `file_access_store` is supplied (no implicit `{cwd}/working` store is created). Disable file memory via `disable_file_memory`; override its backing store via `file_memory_store`. When no file-memory store is supplied, the default is `FileSystemAgentFileStore` rooted at `{cwd}/agent-file-memory`. `create_harness_agent` also wires in `MessageInjectionMiddleware` by default (mirroring the .NET harness's `UseMessageInjection`); it is always on with no opt-out because it is a no-op when no messages are queued for the session. - **Experimental-feature gating** - `create_harness_agent` itself is released (no longer `@experimental`), but a few features it can wire in remain experimental or pre-release: **background agents** (`background_agents`), **file access** (`file_access_store`), **looping** (`loop_should_continue`), and the **shell tooling** (`shell_executor`, from the pre-release `agent-framework-tools` package). Enabling any of them emits a single `ExperimentalWarning` (naming the responsible parameter). The three experimental harness providers share one `HARNESS` dedup key so the downstream experimental provider does not warn a second time; the shell tooling uses a separate dedup key (it is not a `HARNESS`-decorated feature). diff --git a/python/packages/core/agent_framework/_harness/_file_memory.py b/python/packages/core/agent_framework/_harness/_file_memory.py index b64a112376..28c83a3517 100644 --- a/python/packages/core/agent_framework/_harness/_file_memory.py +++ b/python/packages/core/agent_framework/_harness/_file_memory.py @@ -4,8 +4,9 @@ :class:`FileMemoryProvider` gives an agent a session-scoped, file-based memory system. Each memory is stored as an individual file with a meaningful name, and -large files can carry a companion description file (suffixed with -``_description.md``) that provides a short summary used for discovery. A +large files can carry a companion description file (the memory file name with +``_description.md`` appended, so the sidecar of ``notes.md`` is +``notes.md_description.md``) that provides a short summary used for discovery. A ``memories.md`` index file is maintained automatically and injected into the agent's context so the model knows what memories already exist. @@ -42,6 +43,7 @@ from __future__ import annotations +import hashlib import logging from typing import Annotated, Any @@ -86,22 +88,48 @@ _DESCRIPTION_SUFFIX = "_description.md" _MEMORY_INDEX_FILE_NAME = "memories.md" _MAX_INDEX_ENTRIES = 50 +# Filesystem name limits are commonly 255 bytes; the sidecar derivation must stay +# within that budget so every valid memory file name also gets a writable sidecar. +_MAX_DESCRIPTION_FILE_NAME_BYTES = 255 # Prefix for scopes that are not usable verbatim as a folder name. Kept distinct # from the other harness prefixes so the same value under a different component # never lands in the same directory. _ENCODED_SCOPE_PREFIX = "~scope-" +# Prefix for description sidecars of memory file names too long to carry the +# suffix verbatim. Distinct from the other prefixes for the same reason. +# Memory file names that already start with this prefix also take the digest +# branch, so the verbatim namespace never contains the prefix and the two +# branches cannot produce the same sidecar. +_DIGEST_DESCRIPTION_PREFIX = "~memory-desc-" def _description_file_name(file_name: str) -> str: """Return the companion description file name for ``file_name``. - The suffix replaces the original extension when present (so ``notes.md`` - becomes ``notes_description.md``); otherwise it is appended. + The suffix is appended to the *whole* memory file name, so ``notes.md`` + becomes ``notes.md_description.md`` while ``notes`` becomes + ``notes_description.md``. Keeping the extension in the derivation makes + the mapping injective: distinct memory files always get distinct sidecars, + so writes and deletes can never clobber another memory's description. + + A memory file name long enough to push the appended sidecar past the + filesystem's name limit cannot carry the suffix verbatim, so such names + fall back to a SHA-256 digest (still suffixed, still injective in + practice). Names that already start with the reserved digest prefix take + the digest branch too: otherwise a short memory named after another name's + digest stem would share that name's sidecar without any hash collision. + The prefix is matched case-insensitively because the default APFS and NTFS + filesystems are case-insensitive, so an uppercase or mixed-case reserved + prefix would otherwise alias a long name's digest sidecar on disk. """ - dot_index = file_name.rfind(".") - if dot_index > 0: - return f"{file_name[:dot_index]}{_DESCRIPTION_SUFFIX}" - return f"{file_name}{_DESCRIPTION_SUFFIX}" + sidecar = f"{file_name}{_DESCRIPTION_SUFFIX}" + if ( + not file_name.lower().startswith(_DIGEST_DESCRIPTION_PREFIX) + and len(sidecar.encode("utf-8")) <= _MAX_DESCRIPTION_FILE_NAME_BYTES + ): + return sidecar + digest = hashlib.sha256(file_name.encode("utf-8")).hexdigest() + return f"{_DIGEST_DESCRIPTION_PREFIX}{digest}{_DESCRIPTION_SUFFIX}" def _is_internal_file(file_name: str) -> bool: diff --git a/python/packages/core/tests/core/test_harness_file_memory.py b/python/packages/core/tests/core/test_harness_file_memory.py index 5d8f7394be..5bb2d7bda5 100644 --- a/python/packages/core/tests/core/test_harness_file_memory.py +++ b/python/packages/core/tests/core/test_harness_file_memory.py @@ -14,6 +14,7 @@ Content, FileAccessProvider, FileMemoryProvider, + FileSystemAgentFileStore, FunctionTool, InMemoryAgentFileStore, ) @@ -55,15 +56,54 @@ async def _prepare( return context, tools -def test_description_file_name_replaces_extension() -> None: - """The description sidecar replaces a known extension and appends otherwise.""" - assert _description_file_name("notes.md") == "notes_description.md" - assert _description_file_name("data.json") == "data_description.md" +def test_description_file_name_appends_to_full_name() -> None: + """The description sidecar appends to the whole file name, so it is injective.""" + assert _description_file_name("notes.md") == "notes.md_description.md" + assert _description_file_name("notes") == "notes_description.md" + # Names that share a stem must not share a sidecar. + assert _description_file_name("notes.md") != _description_file_name("notes") + assert _description_file_name("data.json") == "data.json_description.md" assert _description_file_name("noext") == "noext_description.md" - # Leading-dot files have no stem, so the suffix is appended. assert _description_file_name(".hidden") == ".hidden_description.md" +def test_description_file_name_stays_within_filesystem_name_limits() -> None: + """Sidecars of very long memory file names must stay writable and distinct.""" + # "a"*235 + ".md" carries the suffix verbatim at exactly 254 bytes. + within_limit = "a" * 235 + ".md" + # "a"*238 + ".md" would push the appended sidecar to 256 bytes. + over_limit = "a" * 238 + ".md" + assert len(_description_file_name(within_limit).encode("utf-8")) <= 255 + assert _description_file_name(within_limit) == within_limit + "_description.md" + + bounded = _description_file_name(over_limit) + assert len(bounded.encode("utf-8")) <= 255 + # The digest sidecar keeps the suffix, so listings still hide it. + assert _is_internal_file(bounded) + + # Distinct long names must not share a digest sidecar. + other = "b" * 238 + ".md" + assert _description_file_name(over_limit) != _description_file_name(other) + # The derivation is stable, so reads and deletes resolve to the same sidecar. + assert _description_file_name(over_limit) == _description_file_name(over_limit) + # A digest sidecar never equals a verbatim sidecar of a shorter name. + assert _description_file_name(over_limit) != _description_file_name("~memory-desc-x") + # Reserved-prefix names take the digest branch even when short, so the + # verbatim namespace can never contain the digest prefix and the two + # branches are disjoint without relying on a SHA-256 collision. + import hashlib + + digest_stem = "~memory-desc-" + hashlib.sha256(over_limit.encode()).hexdigest() + assert digest_stem.startswith("~memory-desc-") + assert _description_file_name(digest_stem) != digest_stem + "_description.md" + assert _description_file_name(digest_stem) != _description_file_name(over_limit) + assert _description_file_name("~memory-desc-x") != "~memory-desc-x_description.md" + # The reserved prefix is matched case-insensitively so case variants also + # take the digest branch and cannot alias a long name's digest sidecar. + for variant in ("~MEMORY-DESC-x", "~Memory-Desc-x"): + assert _description_file_name(variant) != variant + "_description.md" + + def test_is_internal_file_detects_sidecars_and_index() -> None: """Internal files are description sidecars and the memory index, case-insensitively.""" assert _is_internal_file("notes_description.md") @@ -152,7 +192,7 @@ async def test_description_sidecar_is_written_and_listed() -> None: ) assert "with description" in _text(result) - sidecar = await store.read(_combine_paths("user-1", "arch_description.md")) + sidecar = await store.read(_combine_paths("user-1", "arch.md_description.md")) assert sidecar == "system architecture" listed = json.loads(_text(await list_files.invoke())) @@ -160,11 +200,144 @@ async def test_description_sidecar_is_written_and_listed() -> None: # Re-saving without a description removes the sidecar. await save.invoke(arguments={"file_name": "arch.md", "content": "big content"}) - assert await store.read(_combine_paths("user-1", "arch_description.md")) is None + assert await store.read(_combine_paths("user-1", "arch.md_description.md")) is None listed_again = json.loads(_text(await list_files.invoke())) assert listed_again == [{"name": "arch.md", "type": "file", "description": None}] +async def test_long_name_and_matching_digest_stem_short_name_have_independent_lifecycles() -> None: + """A long name and a valid short name equal to its digest stem must not share a sidecar. + + The short name begins with the reserved digest prefix, so it also takes the + digest branch; writing or deleting either memory must leave the other + memory's body and description untouched. + """ + import hashlib + + store = InMemoryAgentFileStore() + provider = FileMemoryProvider(store=store, scope="user-1") + _, tools = await _prepare(provider) + save = tools["file_memory_write"] + read = tools["file_memory_read"] + delete = tools["file_memory_delete"] + list_files = tools["file_memory_ls"] + + long_name = "a" * 238 + ".md" + short_name = "~memory-desc-" + hashlib.sha256(long_name.encode()).hexdigest() + # Both names are accepted by the provider's validation and normalization. + assert _description_file_name(long_name) != _description_file_name(short_name) + + await save.invoke(arguments={"file_name": long_name, "content": "long-body", "description": "long-description"}) + await save.invoke(arguments={"file_name": short_name, "content": "short-body", "description": "short-description"}) + + # Writing the short memory must not overwrite the long memory's description. + listed = {entry["name"]: entry["description"] for entry in json.loads(_text(await list_files.invoke()))} + assert listed[long_name] == "long-description" + assert listed[short_name] == "short-description" + assert _text(await read.invoke(arguments={"file_name": long_name})) == "long-body" + assert _text(await read.invoke(arguments={"file_name": short_name})) == "short-body" + + # Deleting the short memory must not remove the long memory's description. + await delete.invoke(arguments={"file_name": short_name}) + listed_after = {entry["name"]: entry["description"] for entry in json.loads(_text(await list_files.invoke()))} + assert listed_after == {long_name: "long-description"} + assert _text(await read.invoke(arguments={"file_name": long_name})) == "long-body" + + # Rewriting the long memory still updates only its own sidecar. + await save.invoke(arguments={"file_name": long_name, "content": "long-body-2", "description": "long-description-2"}) + listed_rewritten = {entry["name"]: entry["description"] for entry in json.loads(_text(await list_files.invoke()))} + assert listed_rewritten == {long_name: "long-description-2"} + + +@pytest.mark.parametrize("prefix_variant", ["~MEMORY-DESC-", "~Memory-Desc-"]) +async def test_case_variant_reserved_prefix_short_name_does_not_alias_long_name(prefix_variant: str) -> None: + """Upper/mixed-case reserved prefixes must not alias a long name's digest sidecar. + + On case-insensitive filesystems (default APFS/NTFS) a short memory whose + name uses an uppercase or mixed-case reserved prefix would otherwise + resolve to the same on-disk sidecar as a long name with the matching + digest, so writing or deleting it could clobber the long memory's + description without any SHA-256 collision. + """ + import hashlib + + store = InMemoryAgentFileStore() + provider = FileMemoryProvider(store=store, scope="user-1") + _, tools = await _prepare(provider) + save = tools["file_memory_write"] + read = tools["file_memory_read"] + delete = tools["file_memory_delete"] + list_files = tools["file_memory_ls"] + + long_name = "a" * 238 + ".md" + short_name = prefix_variant + hashlib.sha256(long_name.encode()).hexdigest() + assert short_name.startswith(prefix_variant) + # The case-variant reserved name must not share the long name's sidecar. + assert _description_file_name(short_name).lower() != _description_file_name(long_name).lower() + + await save.invoke(arguments={"file_name": long_name, "content": "long-body", "description": "long-description"}) + await save.invoke(arguments={"file_name": short_name, "content": "short-body", "description": "short-description"}) + + # Writing the case-variant memory must not overwrite the long memory's description. + listed = {entry["name"]: entry["description"] for entry in json.loads(_text(await list_files.invoke()))} + assert listed[long_name] == "long-description" + assert listed[short_name] == "short-description" + assert _text(await read.invoke(arguments={"file_name": long_name})) == "long-body" + assert _text(await read.invoke(arguments={"file_name": short_name})) == "short-body" + + # Deleting the case-variant memory must not remove the long memory's description. + await delete.invoke(arguments={"file_name": short_name}) + listed_after = {entry["name"]: entry["description"] for entry in json.loads(_text(await list_files.invoke()))} + assert listed_after == {long_name: "long-description"} + assert _text(await read.invoke(arguments={"file_name": long_name})) == "long-body" + + +async def test_long_name_sidecar_is_writable_on_disk(tmp_path, monkeypatch) -> None: + """A verbatim sidecar over the name limit must fall back to a writable digest on disk. + + Uses the real file system rather than the in-memory store: the long memory + body is written before sidecar access, so an over-limit sidecar would leave + partial state and break later index rebuilds. Windows path limits reject + the longest names, so there the test uses a shorter name and a lower + sidecar budget; Linux CI uses the real 255-byte limit. + """ + import hashlib + import sys + + import agent_framework._harness._file_memory as file_memory + + long_name = "a" * 238 + ".md" + if sys.platform == "win32": + # Windows rejects long names once the full temporary path grows past + # its path limit, so exercise the same derivation with a shorter name + # and a proportionally lower sidecar budget. Linux CI covers the real + # 255-byte boundary verbatim. + long_name = "a" * 120 + ".md" + monkeypatch.setattr(file_memory, "_MAX_DESCRIPTION_FILE_NAME_BYTES", 130) + + store = FileSystemAgentFileStore(tmp_path) + provider = FileMemoryProvider(store=store, scope="user-1") + _, tools = await _prepare(provider) + save = tools["file_memory_write"] + list_files = tools["file_memory_ls"] + + short_name = "~memory-desc-" + hashlib.sha256(long_name.encode()).hexdigest() + for name, body, description in ( + (long_name, "long-body", "long-description"), + (short_name, "short-body", "short-description"), + ): + result = await save.invoke(arguments={"file_name": name, "content": body, "description": description}) + assert "with description" in _text(result) + assert (tmp_path / "user-1" / _description_file_name(name)).read_text(encoding="utf-8") == description + assert len(_description_file_name(name).encode("utf-8")) <= file_memory._MAX_DESCRIPTION_FILE_NAME_BYTES + + listed = {entry["name"]: entry["description"] for entry in json.loads(_text(await list_files.invoke()))} + assert listed == {long_name: "long-description", short_name: "short-description"} + await tools["file_memory_delete"].invoke(arguments={"file_name": short_name}) + listed_after = {entry["name"]: entry["description"] for entry in json.loads(_text(await list_files.invoke()))} + assert listed_after == {long_name: "long-description"} + + async def test_delete_removes_sidecar() -> None: """Deleting a file also removes its companion description sidecar.""" store = InMemoryAgentFileStore() @@ -172,10 +345,91 @@ async def test_delete_removes_sidecar() -> None: _, tools = await _prepare(provider) await tools["file_memory_write"].invoke(arguments={"file_name": "arch.md", "content": "x", "description": "desc"}) - assert await store.read(_combine_paths("user-1", "arch_description.md")) == "desc" + assert await store.read(_combine_paths("user-1", "arch.md_description.md")) == "desc" await tools["file_memory_delete"].invoke(arguments={"file_name": "arch.md"}) - assert await store.read(_combine_paths("user-1", "arch_description.md")) is None + assert await store.read(_combine_paths("user-1", "arch.md_description.md")) is None + + +async def test_sidecars_of_names_sharing_a_stem_do_not_collide() -> None: + """``notes.md`` and ``notes`` must not share one description sidecar.""" + store = InMemoryAgentFileStore() + provider = FileMemoryProvider(store=store, scope="user-1") + _, tools = await _prepare(provider) + save = tools["file_memory_write"] + + await save.invoke(arguments={"file_name": "notes.md", "content": "md body", "description": "markdown notes"}) + await save.invoke(arguments={"file_name": "notes", "content": "plain body", "description": "plain notes"}) + + # Each memory file keeps its own sidecar, and the second write did not + # overwrite the first one's description. + assert await store.read(_combine_paths("user-1", "notes.md_description.md")) == "markdown notes" + assert await store.read(_combine_paths("user-1", "notes_description.md")) == "plain notes" + assert sorted(entry.name for entry in await store.list_children("user-1")) == [ + "memories.md", + "notes", + "notes.md", + "notes.md_description.md", + "notes_description.md", + ] + + listed = json.loads(_text(await tools["file_memory_ls"].invoke())) + assert sorted(listed, key=lambda entry: entry["name"]) == [ + {"name": "notes", "type": "file", "description": "plain notes"}, + {"name": "notes.md", "type": "file", "description": "markdown notes"}, + ] + + index = await store.read(_combine_paths("user-1", _MEMORY_INDEX_FILE_NAME)) + assert index is not None + assert "- **notes.md**: markdown notes" in index + assert "- **notes**: plain notes" in index + + +async def test_rewriting_one_file_without_description_keeps_sibling_sidecar() -> None: + """Dropping a description on one file must not delete another file's sidecar.""" + store = InMemoryAgentFileStore() + provider = FileMemoryProvider(store=store, scope="user-1") + _, tools = await _prepare(provider) + save = tools["file_memory_write"] + + await save.invoke(arguments={"file_name": "notes.md", "content": "md body", "description": "markdown notes"}) + await save.invoke(arguments={"file_name": "notes", "content": "plain body", "description": "plain notes"}) + + # Rewrite ``notes.md`` without a description: only its own sidecar goes away. + await save.invoke(arguments={"file_name": "notes.md", "content": "rewritten"}) + assert await store.read(_combine_paths("user-1", "notes.md_description.md")) is None + assert await store.read(_combine_paths("user-1", "notes_description.md")) == "plain notes" + + listed = json.loads(_text(await tools["file_memory_ls"].invoke())) + assert sorted(listed, key=lambda entry: entry["name"]) == [ + {"name": "notes", "type": "file", "description": "plain notes"}, + {"name": "notes.md", "type": "file", "description": None}, + ] + + +async def test_deleting_one_file_keeps_sibling_sidecar() -> None: + """Deleting one memory must not delete another memory's description.""" + store = InMemoryAgentFileStore() + provider = FileMemoryProvider(store=store, scope="user-1") + _, tools = await _prepare(provider) + save = tools["file_memory_write"] + delete = tools["file_memory_delete"] + + await save.invoke(arguments={"file_name": "notes.md", "content": "md body", "description": "markdown notes"}) + await save.invoke(arguments={"file_name": "notes", "content": "plain body", "description": "plain notes"}) + + await delete.invoke(arguments={"file_name": "notes"}) + assert await store.read(_combine_paths("user-1", "notes_description.md")) is None + assert await store.read(_combine_paths("user-1", "notes.md_description.md")) == "markdown notes" + + listed = json.loads(_text(await tools["file_memory_ls"].invoke())) + assert listed == [{"name": "notes.md", "type": "file", "description": "markdown notes"}] + + # The sidecars stay internal: they are neither listed nor valid write targets. + assert all(not _is_internal_file(entry["name"]) for entry in listed) + for reserved in ("notes.md_description.md", "notes_description.md"): + rejected = await save.invoke(arguments={"file_name": reserved, "content": "x"}) + assert "reserved" in _text(rejected) async def test_index_is_rebuilt_and_injected_on_next_run() -> None: