diff --git a/python/packages/core/AGENTS.md b/python/packages/core/AGENTS.md index 447f3bbafc4..6363ff3ba00 100644 --- a/python/packages/core/AGENTS.md +++ b/python/packages/core/AGENTS.md @@ -16,6 +16,7 @@ agent_framework/ ├── _vectors.py # Vector store models, CRUD/search abstractions, and protocols ├── _middleware.py # Middleware system for request/response interception ├── _sessions.py # AgentSession and context provider abstractions +├── _filesystem.py # Private filesystem safety helpers (link detection, storage-key derivation) ├── _skills.py # Agent Skills system (models, executors, provider) ├── _mcp.py # Model Context Protocol support ├── _telemetry.py # User-Agent identity and internal feature-usage mask @@ -132,6 +133,7 @@ The vector store API is experimental under the shared `VECTOR_STORES` feature ID - **`AgentSession`** - Manages conversation state and session metadata - **`SessionStore`** - Experimental in-memory opaque `session_id -> AgentSession` snapshot store; reads return independent copies - **`FileSessionStore`** - Experimental msgspec file-backed session snapshot store with atomic last-writer-wins updates; JSON is the default, `serialization_format="msgpack"` enables binary MessagePack, opaque keys are encoded to portable filenames, and only syntactically malformed snapshots are quarantined (schema, version, and state-decoder failures preserve the original file) +- **Storage-key derivation (`_filesystem._storage_key_segment`)** - Every component that maps a caller-controlled identifier (session id, owner id, memory scope) onto a storage location uses this one private helper: `FileSessionStore` / `FileHistoryProvider` (`~session-`), `TodoFileStore` (`~todo-`), `MemoryFileStore` (`~memory-`), and `FileMemoryProvider` (`~scope-`). Its contract is **injectivity** — two byte-distinct identifiers must not produce the same segment, because the segment is an isolation boundary. Identifiers that are already a safe single segment (**lowercase** ASCII alnum plus `._-`, no leading `.`, no trailing `.`/space, not a Windows reserved stem) are used verbatim so on-disk layouts stay readable; everything else is encoded as **lowercase base32** under the component prefix, with a `sha256-` digest segment past a length cap (`_MAX_ENCODED_STORAGE_KEY_SEGMENT_LENGTH`). Only that digest branch weakens the contract to collision resistance. The charset is deliberately narrow so the filesystem cannot fold two distinct segments together: **non-ASCII** values are encoded because macOS APFS/HFS+ fold NFC vs NFD, and **uppercase** values are encoded (never lowercased, which would collide with a genuine lowercase identifier) because NTFS and APFS are case-insensitive by default — base32's `a-z2-7` alphabet is itself case-stable. **Never pass such an identifier through a path normalizer** (e.g. `_normalize_relative_path`) to derive storage: that mapping is lossy, and `id` / `id/` / `id\` would collide. New components must reuse this helper with their own `~`-prefixed namespace. - **`register_state_type`** - Registers custom `AgentSession.state` classes with stable, process-wide type IDs and optional mapping codecs. Provider modules own registration for their custom state types and should use package-qualified IDs. Implicit Pydantic registration remains temporarily with `DeprecationWarning`, but module-level registration is needed to guarantee cold-start restoration. - **`ServiceSessionId`** - Mapping alias for structured service-owned continuation handles used in `AgentSession.service_session_id` - **`SessionContext`** - Context object for session-scoped data during agent runs. `extend_messages(...)` can attach @@ -150,7 +152,7 @@ The vector store API is experimental under the shared `VECTOR_STORES` feature ID - **`SkillScriptRunner`** - Protocol for file-based script execution. Any callable matching `(skill, script, args) -> Any` satisfies it. Code-defined scripts do not use a runner. - **`SkillScriptArgumentParser`** - Public type alias for an optional callable `(raw args: dict | list[str] | str | None) -> dict | None` that converts the raw `args` value before an `InlineSkillScript` runs (applied before the inline list-args guard). It is an opt-in customization hook (port of .NET PR #6498) that lets callers support backends sending tool-call arguments in a non-conforming shape (e.g. vLLM JSON strings). The output is constrained to a `dict` (named keyword arguments) or `None`, because inline scripts bind arguments by keyword name. Supply it via the `argument_parser=` constructor arg on `InlineSkillScript`, `InlineSkill` (default for scripts added via `@skill.script`), or `ClassSkill` (default for scripts discovered via `@ClassSkill.script`). When `None` (the default), the raw value is used unchanged. File-based scripts are unaffected (their runner owns arg handling). - **`SkillsProvider`** - Context provider (extends `ContextProvider`) that discovers file-based skills from `SKILL.md` files and/or accepts code-defined `Skill` instances. Follows progressive disclosure: advertise → load → read resources / run scripts. By default all three tools it exposes (`load_skill`, `read_skill_resource`, `run_skill_script`) are registered with `approval_mode="always_require"`, so every skill operation needs approval. To run unattended, pass one of the static auto-approval rules to `ToolApprovalMiddleware` (via `auto_approval_rules`): `SkillsProvider.read_only_tools_auto_approval_rule` approves only the read-only tools (`load_skill`, `read_skill_resource`) while still prompting for `run_skill_script`, and `SkillsProvider.all_tools_auto_approval_rule` approves every skill tool including script execution. Both rules reject any call carrying a `server_label` so they stay scoped to this provider's local tools and never auto-approve a same-named hosted tool. Alternatively, for trusted skills, the constructor / `from_paths` kwargs `disable_load_skill_approval`, `disable_read_skill_resource_approval`, and `disable_run_skill_script_approval` (all default `False`) opt individual tools out of approval entirely by registering them with `approval_mode="never_require"` (the auto-approval rules only apply to tools that still require approval). The tool names are also exposed as class constants (`LOAD_SKILL_TOOL_NAME`, `READ_SKILL_RESOURCE_TOOL_NAME`, `RUN_SKILL_SCRIPT_TOOL_NAME`). -- **`FileSkillsSource`** - `SkillsSource` that discovers file-based skills by scanning configured root paths for `SKILL.md`. The **configured root paths define the trust boundary** and are used as given (a root may itself be a symlink); everything discovered *below* a root is link-checked and fails closed. `_discover_skill_directories` rejects any entry that is a symbolic link, junction, or other reparse point (via the shared `agent_framework._filesystem.is_link_or_reparse_point` helper) before descending into it, and rejects a directory whose `SKILL.md` is itself such a link — otherwise a link planted under a root would be adopted as the skill root, and since every later guard treats the skill root as the boundary and only inspects segments below it, the link itself would never be inspected. Resource and script discovery apply the same rule per path segment via `_has_link_or_reparse_point_in_path`. An `OSError` while inspecting an entry is treated as unsafe (skip / reject), never as "safe". +- **`FileSkillsSource`** - `SkillsSource` that discovers file-based skills by scanning configured root paths for `SKILL.md`. The **configured root paths define the trust boundary** and are used as given (a root may itself be a symlink); everything discovered *below* a root is link-checked and fails closed. `_discover_skill_directories` rejects any entry that is a symbolic link, junction, or other reparse point (via the shared `agent_framework._filesystem._is_link_or_reparse_point` helper) before descending into it, and rejects a directory whose `SKILL.md` is itself such a link — otherwise a link planted under a root would be adopted as the skill root, and since every later guard treats the skill root as the boundary and only inspects segments below it, the link itself would never be inspected. Resource and script discovery apply the same rule per path segment via `_has_link_or_reparse_point_in_path`. An `OSError` while inspecting an entry is treated as unsafe (skip / reject), never as "safe". - **`MCPSkillsSource`** - `SkillsSource` that discovers Agent Skills served over MCP by reading the well-known `skill://index.json` (SEP-2640). Index entries are dispatched by their `type` (case-insensitive): `skill-md` entries become one `MCPSkill` each (its `SKILL.md` body and sibling resources are fetched on demand via `resources/read`), and `archive` entries are downloaded as a single ZIP / TAR / gzip-TAR blob and unpacked **entirely in memory** (via the private `_ArchiveEntryLoader`) into a `FileSkill` whose `SKILL.md` body drives it and whose sibling files (matching the resource extensions, within the search depth) become in-memory `InlineSkillResource` resources. **Nothing is written to disk** — there are no temporary directories to create, own, or prune (this is a deliberate divergence from .NET, which extracts archives to disk; it removes the temp-dir leak and the dangerous prune-of-unowned-subdirs footgun). Entries whose type has no handler (e.g. `mcp-resource-template`) are skipped. **MCP-delivered scripts are never runnable**: the loader emits no `SkillScript`s, so a bundled script can at most surface as a readable resource (and only if it matches the resource extensions — `.py` is not a default resource extension). The archive `SKILL.md` frontmatter `name` must match the advertised index-entry `name` or the skill is skipped. Extraction is hardened: a `..` path-traversal ("zip-slip") member name raises via `_normalize_archive_member_name` and aborts the whole skill (like the file-count/size limits), non-regular TAR members (links/devices) are skipped, and file-count / uncompressed-size (`_read_member_with_limit`) / download-size limits are enforced. Archive behavior is configured with `archive_*` constructor kwargs (`archive_resource_extensions`, `archive_resource_search_depth`, `archive_max_file_count`, `archive_max_size_bytes`, `archive_max_uncompressed_size_bytes`) — Python uses plain kwargs, not a `*Options` object as in .NET. A non-"resource not found" error while downloading an archive propagates (so a failed `CachingSkillsSource` refresh does not overwrite a cached list with a partial result). Unlike .NET's `AgentMcpSkillsSourceOptions.RefreshInterval`, this source has no built-in refresh interval; wrap it in `CachingSkillsSource(..., refresh_interval=...)` for caching/refresh. This is a port of .NET PR #6631; the `FileSkillsSource` `script_extensions`/`resource_extensions` kwargs default to the built-in tuples and treat `None` as "use defaults" and an empty tuple as "discover none" (an empty tuple previously fell back to defaults). `FoundryToolbox.as_skills_provider()` forwards matching `archive_*` kwargs to this source. - **`SkillsSource` decorators** - Skill sources are composable: `SkillsSource` is the abstract base, with concrete sources (`InMemorySkillsSource`, `FileSkillsSource`, `MCPSkillsSource`) and decorators that wrap an inner source — `AggregatingSkillsSource` (concatenate several sources), `FilteringSkillsSource` (predicate filter), `DeduplicatingSkillsSource` (first-wins by name), and `CachingSkillsSource` (cache the inner source's skills list). `DelegatingSkillsSource` is the abstract base for decorators. **`get_skills` takes a `SkillsSourceContext`**: every source/decorator implements `async def get_skills(self, context: SkillsSourceContext) -> list[Skill]` and forwards `context` to inner sources. `SkillsSourceContext` (frozen) carries the invoking `agent` (`SupportsAgentRun`) and optional `session` (`AgentSession | None`); `SkillsProvider` builds it from `before_run`'s `agent`/`session` and passes it into the pipeline. `FilteringSkillsSource`'s predicate is context-aware: `Callable[[Skill, SkillsSourceContext], bool]` (port of .NET #6797). **Default caching is applied only to the built-in, context-independent leaf sources**: for the `Skill` / sequence-of-skills / `from_paths` constructors, `SkillsProvider` builds `DeduplicatingSkillsSource(CachingSkillsSource())` so expensive filesystem/network discovery runs once. A **caller-supplied `SkillsSource` is used as-is — never auto-wrapped in caching or deduplication** — because auto-caching a context-aware caller source in a single shared bucket would replay the first invocation's skills for later `SkillsSourceContext`s and leak skills across agents/tenants (matches .NET, whose custom-source constructor also adds no caching/dedup). Callers who want caching on a custom pipeline compose `CachingSkillsSource(inner, cache_isolation_key_selector=...)` themselves. `disable_caching=True` only affects the built-in leaf caching (it has no effect on a caller-supplied source, which is never cached). `CachingSkillsSource` shares a single in-flight fetch across concurrent callers (per cache key) and does not update its cache on a failed fetch, so the next call retries (an initial failure leaves the cache empty; a refresh failure keeps the previously cached list). By default all callers share one cache bucket; pass `cache_isolation_key_selector=Callable[[SkillsSourceContext], str | None]` to cache separately per key (e.g. per agent name) for context-aware inner sources — the key should be low-cardinality and stable, and returning `None` (or leaving the selector `None`) uses the shared bucket. By default a cached list never expires; pass `refresh_interval=timedelta(...)` (port of .NET `CachingAgentSkillsSourceOptions.RefreshInterval`) to treat a cached list as stale once it is older than the interval so the next call re-queries the inner source (useful when an inner source such as `MCPSkillsSource` changes over the process lifetime; a zero/negative interval makes every result immediately stale, and a failed refresh keeps the prior list and retries). Freshness is measured with a monotonic clock (`time.monotonic()`). `SkillsProvider.__init__` / `from_paths` expose a `cache_refresh_interval` kwarg that is threaded into the built-in `CachingSkillsSource` (it has no effect on a caller-supplied source or when `disable_caching=True`). **`MCPSkillsSource` and `MCPSkill` accept exactly one of `client` (a fixed `ClientSession`) or `session_provider` (`Callable[[], ClientSession]`, resolved on every fetch); providing both/neither raises `ValueError`.** Use `session_provider` when the underlying session may be swapped over time — e.g. a reconnecting `MCPTool`/`FoundryToolbox` whose `session` is replaced on reconnect — so cached `MCPSkill`s keep fetching against the live session instead of a closed one (`MCPSkillsSource` forwards its provider to every `MCPSkill` it creates). A fixed `client` is safe only when the session outlives the skills. @@ -187,7 +189,7 @@ The vector store API is experimental under the shared `VECTOR_STORES` feature ID ### File Memory Harness (`_harness/_file_memory.py`) - **`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. +- **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. - **`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. diff --git a/python/packages/core/agent_framework/_filesystem.py b/python/packages/core/agent_framework/_filesystem.py index ac907621da5..512ca17ff7f 100644 --- a/python/packages/core/agent_framework/_filesystem.py +++ b/python/packages/core/agent_framework/_filesystem.py @@ -4,11 +4,47 @@ from __future__ import annotations +import hashlib import stat +from base64 import b32encode from pathlib import Path +_WINDOWS_RESERVED_FILE_STEMS: frozenset[str] = frozenset({ + "CON", + "PRN", + "AUX", + "NUL", + "COM1", + "COM2", + "COM3", + "COM4", + "COM5", + "COM6", + "COM7", + "COM8", + "COM9", + "LPT1", + "LPT2", + "LPT3", + "LPT4", + "LPT5", + "LPT6", + "LPT7", + "LPT8", + "LPT9", + "COM¹", + "COM²", + "COM³", + "LPT¹", + "LPT²", + "LPT³", +}) -def is_link_or_reparse_point(path: Path) -> bool: +_MAX_ENCODED_STORAGE_KEY_SEGMENT_LENGTH = 180 +_DIGEST_SEGMENT_MARKER = "sha256-" + + +def _is_link_or_reparse_point(path: Path) -> bool: # pyright: ignore[reportUnusedFunction] """Return whether ``path`` is a symbolic link, junction, or other reparse point.""" path_stat = path.lstat() if stat.S_ISLNK(path_stat.st_mode): @@ -21,3 +57,82 @@ def is_link_or_reparse_point(path: Path) -> bool: reparse_attribute = getattr(stat, "FILE_ATTRIBUTE_REPARSE_POINT", 0) file_attributes = getattr(path_stat, "st_file_attributes", 0) return bool(reparse_attribute and file_attributes & reparse_attribute) + + +def _is_literal_storage_key_segment_safe(value: str) -> bool: + """Return whether an opaque identifier can be used verbatim as one path segment. + + The predicate is deliberately conservative because the result is used as a + security boundary (see :func:`_storage_key_segment`). Only *lowercase* ASCII + alphanumerics and ``.``/``_``/``-`` are accepted, so a literal segment can + never be folded onto a different identifier by the filesystem: + + * Rejecting non-ASCII avoids Unicode normalization folding (macOS APFS and + HFS+ map the NFD and NFC forms of the same text onto one directory entry). + * Rejecting uppercase avoids case folding (NTFS and APFS are case-insensitive + by default, so ``A`` and ``a`` would be one directory entry). Uppercase + values are encoded rather than lowercased, because lowercasing ``A`` would + collide with a genuine ``a``. + + Values that are not literal-safe are encoded rather than rejected. + """ + if ( + not value + or value.startswith(".") + or value.endswith((" ", ".")) + or value.split(".", maxsplit=1)[0].upper() in _WINDOWS_RESERVED_FILE_STEMS + ): + return False + if any(ord(character) < 32 for character in value): + return False + return all( + character.isascii() and ((character.isalnum() and not character.isupper()) or character in "._-") + for character in value + ) + + +def _storage_key_segment(value: str, *, encoded_prefix: str) -> str: # pyright: ignore[reportUnusedFunction] + """Return a filesystem-safe path segment for an opaque identifier. + + This is the single derivation used everywhere an identifier that participates + in an isolation boundary (a session ID, an owner ID, a memory scope) is turned + into a storage location. Callers must not pre-normalize the value: a general + path canonicalizer is lossy, and identifiers that differ only in separators or + surrounding whitespace would then share one storage namespace. + + The derivation has three branches: + + * Literal-safe values are returned verbatim, so on-disk layouts stay readable. + * Everything else is encoded under ``encoded_prefix`` using lowercase base32. + This branch is **injective**: base32 is exactly reversible, and its alphabet + (``a``-``z`` and ``2``-``7``) is case-stable, so the mapping stays one-to-one + even on a case-insensitive filesystem. The literal and encoded namespaces + cannot overlap because :func:`_is_literal_storage_key_segment_safe` rejects + every value starting with ``~``, which each ``encoded_prefix`` begins with. + * Past :data:`_MAX_ENCODED_STORAGE_KEY_SEGMENT_LENGTH` the encoding is replaced + by a SHA-256 digest so the segment stays within filesystem name limits. This + branch maps an unbounded input space onto 256 bits, so it is + **collision-resistant** rather than injective: distinct identifiers sharing a + namespace requires finding a SHA-256 collision. + + Args: + value: The opaque identifier to convert. + + Keyword Args: + encoded_prefix: Component-specific prefix applied to encoded values, so + that two components encoding the same identifier stay distinguishable. + Must start with ``~`` to stay outside the literal namespace. + + Returns: + A single path segment containing no path separators. + """ + if _is_literal_storage_key_segment_safe(value): + return value + encoded_value = b32encode(value.encode("utf-8")).decode("ascii").rstrip("=").lower() + encoded_segment = f"{encoded_prefix}{encoded_value}" + if len(encoded_segment) <= _MAX_ENCODED_STORAGE_KEY_SEGMENT_LENGTH: + return encoded_segment + # ``-`` is outside the base32 alphabet, so a digest segment can never collide + # with the encoding of some other (shorter) identifier. + digest = hashlib.sha256(value.encode("utf-8")).hexdigest() + return f"{encoded_prefix}{_DIGEST_SEGMENT_MARKER}{digest}" diff --git a/python/packages/core/agent_framework/_harness/_file_access.py b/python/packages/core/agent_framework/_harness/_file_access.py index a4f074ff0f7..7dc95779ebc 100644 --- a/python/packages/core/agent_framework/_harness/_file_access.py +++ b/python/packages/core/agent_framework/_harness/_file_access.py @@ -35,7 +35,7 @@ from pydantic import BaseModel, Field from .._feature_stage import ExperimentalFeature, experimental -from .._filesystem import is_link_or_reparse_point +from .._filesystem import _is_link_or_reparse_point # pyright: ignore[reportPrivateUsage] from .._serialization import SerializationMixin from .._sessions import AgentSession, ContextProvider, SessionContext from .._telemetry import FeatureIndex, mark_feature_used @@ -157,6 +157,14 @@ def _normalize_relative_path(path: str, *, is_directory: bool = False) -> str: trailing separators are not accepted (so ``"foo/"`` does not silently become the file path ``"foo"``). + This is for *paths supplied as tool arguments*. It is deliberately lossy: + ``"a/"`` and ``"a//"`` both normalize to ``"a"``, as does the equivalent + backslash-terminated spelling. Never use it to derive a storage namespace + from an identifier that participates in an isolation boundary (a session ID, + owner ID, or memory scope) — distinct identifiers would share one location. + Use :func:`~agent_framework._filesystem._storage_key_segment` for that + instead. + Args: path: The relative path to normalize. @@ -1160,7 +1168,7 @@ def _resolve_safe_directory_path(self, relative_directory: str) -> Path: # normal and must stay cheap, but a dangling link still has to be rejected rather than # read as absent. if os.path.lexists(self._root_path): - if is_link_or_reparse_point(self._root_path): + if _is_link_or_reparse_point(self._root_path): raise ValueError("Invalid path: the resolved path contains a symbolic link or reparse point.") if self._root_path.resolve() != self._root_path: raise ValueError("Invalid path: the resolved path escapes the root directory.") @@ -1187,7 +1195,7 @@ def _throw_if_contains_symlink(self, candidate: Path) -> None: for segment in relative_parts: current = current / segment try: - is_link = is_link_or_reparse_point(current) + is_link = _is_link_or_reparse_point(current) except FileNotFoundError: break except OSError as exc: @@ -1301,7 +1309,7 @@ def _list_sync(full_dir: Path) -> list[FileStoreEntry]: files: list[FileStoreEntry] = [] for entry in full_dir.iterdir(): try: - is_link = is_link_or_reparse_point(entry) + is_link = _is_link_or_reparse_point(entry) except OSError: # Fail closed when an entry cannot be inspected. continue @@ -1362,7 +1370,7 @@ def _enumerate_search_files(full_dir: Path, recursive: bool) -> list[tuple[str, current = directories.pop() for entry in current.iterdir(): try: - is_link = is_link_or_reparse_point(entry) + is_link = _is_link_or_reparse_point(entry) except OSError: # Fail closed when an entry cannot be inspected. continue @@ -1392,7 +1400,7 @@ def _search_files_sync( # Re-checked here, not only during enumeration: a candidate can be swapped for a # link in between. O_NOFOLLOW makes the read itself atomic where the platform has # it, and this narrows the window on Windows, where it does not exist. - if is_link_or_reparse_point(entry): + if _is_link_or_reparse_point(entry): logger.warning("Skipping symlinked file during search: %s", entry) skipped.append(relative_name) continue diff --git a/python/packages/core/agent_framework/_harness/_file_memory.py b/python/packages/core/agent_framework/_harness/_file_memory.py index 1dff633906c..bed59710b74 100644 --- a/python/packages/core/agent_framework/_harness/_file_memory.py +++ b/python/packages/core/agent_framework/_harness/_file_memory.py @@ -19,6 +19,14 @@ folder (derived from the session id). Pass an explicit ``scope`` to group memories differently, for example by user id. +A ``scope`` (and the session id it defaults to) is treated as an **opaque +namespace key**, not as a path: it is mapped to exactly one storage folder by a +derivation that is injective for all but pathologically long values, so two +byte-distinct values cannot share a working folder. Applications that derive a +scope from an externally supplied value should still canonicalize and authorize +that value themselves; the mapping here is the storage-layer guarantee, not a +substitute for that check. + The provider exposes the following tools to the agent (registered on the per-invocation :class:`~agent_framework.SessionContext` in :meth:`FileMemoryProvider.before_run`): @@ -40,6 +48,7 @@ from pydantic import BaseModel, Field +from .._filesystem import _storage_key_segment # pyright: ignore[reportPrivateUsage] from .._sessions import AgentSession, ContextProvider, SessionContext from .._tools import tool from .._types import Message @@ -77,6 +86,10 @@ _DESCRIPTION_SUFFIX = "_description.md" _MEMORY_INDEX_FILE_NAME = "memories.md" _MAX_INDEX_ENTRIES = 50 +# 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-" def _description_file_name(file_name: str) -> str: @@ -247,6 +260,11 @@ class FileMemoryProvider(ContextProvider): Memories are isolated per session: each session reads and writes under a working folder derived from its session id. Pass an explicit ``scope`` to group memories differently (for example, per user id) across sessions. + + The scope (or session id) is an opaque namespace key, not a path. It is + mapped to a single folder by a derivation that is injective for all but + pathologically long values, so two byte-distinct values cannot share a + working folder. """ def __init__( @@ -267,7 +285,11 @@ def __init__( scope: The namespace that logically groups and isolates memories (for example, a user ID). Used as the working folder within the store. When ``None`` (the default), the active session's - ``session_id`` is used, isolating memories per session. + ``session_id`` is used, isolating memories per session. The + value is treated as an opaque key rather than a path: it is + mapped onto exactly one folder, so a multi-segment value + such as ``"tenants/alice"`` becomes a single encoded folder + instead of a nested directory. instructions: Optional instruction override. When ``None`` the default file-memory instructions are used. """ @@ -284,11 +306,26 @@ def _resolve_working_folder(self, context: SessionContext) -> str: """Resolve the working folder for the current invocation. Uses the configured ``scope`` when set, otherwise the session id. The - result is normalized as a relative directory path so it cannot escape - the store root. + value is an opaque namespace key, not a path: it is mapped to exactly + one folder name by :func:`~agent_framework._filesystem._storage_key_segment`. + That derivation is injective except for pathologically long values, + which fall back to a collision-resistant digest. Two byte-distinct + scopes or session ids therefore do not resolve to the same working + folder, so a caller authorized for one of them cannot reach another's + memories. + + Raises: + ValueError: When neither ``scope`` nor the session id yields a + namespace. Without one there is nothing to isolate on, and + falling back to the store root would expose every other scope. """ raw_scope = self.scope or context.session_id or "" - return _normalize_relative_path(raw_scope, is_directory=True) + if not raw_scope: + raise ValueError( + "FileMemoryProvider requires a memory scope: pass an explicit 'scope' or run with a session " + "that has a 'session_id'. Without one, memories cannot be isolated from other scopes." + ) + return _storage_key_segment(raw_scope, encoded_prefix=_ENCODED_SCOPE_PREFIX) async def _rebuild_index(self, working_folder: str) -> None: """Rebuild the ``memories.md`` index for ``working_folder``. diff --git a/python/packages/core/agent_framework/_harness/_memory.py b/python/packages/core/agent_framework/_harness/_memory.py index 2c43cb70158..66163abed05 100644 --- a/python/packages/core/agent_framework/_harness/_memory.py +++ b/python/packages/core/agent_framework/_harness/_memory.py @@ -10,7 +10,8 @@ import threading import weakref from abc import ABC, abstractmethod -from base64 import urlsafe_b64decode, urlsafe_b64encode +from base64 import b32decode +from binascii import Error as BinasciiError from collections.abc import Callable, Mapping, Sequence from datetime import datetime, timedelta, timezone from pathlib import Path @@ -19,6 +20,7 @@ from .._clients import SupportsChatGetResponse from .._compaction import group_messages from .._feature_stage import ExperimentalFeature, experimental +from .._filesystem import _storage_key_segment # pyright: ignore[reportPrivateUsage] from .._sessions import AgentSession, FileHistoryProvider, HistoryProvider, JsonDumps, JsonLoads, SessionContext from .._telemetry import FeatureIndex, mark_feature_used from .._tools import tool @@ -648,7 +650,13 @@ def search_transcripts( session_id: str | None = None, limit: int = 20, ) -> list[dict[str, Any]]: - """Search the raw transcript archive for matching text snippets.""" + """Search the raw transcript archive for matching text snippets. + + When ``session_id`` is given, only transcripts belonging to that session are searched, and + every returned row reports that same ``session_id``. Otherwise all transcripts are searched + and each row reports the session the transcript could be attributed to, or ``None`` when the + implementation cannot determine it. + """ @experimental(feature_id=ExperimentalFeature.HARNESS) @@ -731,10 +739,18 @@ def import_provider_state(self, session: AgentSession, *, state: Mapping[str, An ) session.state[self.owner_state_key] = owner_value - @staticmethod - def _encode_path_component(value: str) -> str: - encoded_value = urlsafe_b64encode(value.encode("utf-8")).decode("ascii").rstrip("=") - return encoded_value or "_" + _ENCODED_SEGMENT_PREFIX: ClassVar[str] = "~memory-" + + @classmethod + def _encode_path_component(cls, value: str) -> str: + """Return a filesystem-safe path segment for an owner ID or source ID. + + Delegates to the shared + :func:`~agent_framework._filesystem._storage_key_segment` derivation, so + two byte-distinct values do not share a directory (values past a + length cap fall back to a collision-resistant digest). + """ + return _storage_key_segment(value, encoded_prefix=cls._ENCODED_SEGMENT_PREFIX) def _get_memory_root(self, session: AgentSession, *, source_id: str) -> Path: owner_component = self._encode_path_component(f"{self.owner_prefix}{self._get_owner_id(session)}") @@ -773,8 +789,26 @@ def _decode_transcript_session_id(file_path: Path) -> str | None: if not file_stem.startswith(_FILE_HISTORY_ENCODED_SESSION_PREFIX): return file_stem encoded_value = file_stem[len(_FILE_HISTORY_ENCODED_SESSION_PREFIX) :] - padded_value = encoded_value + ("=" * (-len(encoded_value) % 4)) - return urlsafe_b64decode(padded_value.encode("ascii")).decode("utf-8") + padded_value = encoded_value + ("=" * (-len(encoded_value) % 8)) + try: + return b32decode(padded_value.encode("ascii"), casefold=True).decode("utf-8") + except (BinasciiError, UnicodeDecodeError, ValueError): + # Very long session IDs are stored under an irreversible digest stem, + # and unrelated files may share the prefix. Neither maps back to a + # session ID, so treat the transcript as unattributed rather than + # failing the whole scan. + return None + + @staticmethod + def _transcript_file_stem(session_id: str) -> str: + """Return the transcript file stem that ``FileHistoryProvider`` writes for ``session_id``. + + This is the forward counterpart to :meth:`_decode_transcript_session_id`. Deriving the stem + works for every session ID, including the very long ones stored under an irreversible digest + stem that cannot be decoded back to a session ID. + """ + raw_session_id = session_id or FileHistoryProvider.DEFAULT_SESSION_FILE_STEM + return _storage_key_segment(raw_session_id, encoded_prefix=_FILE_HISTORY_ENCODED_SESSION_PREFIX) def list_topics(self, session: AgentSession, *, source_id: str) -> list[MemoryTopicRecord]: """Return all topic memory files visible from the current owner.""" @@ -890,7 +924,13 @@ def search_transcripts( session_id: str | None = None, limit: int = 20, ) -> list[dict[str, Any]]: - """Search the raw transcript archive for matching text snippets.""" + """Search the raw transcript archive for matching text snippets. + + When ``session_id`` is given, transcripts are selected by deriving the file stem that + :class:`FileHistoryProvider` writes for that session ID and comparing it to each file's stem. + Matching forward this way keeps transcripts reachable even when their stem is an irreversible + digest, which reverse-decoding cannot recover. + """ normalized_query = query.strip() if not normalized_query: raise ValueError("query must not be empty.") @@ -898,12 +938,17 @@ def search_transcripts( transcripts_directory = self.get_transcripts_directory(session, source_id=source_id) if not transcripts_directory.exists(): return [] + expected_stem = None if session_id is None else self._transcript_file_stem(session_id) transcript_files = sorted(transcripts_directory.glob("*.jsonl")) results: list[dict[str, Any]] = [] for transcript_file in transcript_files: - decoded_session_id = self._decode_transcript_session_id(transcript_file) - if session_id is not None and decoded_session_id != session_id: - continue + if expected_stem is not None: + if transcript_file.stem != expected_stem: + continue + # Report what the caller asked for: decoding a digest stem yields ``None``. + matched_session_id = session_id + else: + matched_session_id = self._decode_transcript_session_id(transcript_file) with transcript_file.open(encoding="utf-8") as file_handle: for line_number, line in enumerate(file_handle, start=1): serialized = line.strip() @@ -917,7 +962,7 @@ def search_transcripts( if not text or query_casefold not in text.casefold(): continue results.append({ - "session_id": decoded_session_id, + "session_id": matched_session_id, "line_number": line_number, "role": message.role, "text": text, diff --git a/python/packages/core/agent_framework/_harness/_todo.py b/python/packages/core/agent_framework/_harness/_todo.py index 1c675097326..daa554a585e 100644 --- a/python/packages/core/agent_framework/_harness/_todo.py +++ b/python/packages/core/agent_framework/_harness/_todo.py @@ -7,7 +7,6 @@ import os import weakref from abc import ABC, abstractmethod -from base64 import urlsafe_b64encode from collections.abc import Mapping, MutableMapping from pathlib import Path from typing import Any, ClassVar, cast @@ -15,6 +14,10 @@ from typing_extensions import NotRequired, TypedDict from .._feature_stage import ExperimentalFeature, experimental +from .._filesystem import ( + _is_literal_storage_key_segment_safe, # pyright: ignore[reportPrivateUsage] + _storage_key_segment, # pyright: ignore[reportPrivateUsage] +) from .._serialization import SerializationMixin from .._sessions import AgentSession, ContextProvider, SessionContext from .._telemetry import FeatureIndex, mark_feature_used @@ -316,30 +319,6 @@ def __init__( self._base_root = self.base_path.resolve() _ENCODED_SEGMENT_PREFIX: ClassVar[str] = "~todo-" - _WINDOWS_RESERVED_FILE_STEMS: ClassVar[frozenset[str]] = frozenset({ - "CON", - "PRN", - "AUX", - "NUL", - "COM1", - "COM2", - "COM3", - "COM4", - "COM5", - "COM6", - "COM7", - "COM8", - "COM9", - "LPT1", - "LPT2", - "LPT3", - "LPT4", - "LPT5", - "LPT6", - "LPT7", - "LPT8", - "LPT9", - }) def _get_state_path(self, session: AgentSession, *, source_id: str) -> Path: """Return the JSON file path for one session and source ID.""" @@ -362,28 +341,22 @@ def _get_state_path(self, session: AgentSession, *, source_id: str) -> Path: @classmethod def _path_segment(cls, value: object, *, label: str, reject_path_separators: bool = False) -> str: - """Return a filesystem-safe path segment for user-controlled state values.""" + """Return a filesystem-safe path segment for user-controlled state values. + + Delegates to the shared + :func:`~agent_framework._filesystem._storage_key_segment` derivation, so + two byte-distinct values do not share a directory (values past a + length cap fall back to a collision-resistant digest). + """ raw_value = str(value) if reject_path_separators and ("/" in raw_value or "\\" in raw_value): raise ValueError(f"TodoFileStore {label} must not contain path separators: {raw_value!r}") - if cls._is_literal_path_segment_safe(raw_value): - return raw_value - encoded_value = urlsafe_b64encode(raw_value.encode("utf-8")).decode("ascii").rstrip("=") - return f"{cls._ENCODED_SEGMENT_PREFIX}{encoded_value or label}" + return _storage_key_segment(raw_value, encoded_prefix=cls._ENCODED_SEGMENT_PREFIX) @classmethod def _is_literal_path_segment_safe(cls, value: str) -> bool: """Return whether a value can be used directly as one path segment.""" - if ( - not value - or value.startswith(".") - or value.endswith((" ", ".")) - or value.upper() in cls._WINDOWS_RESERVED_FILE_STEMS - ): - return False - if any(ord(character) < 32 for character in value): - return False - return all(character.isalnum() or character in "._-" for character in value) + return _is_literal_storage_key_segment_safe(value) def _state_filename(self, source_id: str) -> str: """Return a source-specific JSON state filename.""" diff --git a/python/packages/core/agent_framework/_sessions.py b/python/packages/core/agent_framework/_sessions.py index 27e313b8e6e..c11a8ae9c28 100644 --- a/python/packages/core/agent_framework/_sessions.py +++ b/python/packages/core/agent_framework/_sessions.py @@ -18,7 +18,6 @@ import asyncio import contextlib import copy -import hashlib import json import logging import math @@ -28,7 +27,6 @@ import warnings import weakref from abc import abstractmethod -from base64 import urlsafe_b64encode from collections import deque from collections.abc import AsyncIterable, Awaitable, Callable, Generator, Iterable, Mapping, Sequence from contextvars import ContextVar, Token @@ -40,6 +38,10 @@ import msgspec from ._feature_stage import ExperimentalFeature, experimental +from ._filesystem import ( + _is_literal_storage_key_segment_safe, # pyright: ignore[reportPrivateUsage] + _storage_key_segment, # pyright: ignore[reportPrivateUsage] +) from ._middleware import ChatContext, ChatMiddleware from ._telemetry import FeatureIndex, mark_feature_used from ._types import ( @@ -73,36 +75,6 @@ StateEncoder: TypeAlias = Callable[[Any], Mapping[str, Any]] StateDecoder: TypeAlias = Callable[[Mapping[str, Any]], Any] _STATE_SCALAR_TYPES = (str, int, float, bool, type(None)) -_WINDOWS_RESERVED_FILE_STEMS: frozenset[str] = frozenset({ - "CON", - "PRN", - "AUX", - "NUL", - "COM1", - "COM2", - "COM3", - "COM4", - "COM5", - "COM6", - "COM7", - "COM8", - "COM9", - "LPT1", - "LPT2", - "LPT3", - "LPT4", - "LPT5", - "LPT6", - "LPT7", - "LPT8", - "LPT9", - "COM¹", - "COM²", - "COM³", - "LPT¹", - "LPT²", - "LPT³", -}) _DEFAULT_JSON_ENCODER = msgspec.json.Encoder() @@ -113,7 +85,6 @@ _JSON_LINES_FILE_EXTENSION = ".jsonl" _MSGPACK_FILE_EXTENSION = ".msgpack" _SESSION_SNAPSHOT_VERSION = "1.0" -_MAX_ENCODED_SESSION_FILE_STEM_LENGTH = 180 def _default_json_dumps(value: Any) -> bytes: @@ -152,29 +123,17 @@ def _is_literal_session_file_stem_safe(session_id: str) -> bool: with separators and platform-reserved names such as ``CON``. Unsafe values are encoded by :func:`_session_file_stem` rather than rejected. """ - windows_stem = session_id.split(".", maxsplit=1)[0].upper() - if ( - not session_id - or session_id.startswith(".") - or session_id.endswith((" ", ".")) - or windows_stem in _WINDOWS_RESERVED_FILE_STEMS - ): - return False - if any(ord(character) < 32 for character in session_id): - return False - return all(character.isascii() and (character.isalnum() or character in "._-") for character in session_id) + return _is_literal_storage_key_segment_safe(session_id) def _session_file_stem(session_id: str, *, encoded_prefix: str) -> str: - """Return a safe filename stem for an opaque session ID.""" - if _is_literal_session_file_stem_safe(session_id): - return session_id - encoded_session_id = urlsafe_b64encode(session_id.encode("utf-8")).decode("ascii").rstrip("=") - encoded_stem = f"{encoded_prefix}{encoded_session_id}" - if len(encoded_stem) <= _MAX_ENCODED_SESSION_FILE_STEM_LENGTH: - return encoded_stem - digest = hashlib.sha256(session_id.encode("utf-8")).hexdigest() - return f"{encoded_prefix}sha256-{digest}" + """Return a safe filename stem for an opaque session ID. + + Delegates to the shared :func:`~agent_framework._filesystem._storage_key_segment` + derivation so every component that maps an identifier onto storage produces + the same injective result. + """ + return _storage_key_segment(session_id, encoded_prefix=encoded_prefix) def _deduplicate_origin_session_ids(origin_session_ids: Iterable[str]) -> list[str]: diff --git a/python/packages/core/agent_framework/_skills.py b/python/packages/core/agent_framework/_skills.py index d16d5616ea9..e118464438a 100644 --- a/python/packages/core/agent_framework/_skills.py +++ b/python/packages/core/agent_framework/_skills.py @@ -66,7 +66,7 @@ from typing import IO, TYPE_CHECKING, Any, ClassVar, Final, Protocol, TypeAlias, TypeVar, cast, runtime_checkable from ._feature_stage import ExperimentalFeature, experimental -from ._filesystem import is_link_or_reparse_point +from ._filesystem import _is_link_or_reparse_point # pyright: ignore[reportPrivateUsage] from ._sessions import ContextProvider from ._telemetry import FeatureIndex, mark_feature_used from ._tools import ApprovalMode, FunctionTool @@ -3022,7 +3022,7 @@ def _has_link_or_reparse_point_in_path(path: str, directory: str) -> bool: for part in relative.parts: current = current / part try: - is_link = is_link_or_reparse_point(current) + is_link = _is_link_or_reparse_point(current) except OSError: return True if is_link: @@ -3569,7 +3569,7 @@ def _discover_skill_directories(skill_paths: Sequence[str]) -> list[str]: def _is_unsafe_link(path: Path) -> bool: try: - return is_link_or_reparse_point(path) + return _is_link_or_reparse_point(path) except OSError: return True diff --git a/python/packages/core/tests/core/test_filesystem.py b/python/packages/core/tests/core/test_filesystem.py new file mode 100644 index 00000000000..764f8307aac --- /dev/null +++ b/python/packages/core/tests/core/test_filesystem.py @@ -0,0 +1,229 @@ +# Copyright (c) Microsoft. All rights reserved. + +"""Tests for the shared storage-key derivation in ``agent_framework._filesystem``. + +Every component that maps a caller-controlled identifier (a session ID, an owner +ID, a memory scope) onto a storage location routes through +:func:`_storage_key_segment`. Its defining property is **injectivity**: two +byte-distinct identifiers must not produce the same segment, because the segment +is an isolation boundary. Only the digest fallback for pathologically long +values weakens this to collision resistance. + +Injectivity is asserted **case-insensitively** throughout, because NTFS and APFS +are case-insensitive by default: two segments that differ only in case are one +directory entry there, which would reintroduce the collision this module exists +to prevent. +""" + +from __future__ import annotations + +import pytest + +from agent_framework._filesystem import ( # pyright: ignore[reportPrivateUsage] + _MAX_ENCODED_STORAGE_KEY_SEGMENT_LENGTH, + _is_literal_storage_key_segment_safe, + _storage_key_segment, +) + +# Identifiers that a lossy path normalizer -- or a case-insensitive filesystem -- +# would fold together. Shared with the per-provider parity tests so every +# component is held to the same contract. +COLLIDING_IDENTIFIERS: tuple[str, ...] = ( + "customer-42", + "customer-42/", + "customer-42//", + "customer-42\\", + "customer-42\\\\", + "/customer-42", + "//customer-42", + " customer-42", + "customer-42 ", + " customer-42 ", + "\tcustomer-42", + "customer-42/.", + "./customer-42", + "customer-42/..", + # Case variants: distinct identifiers that a case-insensitive filesystem + # folds onto one directory entry unless they are encoded apart. + "Customer-42", + "CUSTOMER-42", + "cUsToMeR-42", + "a", + "A", +) + +UNSAFE_IDENTIFIERS: tuple[str, ...] = ( + "", + ".", + "..", + ".hidden", + "trailing.", + "trailing ", + "CON", + "con", + "CON.txt", + "NUL.log", + "LPT1", + "COM\u00b9", + "with\nnewline", + "with\x00null", + "caf\u00e9", # NFC + "cafe\u0301", # NFD - byte-distinct from the NFC form above + "\u65e5\u672c\u8a9e", + "a/b", + "a\\b", + "a:b", + "a*b", + "a b", + "UID1", # uppercase is encoded, never lowercased + "Session-1", + "0198F0C5-1A2B-7C3D-8E4F-5A6B7C8D9E0F", +) + +SAFE_IDENTIFIERS: tuple[str, ...] = ( + "customer-42", + "customer_42", + "customer.42", + "0", + "a", + "0198f0c5-1a2b-7c3d-8e4f-5a6b7c8d9e0f", + "session-1", + "console", + "cont", + "x" * 200, +) + + +def test_safe_identifiers_are_used_verbatim() -> None: + """An already-safe identifier keeps its literal folder name. + + This is what keeps typical deployments (lowercase UUIDs, slugs) from needing + a data migration. + """ + for value in SAFE_IDENTIFIERS: + assert _is_literal_storage_key_segment_safe(value) + assert _storage_key_segment(value, encoded_prefix="~scope-") == value + + +def test_unsafe_identifiers_are_encoded_not_rejected() -> None: + for value in UNSAFE_IDENTIFIERS: + assert not _is_literal_storage_key_segment_safe(value) + segment = _storage_key_segment(value, encoded_prefix="~scope-") + assert segment.startswith("~scope-") + + +def test_derivation_is_injective_over_colliding_identifiers() -> None: + """The MSRC case: values a path normalizer folds together stay distinct.""" + segments = {value: _storage_key_segment(value, encoded_prefix="~scope-") for value in COLLIDING_IDENTIFIERS} + assert len(set(segments.values())) == len(COLLIDING_IDENTIFIERS), segments + + +def test_derivation_is_injective_over_every_known_identifier() -> None: + all_values = (*COLLIDING_IDENTIFIERS, *UNSAFE_IDENTIFIERS, *SAFE_IDENTIFIERS) + unique_values = set(all_values) + segments = {_storage_key_segment(value, encoded_prefix="~scope-") for value in unique_values} + assert len(segments) == len(unique_values) + + +def test_derivation_is_injective_on_case_insensitive_filesystems() -> None: + """Distinct identifiers must not fold together on NTFS or APFS. + + Comparing case-folded segments is the same check the filesystem performs + when deciding whether two names are one directory entry. + """ + all_values = (*COLLIDING_IDENTIFIERS, *UNSAFE_IDENTIFIERS, *SAFE_IDENTIFIERS) + unique_values = set(all_values) + folded = {_storage_key_segment(value, encoded_prefix="~scope-").lower() for value in unique_values} + assert len(folded) == len(unique_values) + + +def test_uppercase_identifiers_are_encoded_rather_than_lowercased() -> None: + """``A`` must not be stored as ``a``: that is a genuine other identifier.""" + assert _storage_key_segment("a", encoded_prefix="~scope-") == "a" + upper = _storage_key_segment("A", encoded_prefix="~scope-") + assert upper.startswith("~scope-") + assert upper.lower() != "a" + + +def test_encoded_segments_use_a_case_stable_alphabet() -> None: + """Lowercase base32 (``a``-``z``, ``2``-``7``) survives case folding intact.""" + alphabet = set("abcdefghijklmnopqrstuvwxyz234567") + for value in (*COLLIDING_IDENTIFIERS, *UNSAFE_IDENTIFIERS): + segment = _storage_key_segment(value, encoded_prefix="~scope-") + if not segment.startswith("~scope-"): + continue + body = segment.removeprefix("~scope-") + assert set(body) <= alphabet, (value, segment) + assert body == body.lower() + + +def test_unicode_normalization_forms_do_not_collide() -> None: + """NFC and NFD spellings are byte-distinct and must stay distinct. + + A literal non-ASCII folder name would be folded onto one directory entry by + macOS APFS/HFS+, so non-ASCII values are always encoded. + """ + nfc = _storage_key_segment("caf\u00e9", encoded_prefix="~scope-") + nfd = _storage_key_segment("cafe\u0301", encoded_prefix="~scope-") + assert nfc != nfd + + +def test_segments_never_contain_path_separators() -> None: + for value in (*COLLIDING_IDENTIFIERS, *UNSAFE_IDENTIFIERS, *SAFE_IDENTIFIERS): + segment = _storage_key_segment(value, encoded_prefix="~scope-") + assert "/" not in segment + assert "\\" not in segment + + +def test_literal_namespace_cannot_collide_with_encoded_namespace() -> None: + """No literal-safe value can look like an encoded segment. + + ``~`` is outside the literal charset, so an attacker cannot pick a plain + identifier that lands on some other identifier's encoded folder. + """ + assert not _is_literal_storage_key_segment_safe("~scope-mn2xg5dpnvsxeljugixq") + literal = _storage_key_segment("customer-42", encoded_prefix="~scope-") + encoded = _storage_key_segment("customer-42/", encoded_prefix="~scope-") + assert not literal.startswith("~") + assert encoded.startswith("~") + + +def test_prefix_separates_components_for_the_same_identifier() -> None: + """Two components encoding the same value stay in distinct folders.""" + value = "customer-42/" + assert _storage_key_segment(value, encoded_prefix="~scope-") != _storage_key_segment(value, encoded_prefix="~todo-") + + +def test_long_identifiers_fall_back_to_a_digest_segment() -> None: + long_value = "customer-42/" + "x" * 400 + segment = _storage_key_segment(long_value, encoded_prefix="~scope-") + assert segment.startswith("~scope-sha256-") + assert len(segment) <= _MAX_ENCODED_STORAGE_KEY_SEGMENT_LENGTH + + +def test_digest_segments_are_collision_resistant() -> None: + """The digest branch is collision-resistant, not injective. + + SHA-256 maps an unbounded input space onto 256 bits, so distinct long + identifiers sharing a namespace requires finding a SHA-256 collision. + """ + first = _storage_key_segment("a" * 400, encoded_prefix="~scope-") + second = _storage_key_segment("a" * 401, encoded_prefix="~scope-") + assert first != second + + +def test_digest_marker_is_outside_the_encoded_alphabet() -> None: + """A base32 encoding can never be mistaken for a digest segment. + + ``-`` is not a base32 character, so the two encoded forms occupy disjoint + namespaces even though they share a prefix. + """ + encoded = _storage_key_segment("customer-42/", encoded_prefix="~scope-") + assert "-" not in encoded.removeprefix("~scope-") + + +@pytest.mark.parametrize("value", ["customer-42", "customer-42/", "caf\u00e9", "a" * 400]) +def test_derivation_is_deterministic(value: str) -> None: + assert _storage_key_segment(value, encoded_prefix="~scope-") == _storage_key_segment( + value, encoded_prefix="~scope-" + ) diff --git a/python/packages/core/tests/core/test_harness_file_access.py b/python/packages/core/tests/core/test_harness_file_access.py index 2789f438228..da7b4255f40 100644 --- a/python/packages/core/tests/core/test_harness_file_access.py +++ b/python/packages/core/tests/core/test_harness_file_access.py @@ -29,7 +29,7 @@ Message, SupportsChatGetResponse, ) -from agent_framework._filesystem import is_link_or_reparse_point +from agent_framework._filesystem import _is_link_or_reparse_point from agent_framework._harness import _file_access as _file_access_module from agent_framework._harness._file_access import ( _SEARCH_SNIPPET_RADIUS, @@ -335,7 +335,12 @@ async def test_filesystem_search_does_not_read_through_a_symlink( "_enumerate_search_files", staticmethod(lambda full_dir, recursive: [("notes.txt", swapped)]), ) - monkeypatch.setattr(_file_access_module, "is_link_or_reparse_point", lambda candidate: candidate == swapped) + monkeypatch.setattr( + _file_access_module, + "_is_link_or_reparse_point", + lambda candidate: candidate == swapped, + raising=True, + ) store = FileSystemAgentFileStore(root) results = await store.search("", "needle", recursive=True) @@ -957,7 +962,7 @@ def fake_lstat(self: Path) -> SimpleNamespace: monkeypatch.setattr(Path, "lstat", fake_lstat) - assert is_link_or_reparse_point(path) is True + assert _is_link_or_reparse_point(path) is True def test_file_access_harness_classes_are_marked_experimental() -> None: 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 aa416ad3cf1..1442b151890 100644 --- a/python/packages/core/tests/core/test_harness_file_memory.py +++ b/python/packages/core/tests/core/test_harness_file_memory.py @@ -26,6 +26,8 @@ ) from agent_framework._sessions import SessionContext +from .test_filesystem import COLLIDING_IDENTIFIERS + def _tool_by_name(tools: list[object], name: str) -> FunctionTool: """Return the tool with the requested name from a prepared tool list.""" @@ -481,6 +483,145 @@ async def current() -> str: assert "Duplicate" in _text(dup) +# region Session isolation (MSRC): the working folder derivation must be injective + + +async def test_colliding_session_ids_resolve_to_distinct_working_folders() -> None: + """Session IDs that a path normalizer folds together must stay separate. + + The provider previously ran the session ID through a lossy path normalizer, + so ``"customer-42"`` and ``"customer-42/"`` shared one working folder. An + application that treats the session ID as part of its authorization boundary + would then have that decision silently undone by the storage layer. + """ + provider = FileMemoryProvider(store=InMemoryAgentFileStore()) + folders = { + session_id: provider._resolve_working_folder( # pyright: ignore[reportPrivateUsage] + SessionContext(session_id=session_id, input_messages=[]) + ) + for session_id in COLLIDING_IDENTIFIERS + } + assert len(set(folders.values())) == len(COLLIDING_IDENTIFIERS), folders + # Case-insensitive too: NTFS and APFS fold names that differ only in case. + assert len({folder.lower() for folder in folders.values()}) == len(COLLIDING_IDENTIFIERS), folders + + +async def test_colliding_scopes_resolve_to_distinct_working_folders() -> None: + """An explicitly configured scope gets the same injectivity guarantee.""" + folders = { + scope: FileMemoryProvider(store=InMemoryAgentFileStore(), scope=scope)._resolve_working_folder( # pyright: ignore[reportPrivateUsage] + SessionContext(session_id="session-1", input_messages=[]) + ) + for scope in COLLIDING_IDENTIFIERS + } + assert len(set(folders.values())) == len(COLLIDING_IDENTIFIERS), folders + assert len({folder.lower() for folder in folders.values()}) == len(COLLIDING_IDENTIFIERS), folders + + +async def test_safe_session_id_keeps_its_literal_working_folder() -> None: + """Canonical IDs are unchanged, so existing stores need no migration.""" + provider = FileMemoryProvider(store=InMemoryAgentFileStore()) + folder = provider._resolve_working_folder( # pyright: ignore[reportPrivateUsage] + SessionContext(session_id="customer-42", input_messages=[]) + ) + assert folder == "customer-42" + + +async def test_working_folder_is_never_the_store_root() -> None: + """Without a scope or session ID the provider fails closed. + + An empty working folder is the store root, from which every other scope's + directory is visible and writable. + """ + provider = FileMemoryProvider(store=InMemoryAgentFileStore()) + with pytest.raises(ValueError, match="requires a memory scope"): + provider._resolve_working_folder( # pyright: ignore[reportPrivateUsage] + SessionContext(session_id=None, input_messages=[]) + ) + with pytest.raises(ValueError, match="requires a memory scope"): + provider._resolve_working_folder( # pyright: ignore[reportPrivateUsage] + SessionContext(session_id="", input_messages=[]) + ) + + +async def test_before_run_fails_closed_without_a_scope() -> None: + """The failure surfaces through the public provider entry point too.""" + provider = FileMemoryProvider(store=InMemoryAgentFileStore()) + with pytest.raises(ValueError, match="requires a memory scope"): + await provider.before_run( + agent=None, + session=AgentSession(session_id="ignored"), + context=SessionContext(session_id=None, input_messages=[]), + state={}, + ) + + +@pytest.mark.parametrize( + "attacker_session_id", + ["customer-42/", "customer-42//", "customer-42\\", " customer-42 ", "customer-42/."], +) +async def test_colliding_session_cannot_read_write_or_delete_victim_memory(attacker_session_id: str) -> None: + """End-to-end: every public memory tool stays scoped to its own session. + + Mirrors the reported attack: the victim owns ``customer-42`` and the + attacker is authorized only for a normalization variant of it. + """ + store = InMemoryAgentFileStore() + provider = FileMemoryProvider(store=store) + + _, victim = await _prepare(provider, session_id="customer-42") + await victim["file_memory_write"].invoke( + arguments={"file_name": "secret.md", "content": "VICTIM_TOKEN=demo-secret"} + ) + + _, attacker = await _prepare(provider, session_id=attacker_session_id) + + # Discovery surfaces must not reveal the victim's memory. + assert json.loads(_text(await attacker["file_memory_ls"].invoke())) == [] + assert json.loads(_text(await attacker["file_memory_grep"].invoke(arguments={"regex_pattern": "VICTIM"}))) == [] + + # Read must not return the victim's content. + read_result = _text(await attacker["file_memory_read"].invoke(arguments={"file_name": "secret.md"})) + assert "demo-secret" not in read_result + + # Mutations must not reach the victim's file. + await attacker["file_memory_write"].invoke( + arguments={"file_name": "secret.md", "content": "ATTACKER_REPLACED_MEMORY"} + ) + await attacker["file_memory_replace"].invoke( + arguments={"file_name": "secret.md", "old_string": "ATTACKER", "new_string": "OVERWRITTEN"} + ) + await attacker["file_memory_replace_lines"].invoke( + arguments={"file_name": "secret.md", "edits": [{"line_number": 1, "new_line": "CLOBBERED\n"}]} + ) + await attacker["file_memory_delete"].invoke(arguments={"file_name": "secret.md"}) + + # The victim's memory is intact and unchanged. + _, victim_after = await _prepare(provider, session_id="customer-42") + assert [e["name"] for e in json.loads(_text(await victim_after["file_memory_ls"].invoke()))] == ["secret.md"] + assert ( + _text(await victim_after["file_memory_read"].invoke(arguments={"file_name": "secret.md"})) + == "VICTIM_TOKEN=demo-secret" + ) + + +async def test_multi_segment_scope_becomes_a_single_folder() -> None: + """A scope is an opaque key, so it never expands into a nested directory.""" + provider = FileMemoryProvider(store=InMemoryAgentFileStore(), scope="tenants/alice") + folder = provider._resolve_working_folder( # pyright: ignore[reportPrivateUsage] + SessionContext(session_id="session-1", input_messages=[]) + ) + assert "/" not in folder + # ...and it must not share a folder with the sibling tenant. + sibling = FileMemoryProvider(store=InMemoryAgentFileStore(), scope="tenants/bob")._resolve_working_folder( # pyright: ignore[reportPrivateUsage] + SessionContext(session_id="session-1", input_messages=[]) + ) + assert folder != sibling + + +# endregion + + # region file-memory guards diff --git a/python/packages/core/tests/core/test_harness_memory.py b/python/packages/core/tests/core/test_harness_memory.py index 9605b165c6d..d1c7b3dac51 100644 --- a/python/packages/core/tests/core/test_harness_memory.py +++ b/python/packages/core/tests/core/test_harness_memory.py @@ -6,6 +6,7 @@ import json from collections.abc import Mapping, Sequence from datetime import datetime, timedelta, timezone +from pathlib import Path from typing import Any import pytest @@ -27,6 +28,8 @@ Message, ) +from .test_filesystem import COLLIDING_IDENTIFIERS + def _no_store_options() -> ChatOptions: return {"store": False} @@ -213,6 +216,75 @@ async def test_memory_file_store_writes_topics_index_state_and_transcripts(tmp_p ] +async def _write_transcript(store: MemoryFileStore, session: AgentSession, session_id: str, text: str) -> None: + """Append a one-message transcript for ``session_id`` through the real write path.""" + provider = FileHistoryProvider(store.get_transcripts_directory(session, source_id=DEFAULT_MEMORY_SOURCE_ID)) + await provider.save_messages(session_id, [Message(role="user", contents=[text])]) + + +async def test_search_transcripts_finds_session_stored_under_digest_stem(tmp_path) -> None: + """A session ID too long to encode is stored under an irreversible digest stem but stays findable.""" + session = AgentSession(session_id="session-1") + session.state["owner_id"] = "user-1" + store = MemoryFileStore(tmp_path, owner_state_key="owner_id") + long_session_id = "a/" + "a" * 108 + + await _write_transcript(store, session, "short-id", "Short session transcript.") + await _write_transcript(store, session, long_session_id, "Long session transcript.") + + # The long ID must genuinely land on a digest stem, otherwise this test proves nothing. + stem = store._transcript_file_stem(long_session_id) + assert "sha256-" in stem + assert store._decode_transcript_session_id(Path(f"{stem}.jsonl")) is None + + results = store.search_transcripts( + session, source_id=DEFAULT_MEMORY_SOURCE_ID, query="transcript", session_id=long_session_id + ) + assert [(result["session_id"], result["text"]) for result in results] == [ + (long_session_id, "Long session transcript.") + ] + + results = store.search_transcripts( + session, source_id=DEFAULT_MEMORY_SOURCE_ID, query="transcript", session_id="short-id" + ) + assert [(result["session_id"], result["text"]) for result in results] == [("short-id", "Short session transcript.")] + + unfiltered = store.search_transcripts(session, source_id=DEFAULT_MEMORY_SOURCE_ID, query="transcript") + assert {result["text"] for result in unfiltered} == {"Short session transcript.", "Long session transcript."} + + +async def test_search_transcripts_isolates_colliding_session_ids(tmp_path) -> None: + """Session IDs that normalize alike must each retrieve only their own transcript.""" + session = AgentSession(session_id="session-1") + session.state["owner_id"] = "user-1" + store = MemoryFileStore(tmp_path, owner_state_key="owner_id") + + for index, candidate in enumerate(COLLIDING_IDENTIFIERS): + await _write_transcript(store, session, candidate, f"Transcript {index}.") + + for index, candidate in enumerate(COLLIDING_IDENTIFIERS): + results = store.search_transcripts( + session, source_id=DEFAULT_MEMORY_SOURCE_ID, query="Transcript", session_id=candidate + ) + assert [result["text"] for result in results] == [f"Transcript {index}."] + assert all(result["session_id"] == candidate for result in results) + + +async def test_search_transcripts_returns_nothing_for_unknown_session(tmp_path) -> None: + """Filtering by a session that never wrote a transcript should return no results.""" + session = AgentSession(session_id="session-1") + session.state["owner_id"] = "user-1" + store = MemoryFileStore(tmp_path, owner_state_key="owner_id") + await _write_transcript(store, session, "known-id", "Known transcript.") + + assert ( + store.search_transcripts( + session, source_id=DEFAULT_MEMORY_SOURCE_ID, query="transcript", session_id="unknown-id" + ) + == [] + ) + + def test_memory_file_store_rejects_owner_path_traversal(tmp_path) -> None: """Owner IDs with path traversal segments should not escape ``base_path``.""" session = AgentSession(session_id="session-1") @@ -875,3 +947,62 @@ def test_extract_keywords_handles_non_english_text() -> None: assert cyrillic == {"привет", "мир", "друзья"} # English extraction is unchanged. assert english == {"hello", "world"} + + +# region Storage-key parity: identifiers must map injectively onto directories + + +def test_memory_file_store_derives_distinct_roots_for_colliding_owner_ids(tmp_path) -> None: + """Owner IDs that a path normalizer would fold together stay separate. + + ``MemoryFileStore`` shares the storage-key derivation with the session + store, the todo store, and the file-memory provider, so it is held to the + same injectivity contract. Owner IDs that trip the independent traversal + guard are rejected outright rather than merged. + """ + store = MemoryFileStore(tmp_path, owner_state_key="owner_id") + roots: dict[str, Path] = {} + rejected: set[str] = set() + for owner_id in COLLIDING_IDENTIFIERS: + session = AgentSession(session_id="session-1") + session.state["owner_id"] = owner_id + try: + roots[owner_id] = store._get_memory_root(session, source_id=DEFAULT_MEMORY_SOURCE_ID) + except ValueError: + rejected.add(owner_id) + + assert rejected, "the traversal guard should still reject absolute and '..' owner IDs" + assert len(set(roots.values())) == len(roots), roots + assert len({str(root).lower() for root in roots.values()}) == len(roots), roots + assert len(roots) + len(rejected) == len(COLLIDING_IDENTIFIERS) + for root in roots.values(): + assert root.is_relative_to(tmp_path.resolve()) + + +def test_memory_file_store_encodes_non_ascii_owner_ids(tmp_path) -> None: + """NFC and NFD spellings of one word must not share a directory.""" + store = MemoryFileStore(tmp_path, owner_state_key="owner_id") + roots: list[Path] = [] + for owner_id in ("caf\u00e9", "cafe\u0301"): + session = AgentSession(session_id="session-1") + session.state["owner_id"] = owner_id + root = store._get_memory_root(session, source_id=DEFAULT_MEMORY_SOURCE_ID) + assert all(part.isascii() for part in root.relative_to(tmp_path.resolve()).parts) + roots.append(root) + + assert roots[0] != roots[1] + + +def test_memory_file_store_uses_literal_folders_for_safe_identifiers(tmp_path) -> None: + """Safe identifiers are readable on disk rather than opaque encoded segments.""" + store = MemoryFileStore(tmp_path, owner_prefix="user_", owner_state_key="owner_id") + session = AgentSession(session_id="session-1") + session.state["owner_id"] = "alice" + + root = store._get_memory_root(session, source_id="memory") + parts = root.relative_to(tmp_path.resolve()).parts + assert parts[0] == "memory" + assert parts[1] == "user_alice" + + +# endregion diff --git a/python/packages/core/tests/core/test_harness_todo.py b/python/packages/core/tests/core/test_harness_todo.py index 891ea7d94df..2b76c1b49ce 100644 --- a/python/packages/core/tests/core/test_harness_todo.py +++ b/python/packages/core/tests/core/test_harness_todo.py @@ -23,6 +23,8 @@ ) from agent_framework._harness._todo import TodoInput +from .test_filesystem import COLLIDING_IDENTIFIERS + def _tool_by_name(tools: list[object], name: str) -> object: """Return the tool with the requested name from a prepared tool list.""" @@ -377,3 +379,50 @@ def test_todo_harness_graduated_classes_are_not_experimental() -> None: assert TodoFileStore.__feature_id__ == ExperimentalFeature.HARNESS.value # type: ignore[attr-defined] # ty: ignore[unresolved-attribute] assert TodoFileStore.__doc__ is not None assert ".. warning:: Experimental" in TodoFileStore.__doc__ + + +def test_todo_file_store_derives_distinct_directories_for_colliding_owner_ids(tmp_path: Path) -> None: + """Owner IDs that a path normalizer would fold together stay separate. + + ``TodoFileStore`` shares the storage-key derivation with the session store, + the memory store, and the file-memory provider, so it is held to the same + injectivity contract. + """ + store = TodoFileStore(tmp_path, owner_state_key="owner_id") + paths: dict[str, Path] = {} + for owner_id in COLLIDING_IDENTIFIERS: + session = AgentSession(session_id="session-1") + session.state["owner_id"] = owner_id + paths[owner_id] = store._get_state_path(session, source_id="todo") # pyright: ignore[reportPrivateUsage] + + assert len(set(paths.values())) == len(COLLIDING_IDENTIFIERS), paths + assert len({str(path).lower() for path in paths.values()}) == len(COLLIDING_IDENTIFIERS), paths + for path in paths.values(): + assert path.is_relative_to(tmp_path.resolve()) + + +def test_todo_file_store_encodes_non_ascii_owner_ids(tmp_path: Path) -> None: + """Non-ASCII IDs are encoded, not used verbatim. + + A literal non-ASCII directory name is folded onto a single entry by macOS + APFS/HFS+, so the NFC and NFD spellings of one word would otherwise share a + directory despite being byte-distinct. + """ + store = TodoFileStore(tmp_path, owner_state_key="owner_id") + paths: list[Path] = [] + for owner_id in ("caf\u00e9", "cafe\u0301"): + session = AgentSession(session_id="session-1") + session.state["owner_id"] = owner_id + path = store._get_state_path(session, source_id="todo") # pyright: ignore[reportPrivateUsage] + assert path.parent.parent.name.isascii() + paths.append(path) + + assert paths[0] != paths[1] + + +def test_todo_file_store_encodes_windows_reserved_stems_with_an_extension(tmp_path: Path) -> None: + """``CON.txt`` still resolves to the reserved console device on Windows.""" + session = AgentSession(session_id="CON.txt") + store = TodoFileStore(tmp_path) + path = store._get_state_path(session, source_id="todo") # pyright: ignore[reportPrivateUsage] + assert path.parent.name.startswith("~todo-") diff --git a/python/packages/core/tests/core/test_sessions.py b/python/packages/core/tests/core/test_sessions.py index 9b789496a09..27028dc7315 100644 --- a/python/packages/core/tests/core/test_sessions.py +++ b/python/packages/core/tests/core/test_sessions.py @@ -48,6 +48,8 @@ from agent_framework._telemetry import FeatureIndex from agent_framework.exceptions import MiddlewareException +from .test_filesystem import COLLIDING_IDENTIFIERS + if TYPE_CHECKING: from agent_framework._agents import SupportsAgentRun @@ -1298,6 +1300,31 @@ async def test_reserved_windows_filename_is_encoded(self, tmp_path: Path, sessio assert session_file.name.startswith("~session-") assert session_file.is_file() + def test_colliding_session_ids_get_distinct_history_files(self, tmp_path: Path) -> None: + """Session IDs that a path normalizer would fold together stay separate. + + ``FileHistoryProvider`` shares the storage-key derivation with the todo + store, the memory store, and the file-memory provider, so it is held to + the same injectivity contract. + """ + provider = FileHistoryProvider(tmp_path) + paths = {session_id: provider._session_file_path(session_id) for session_id in COLLIDING_IDENTIFIERS} + + assert len(set(paths.values())) == len(COLLIDING_IDENTIFIERS), paths + assert len({str(path).lower() for path in paths.values()}) == len(COLLIDING_IDENTIFIERS), paths + for path in paths.values(): + assert path.parent == tmp_path.resolve() + + def test_non_ascii_session_ids_are_encoded(self, tmp_path: Path) -> None: + """NFC and NFD spellings of one word must not share a history file.""" + provider = FileHistoryProvider(tmp_path) + nfc = provider._session_file_path("caf\u00e9") + nfd = provider._session_file_path("cafe\u0301") + + assert nfc != nfd + assert nfc.name.isascii() + assert nfd.name.isascii() + # --------------------------------------------------------------------------- # InMemoryHistoryProvider tests diff --git a/python/packages/core/tests/core/test_skills.py b/python/packages/core/tests/core/test_skills.py index b1e98c8a708..237b723841b 100644 --- a/python/packages/core/tests/core/test_skills.py +++ b/python/packages/core/tests/core/test_skills.py @@ -1129,7 +1129,7 @@ def test_entry_that_cannot_be_inspected_is_skipped(self, tmp_path: Path) -> None def _raise(path: Path) -> bool: raise OSError("cannot inspect") - with patch("agent_framework._skills.is_link_or_reparse_point", side_effect=_raise): + with patch("agent_framework._skills._is_link_or_reparse_point", side_effect=_raise): assert FileSkillsSource._discover_skill_directories([str(root)]) == [] def test_skill_file_that_cannot_be_inspected_is_skipped(self, tmp_path: Path) -> None: @@ -1142,7 +1142,7 @@ def _raise_for_skill_file(path: Path) -> bool: raise OSError("cannot inspect") return False - with patch("agent_framework._skills.is_link_or_reparse_point", side_effect=_raise_for_skill_file): + with patch("agent_framework._skills._is_link_or_reparse_point", side_effect=_raise_for_skill_file): assert FileSkillsSource._discover_skill_directories([str(root)]) == [] @@ -2035,7 +2035,7 @@ def test_fails_closed_when_path_cannot_be_inspected(self, tmp_path: Path, monkey def fail_probe(path: Path) -> bool: raise PermissionError(path) - monkeypatch.setattr("agent_framework._skills.is_link_or_reparse_point", fail_probe) + monkeypatch.setattr("agent_framework._skills._is_link_or_reparse_point", fail_probe) assert FileSkillsSource._has_link_or_reparse_point_in_path(str(target), str(tmp_path)) is True diff --git a/python/packages/core/tests/workflow/test_agent_executor.py b/python/packages/core/tests/workflow/test_agent_executor.py index 2cc2ed2ce6e..4b067ab7c1c 100644 --- a/python/packages/core/tests/workflow/test_agent_executor.py +++ b/python/packages/core/tests/workflow/test_agent_executor.py @@ -1,7 +1,6 @@ # Copyright (c) Microsoft. All rights reserved. import pickle - from collections.abc import AsyncIterable, Awaitable from typing import Any, Literal, overload diff --git a/python/packages/gemini/tests/test_gemini_client.py b/python/packages/gemini/tests/test_gemini_client.py index f9f3f774d3d..a2b4908d7ad 100644 --- a/python/packages/gemini/tests/test_gemini_client.py +++ b/python/packages/gemini/tests/test_gemini_client.py @@ -405,9 +405,7 @@ async def test_get_response_returns_text() -> None: (RuntimeError("connection reset"), ChatClientException), ], ) -async def test_get_response_wraps_sdk_errors( - sdk_exception: Exception, expected_exception: type[Exception] -) -> None: +async def test_get_response_wraps_sdk_errors(sdk_exception: Exception, expected_exception: type[Exception]) -> None: """Non-streaming get_response must translate raw google-genai SDK errors into the framework's ChatClientException hierarchy, matching every other provider (OpenAI, Anthropic, Mistral, Ollama, Bedrock).""" diff --git a/python/samples/02-agents/context_providers/file_memory_provider.py b/python/samples/02-agents/context_providers/file_memory_provider.py index 776dd5f24aa..7574fb72c63 100644 --- a/python/samples/02-agents/context_providers/file_memory_provider.py +++ b/python/samples/02-agents/context_providers/file_memory_provider.py @@ -38,6 +38,16 @@ session for that user shares — which is what lets the second conversation below recall what the user said in the first. + A scope is an opaque namespace key, not a path. It is mapped to exactly one + folder, and two byte-distinct scopes do not share a working folder, so a + value containing separators becomes a single encoded folder rather than a + nested directory. Prefer a flat, canonical, lowercase scope such as + ``f"user-{USER_ID}"``; values outside the safe lowercase set are encoded, so + the folder name stays unique but is no longer readable. If the scope comes + from an external request, keep authorizing it in your own application code — + the storage mapping is a storage-layer guarantee, not an authorization + check. + Prerequisites: - ``FOUNDRY_PROJECT_ENDPOINT``: Your Microsoft Foundry project endpoint. - ``FOUNDRY_MODEL``: Chat model deployment name. @@ -45,8 +55,9 @@ """ # The id of the user we are storing memories for. It is used below as the -# provider's scope so that each user gets their own memory folder. -USER_ID = "UID1" +# provider's scope so that each user gets their own memory folder. Keep it +# lowercase so the folder name stays readable rather than encoded. +USER_ID = "uid1" async def main() -> None: @@ -71,7 +82,10 @@ async def main() -> None: # conversation further down to recall what the user said in the first. # - Omitting ``scope`` (the default) isolates memories to a single session # (the working folder is derived from the session id). - file_memory_provider = FileMemoryProvider(store, scope=f"users/{USER_ID}") + # Keep the scope a flat, canonical, lowercase value: it is an opaque key + # mapped onto exactly one folder, not a path that expands into + # subdirectories. + file_memory_provider = FileMemoryProvider(store, scope=f"user-{USER_ID}") # 3. Attach the provider to the agent so it gets the file_memory_* tools. agent = Agent( @@ -85,7 +99,7 @@ async def main() -> None: ) # - working_folder = memory_root / "users" / USER_ID + working_folder = memory_root / f"user-{USER_ID}" print(f"Memory files will be written to: {working_folder}\n") # 4. First conversation: tell the agent something worth remembering. The @@ -129,7 +143,7 @@ async def main() -> None: """ Sample output (abridged; exact text varies by model): -Memory files will be written to: .../context_providers/agent-file-memory/users/UID1 +Memory files will be written to: .../context_providers/agent-file-memory/user-uid1 === First conversation === Got it — I'll remember that you're vegetarian and always travel with your dog. I've saved