Repository navigation
Python: fix(core): prevent path collisions between files and directories in InMemoryAgentFileStore - #8687
Conversation
…ies in InMemoryAgentFileStore Signed-off-by: Manohar Paturi <186662190+ManoharPaturi@users.noreply.github.com>
GitHub Actions
Active
GitHub Actions
Active
agent-framework-automation
Bot
added
the
python
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
MAF Automated Review — Iteration 1
Result: Findings reported
Scope: full PR (1 commit(s)): 05f1554d6486
Model: gpt-5.6-sol-fast
Overview
The PR adds lock-protected, case-insensitive ancestor and descendant checks, with focused regression tests showing rejected writes leave existing content intact. The collision invariant remains incomplete because successful explicit directory creation does not reserve a path, and the new validation loops can synchronously block the event loop based on either total store size or path depth. These issues affect backend parity and shared-store availability.
Reviewed the supplied pull-request change set across correctness, security/reliability, architecture, and failure behavior.
3 verified findings remained after source verification (3 medium) across 1 file. Details are attached to the affected lines below.
Affected areas: python/packages/core/agent_framework/_harness/_file_access.py
| """No-op: directories are implicit from file paths in the in-memory store.""" | ||
| del path | ||
| """Ensure the directory at ``path`` exists.""" | ||
| display = _normalize_relative_path(path, is_directory=True) |
There was a problem hiding this comment.
create_directory() only validates path; it never records the directory. After await create_directory("Reports"), write("Reports", ...) still succeeds and the in-memory backend reports a file where FileSystemAgentFileStore raises IsADirectoryError, so successful empty/scoped directory creation does not satisfy the AgentFileStore contract. Please persist explicit directories and include them in write collision checks and directory listings so this call reserves the path.
There was a problem hiding this comment.
Addressed in 9b05a0f. Explicit directories and their intermediate parents are now recorded in _explicit_dirs, included in directory listings, and reject subsequent file writes with IsADirectoryError.
| ancestor = "/".join(parts[:i]) | ||
| if ancestor in self._files: | ||
| raise NotADirectoryError(f"Not a directory: {path!r}") | ||
| for existing_key in self._files: |
There was a problem hiding this comment.
This scans every stored file for every write. With no entry limit or automatic cleanup, 20,000 flat writes took about 5.9 seconds in a local reproduction, and later writes continue synchronously scanning shared state under _lock, so model-driven writes can increasingly stall other sessions using the store. Please maintain indexed directory-prefix metadata so collision detection is bounded by path depth rather than total store size.
There was a problem hiding this comment.
Addressed in 9b05a0f. Added an indexed reference counter _dir_counts that increments on file creation and decrements on deletion across ancestor keys, making collision detection O(depth) rather than O(total files).
| async with self._lock: | ||
| parts = key.split("/") | ||
| for i in range(1, len(parts)): | ||
| ancestor = "/".join(parts[:i]) |
There was a problem hiding this comment.
Building every ancestor with "/".join(parts[:i]) makes a single write quadratic in path depth and runs synchronously on the event loop. A 40,000-segment path accepted by the current normalizer blocked for about 7.5 seconds in a local reproduction, and neither the schema nor normalizer limits path length or depth. Please bound path complexity or build and check ancestors in linear time so one write cannot monopolize the event loop.
There was a problem hiding this comment.
Addressed in 9b05a0f. Replaced repeated slicing and joins with _ancestor_keys, which walks slash indices in linear time relative to path length.
…AgentFileStore Signed-off-by: Manohar Paturi <186662190+ManoharPaturi@users.noreply.github.com>
GitHub Actions
Active
GitHub Actions
Active
GitHub Actions
Active
The upstream microsoft#8507 assertion expected list_children("") to stay empty, which held while create_directory was a no-op. This PR makes explicit directories listable, so the session-1 directory created during before_run now appears at the root. The invariant the test protects, that a rejected nested write leaves nothing behind, is preserved: the session directory exists and stays empty, with no notes child.
GitHub Actions
Active
|
CI note: the 3.10/3.14 ubuntu failures came from upstream #8507 landing test_tools_reject_nested_paths, which asserted list_children("") stays empty. that held while create_directory was a no-op; this PR makes explicit directories listable, so the session-1 directory created during before_run now shows up at the root. the invariant the test protects is unchanged, a rejected nested write leaves nothing behind. updated the assertion in 92785b5 to pin the session dir as the only root entry and that it stays empty. both file harness suites pass locally, fresh matrix running. |
|
/review |
GitHub Actions
Active
There was a problem hiding this comment.
MAF Automated Review — Iteration 2
Result: Findings reported
Scope: 106 net-new commit(s): 76ccf3c44cee, a638ce136ed6, 0bbb7245d408, 834eb7ff12c8, 30b9b8e06766, 622737258c73, 7fb41d673c97, 4f4e72284c9c, 2055e666e1ac, 62337887116c, b20927b921d4, 2191c60c5e7d, 717557a3791a, 91fd47035684, f83a892f9f0e, 0843a9c0537b, e2c6df28daa9, 72272613d0c0, 5ee3e87d6768, 8ff549d3f721, 024dd9908bc1, 2c46deb91e70, 9b05a0f91616, f9310e5ac2a1, 6f1522a50b66, 7c47fc87d162, 273e63508fe5, 7be69facedc3, 11c0dd7ccfdd, 109234952ad4, 218700d4e94a, 2a8daa077b74, aab258745c6d, 93cf98a042c0, a38414391c1b, 9bca1ba4a800, 9acaf82b61a9, ed745e5e81de, fd932dcb8ec0, cc89fe677348, 529564c5b24e, 789d514d9cbf, 153aaa1e0416, 11c45ce72ec1, 6bb47d4fa7c6, ce4fcb2665c3, c91cf3234101, 16aa03b33a46, 490e01947e9a, 20c404524b2c, e9d9962bfc1f, 08eb7d94e9af, c804f32c983d, 238d7e2e9bb9, 3f1f3b50b57c, 3efb73a856a0, 1370903bd8e6, 37ce872a85fe, 2024d4df4c4d, 0d6d2bb6b0b5, be01deccaf3d, 3b27cd33800c, f47c3782376f, 95e711a6280d, b86cbe4dbe21, 8e6bf37c6d26, 01cf5b547c5f, b000c15d3966, 5a2b135ddb2b, 06e9b48937d5, 7a98aa9f3c21, 763887452f37, 202e85d0d876, 77f832ff1ac9, 0911ad636de4, f160d050937b, ba320332d496, 987b3459ff61, 1a2b4fb838c0, 8258db8cf08c, 30aee9fc8da4, e7d99c189678, 8220ae265631, e3a04fecbb20, e626cef6c953, 71e84a098dec, d4157d809a31, f8f15f28db6a, 534334731279, e23f3c89f29e, c8126a0cedb7, a708babd7c74, 142a1c01bf4e, 3a837e327226, 2d995049b9b0, 908ca8c1a868, 52ff4d28011a, 1a03705ae453, 4b1c231b4fa4, 2a7470a92c9b, 3d39258d72d2, 4b0e3d8e84c2, 2f5713f60e6e, 612858c7170c, 2b6c5cbe1992, 92785b5d7572
Model: gpt-5.6-sol
Overview
The change replaces full-store collision scans with ancestor indexes, persists explicitly created directories, and adds focused collision and provider regression coverage; normalization and a shared lock keep each mutation atomic. Explicit directories now satisfy the reservation contract, but session-scoped providers can allocate them automatically with no release, expiry, or quota. On a shared in-memory store, that leaves a persistent process-wide resource-exhaustion path.
Reviewed the supplied incremental change set across correctness, security/reliability, architecture, and failure behavior.
1 verified finding remained after source verification (1 high) across 1 file. Details are attached to the affected lines below.
Affected areas: python/packages/core/agent_framework/_harness/_file_access.py
| ancestor_display = display | ||
| if ancestor not in self._explicit_dirs: | ||
| self._explicit_dirs[ancestor] = ancestor_display | ||
| self._explicit_dirs[key] = display |
There was a problem hiding this comment.
Each new session-scoped run can reach this assignment with a unique session ID, but delete() cannot remove explicit directories and there is no expiry or quota. Because list_children() copies and scans this shared map, an untrusted caller can permanently slow unrelated sessions and eventually exhaust process memory. Please add lifecycle cleanup/expiry or bounded tenant-local retention for these entries.
Explicitly created directories had no deletion path, so session-scoped runs kept accumulating entries in the shared map. delete() now removes an explicit directory once nothing is stored under it and no other explicit directory is nested below, giving session directories a lifecycle. Non-empty directories stay put, matching the filesystem store's file-only deletes. Signed-off-by: Manohar Paturi <186662190+ManoharPaturi@users.noreply.github.com>
GitHub Actions
Active
|
Pushed b4bf58e: delete() now removes an explicitly created directory once it is empty (nothing stored under it, no nested explicit directory), so session-scoped directories get a lifecycle instead of living in the map forever. Non-empty directories are refused, which keeps parity with the filesystem store's file-only deletes. A quota or TTL on top of that felt like a policy call I shouldn't pick alone, happy to take direction if you want one. |
|
/review |
There was a problem hiding this comment.
MAF Automated Review — Iteration 3
Result: Findings reported
Scope: 167 net-new commit(s): 853c4564476a, 7634356a1de0, d5b2de2ed66d, c3bb759b3dd7, 06f6e5f21636, 577caa2f0de1, e15c6dce2d10, 8b4713ed5c0f, 4ce5ae3f2812, 2f84b1410eb0, aa6c73cce721, cb63c2abc42e, 387d5c7bc389, a850ee6c221a, f1fb145c4d80, 99215f96dafa, d52db94e4b61, a15c0197b282, 2e8b58c06da2, 501989469265, 6d8e16ea3641, f8df81a2a84e, 68e58bf8e79c, a91fe6682f16, ef9084c8d947, 098fca4dc4b7, bbb639effc0f, e9cb7fe68707, 8e2fcbf348f3, 3e1289d180ff, 30055e4d36eb, a2f4506c0ba3, 6967986f8e4a, ffd85a2caedf, 84d135379442, cb77f68f005f, 6e681cc1e4d2, e9857bd75a21, 735050553822, 3c2fe0a8a365, d7ef9e240836, 90615858cbf0, de3a3b08df5b, cdd339b0a541, 4dfb7565b67e, 301a43c862be, 2606b1a0498a, 5bba4cec7380, ccf44d95988b, 52fed574f05a, 6ef0b73ed8ee, 2df61296bb56, 636e8562d1db, bd5ab48cfdeb, b9d24c8fb484, e63ca8ac7d9a, ac5ed2f49ee9, 23821dea4ea7, 6d896b843289, 1b93be1a2d66, 1aca61d51bb9, 41c4f1c9b024, b306dde15cd1, 0774607d8d3f, 73a96cb1a902, 9251bc9911d3, 41e785fb9a3c, 279d97f75cb2, 847665f853a1, d0ccaa62761e, 245d4284a501, fb1be94a9ace, 15cda5ca9c90, d875bf08ae4e, df4f41d1588e, 866253d87f0c, 8254704d57dc, 5f2294175235, 45a92e6a1e8c, 08f9136d0bff, dcd74e0a54ed, 1e722004da32, 13150433d155, af1995301a47, dd1488236f3a, 40763bed1a5e, 3d0bde5736a3, 76b3cf7c0668, 919e62421540, 1cfd2c59f54a, 26fa33f9ec46, 9ca28e85fd80, 93dc1abc694e, c7ffabe0f2aa, 7a262d6a7236, 917e0c28dac2, a35f8c48e295, 9176e176addd, 4de99bc2a30a, ca936db37708, 6e630f4796b8, 91ab44faa482, 2d9cc3f8ae46, 4e9984295050, 37b606a0b5f7, 513ec8f9afe8, d53cdffab41c, 3e6db916fb8f, 9c622e69c358, 723d8caf87eb, 477ecc67231f, 9f21f1a56b82, 708b1aba787e, 65a12f9bc826, b2ee384737e0, 524f79c15948, c2c8d36496f6, c1c288b64eef, f72bf952415a, fd52de715791, 4d0da3cacb0c, cbe008ebcc8b, 4dc30df5ced2, 81836614649a, 50c612bb37bc, cc5010c7e1c3, 967d2c6c3a91, 41579d2fb235, 60bc6ef5a5d3, 5a131d721ef0, f64e4c02c562, c809095b7f3b, 34e980a4773b, 24b7550bfd32, ce1c3b7a0f16, a72eb657887d, f9480ea78009, 2c33eba9861b, 9d4672f7a12a, fc24e6f6e433, e7ca6c21a901, a75e803ef6ec, 2d58876e49d7, 726dcff81e93, b53d25ed1cd2, 538da719e86c, 2e1505683a7d, 691b115e9e02, 457487288767, ce83eab7a300, 129eeb8ef64e, d57f9e8ac632, cc81a8e72a0b, 03c0f322e0ea, 7d09721562c2, f76d0a3de369, a70a727eec6a, a57b37da903c, 2b074fa8ddae, dc649e5980d4, 4a90089f3cdf, ec47b901107f, b5e552112f0a, dcbbf7967afe, 1b9879c58ac9, 2002bfe645d2, b4bf58ee38a5
Model: gpt-5.6-sol
Overview
The review found 1 verified inline finding(s).
Reviewed the supplied incremental change set across correctness, security/reliability, architecture, and failure behavior.
1 verified finding remained after source verification (1 medium) across 1 file. Details are attached to the affected lines below.
Affected areas: python/packages/core/agent_framework/_harness/_file_access.py
There was a problem hiding this comment.
Deleting an empty explicit directory returns True in _file_access.py:1138-1145, despite the file-only contract at :798-805 and filesystem behavior at :1516-1524; backend substitution is therefore observable, while any fix must preserve session-scoped directory cleanup.
Motivation & Context
In
InMemoryAgentFileStore, writing to a path whose ancestor is already an existing file succeeded, as did writing to a path that already has descendant files in the store. This allowedReportsandReports/q1.txtto coexist simultaneously, causinglist_childrento surface both a directory and a file under the exact same name. Real filesystems reject these operations withNotADirectoryErrorandIsADirectoryError.This change aligns
InMemoryAgentFileStorewithFileSystemAgentFileStoreby enforcing directory and file path invariants duringwriteandcreate_directory.Description & Review Guide
write, verified that no ancestor path of the target is an existing file inself._files, raisingNotADirectoryErrorif encountered.write, checked whether the target path is already an existing directory with descendants, raisingIsADirectoryErrorif so.create_directory, ensured that neither the target path nor any ancestor is an existing file, raisingFileExistsErrorif a collision is detected.test_harness_file_access.pycovering both collision orders and confirming existing file contents remain intact.Related Issue
Fixes #8567
Contribution Checklist
breaking changelabel (or add "[BREAKING]" to the title prefix, before or after any language prefix) — a workflow keeps the label and title prefix in sync automatically.