Repository navigation
Python: [Bug]: InMemoryAgentFileStore permits a path to be both a file and a directory #8567
Description
Description
InMemoryAgentFileStore permits both Reports and Reports/q1.txt to exist. Consequently, list_children("") returns two entries named Reports, one reported as a file and one as a directory. A caller cannot tell which path to use, and this state cannot occur in FileSystemAgentFileStore.
This happens in either write order. write() checks only whether the exact file key exists; it does not reject a file ancestor or an existing descendant. Its case-insensitive key normalization means Reports and reports/q1.txt conflict as well.
Expected: Reject the second write without changing the existing content. A file path cannot also serve as a directory, regardless of casing.
Actual: Both writes succeed and the root listing contains a same-named file and directory.
Code Sample
import asyncio
from agent_framework import InMemoryAgentFileStore
async def main() -> None:
store = InMemoryAgentFileStore()
await store.write("Reports", "file")
await store.write("Reports/q1.txt", "nested")
print([(entry.name, entry.type) for entry in await store.list_children()])
# [('Reports', 'directory'), ('Reports', 'file')]
asyncio.run(main())The new regression test fails against the unmodified package (Failed: DID NOT RAISE NotADirectoryError) and passes against the local fix.
Error Messages / Stack Traces
None. The inconsistent state is accepted silently.
Package Versions
Source checkout at 999dda7970fe969c0901d524365ce34ecbda6227 (agent-framework-core version 1.18.0); the relevant write() implementation was also checked against the repository's current default branch on 2026-09-21. The public API reproduction and regression tests used this source checkout through an existing virtual environment, not a separately installed release package.
Python Version
Python 3.12.0 for the isolated source execution.
Additional Context
FileSystemAgentFileStore uses real filesystem paths, where a file cannot be the parent of another file and a nonempty directory cannot be replaced by a file. The in-memory store is used for tests and lightweight use, so matching this basic path invariant avoids testing a state the disk store cannot represent.
Local draft fix: reject a write if any ancestor is already a file or if the target path already has descendants. A focused regression test covers both write orders and verifies the existing content remains intact. The new test fails on the baseline and passes with the fix. The full test_harness_file_access.py file passes with the fix: 73 passed, 6 skipped. Ruff check, Ruff format check, py_compile, and git diff --check also pass. The reused virtual environment lacks pytest-timeout, causing an unrelated pytest configuration warning.
Searches of repository issues and PRs for InMemoryAgentFileStore conflict, file and directory same name, and file store path collision found no matching report or implementation on 2026-09-21.
Metadata
Metadata
Assignees
Labels
Type
Projects
- StatusShow more project fieldsDone
Activity
github-actions commentedon Sep 21, 2026
🤖 Automated triage reproduction notes (agent-authored — trust but verify)
Agent analysis
Repro:
python/packages/core/agent_framework/_harness/_file_access.py::InMemoryAgentFileStore.writeat lines 1046-1058 accepts a target whose normalized case-insensitive path has either an existing file ancestor or existing descendants. WritingReportsandReports/q1.txtin either order triggers the bug, after whichlist_children()can returnReportsas both a directory and file. Minimal repro: instantiate the store, write either path, then assert the second write raisesNotADirectoryError; both parameterized orders currently fail withDID NOT RAISE.python/packages/core/tests/core/test_harness_file_access.py::test_in_memory_store_rejects_file_directory_path_collisions1.18.01.19.0manjunathshiva commentedon Sep 21, 2026
Reproduced on
main(53e98134c) — writingReportsand thenReports/q1.txtboth succeed, andlist_children("")comes back with two entries namedReports, onedirectoryand onefile:Two things worth settling before anyone writes the fix, though.
Is this intended? The class docstring says the quiet part out loud — "Directory concepts are simulated using path prefixes — no explicit directory structure is maintained" (
_file_access.py:1027). The .NETInMemoryAgentFileStorecarries that same sentence, and itsWriteAsyncis a barethis._files[path] = content;over anOrdinalIgnoreCasedictionary, so it behaves identically.create_directoryis also a no-op here, so the store already cannot represent an empty directory. That reads like a deliberate simplification rather than an oversight, and fixing only Python would split the two implementations apart.If it should be enforced, the exception needs deciding, and
NotADirectoryErrorwould not matchFileSystemAgentFileStore. I measured the disk store rather than assuming, on Windows:Reports, thenReports/q1.txtFileExistsError(WinError 183), fromparent.mkdir(exist_ok=True)Reports/q1.txt, thenReportsPermissionError(Errno 13)So the disk store raises neither
NotADirectoryErrornor a consistent type — it differs by write order, and the second case is platform-dependent, so "match the disk store" is not actually a well-defined target.FileExistsErrorwould also be an actively bad choice:file_access_writealready catches it (_file_access.py:1919) and tells the model "File 'X' already exists. To replace it, write again with overwrite set to true." — advice that would not work, sinceoverwrite=Truedoes not resolve a file/directory collision.So: westey (@westey-m), is the collision an accepted consequence of the documented prefix simulation, or should the in-memory store enforce the invariant? If the latter, which exception, and should the .NET store change in the same PR to keep the two aligned?
Happy to implement whichever way you decide — an ancestor-is-a-file check plus a descendant-prefix scan in
write(), with tests covering both write orders and the case-insensitive variant. 林黛玉倒拔垂杨柳 (@boshilin123) nice find; the duplicate-name output fromlist_childrenis the part that makes it more than cosmetic.westey-m commentedon Sep 21, 2026
Manjunath Janardhan (@manjunathshiva), thanks for the offer to implement. I think we should just stick to matching what a regular file system does, which is not allowing both a file and a directory with the same name to exist. The abstraction doesn't currently define a clear protocol around what exceptions should be raised for these cases, but we really should add that, and ensure that each implementation matches the common exception types.
manjunathshiva commentedon Sep 22, 2026
Thanks — matching a regular file system is a clear answer, and it settles the part I could not decide. Four things I ran into while scoping it, because the exception protocol turns out to reach further than
InMemoryAgentFileStore.1. Proposed mapping.
FileExistsErroris already taken by a different contract —AgentFileStore.writedocuments that an implementation "must perform an atomic exclusive create and raiseFileExistsError" whenoverwrite=False. Reusing it for a collision would put two unrelated meanings on one type, so:Reportsis a file, writeReports/q1.txt)NotADirectoryError(ENOTDIR)IOExceptionReports/q1.txtexists, writeReports)IsADirectoryError(EISDIR)IOExceptionPython has distinct classes for both errnos; .NET has no equivalents, so "common exception types" can only mean equivalent semantics with language-idiomatic types, not literally the same type. If you would rather .NET carried distinct types too, that is a new-public-type decision and I would want your call before adding any.
2. Case sensitivity is not determined by "match a regular file system", and it changes the behaviour. Both in-memory stores are case-insensitive — Python lowercases keys, .NET uses
StringComparer.OrdinalIgnoreCase. A POSIX file system is case-sensitive. SoReportsplusreports/q1.txtcollides in the in-memory stores and on Windows, but is legal on Linux. Should collision detection follow the store's existing case-insensitive keys, or the case sensitivity of the underlying platform forFileSystemAgentFileStore? The second makes the two implementations disagree by platform, which I assume is not what you want, but it is the more literal reading of "what a regular file system does".3. The filesystem stores need work too, which I do not think is obvious from the issue.
FileSystemAgentFileStoreonly translatesOSErrorfor path validation (symlink and escape checks, re-raised asValueError); write-time OS errors pass straight through. Measured on Windows:ReportsthenReports/q1.txt→FileExistsError(WinError 183), out of thefull_path.parent.mkdir(parents=True, exist_ok=True)at_file_access.py:1319Reports/q1.txtthenReports→PermissionError(Errno 13); POSIX would differSo "ensure each implementation matches the common exception types" means normalising OS errors in the filesystem stores as well — four implementations across the two languages, plus conformance coverage. Worth noting the .NET abstraction currently documents no exceptions at all (
AgentFileStorehas zero<exception>tags) andAgentFileStoreContractTestsasserts onlyArgumentException,ArgumentNullExceptionandOperationCanceledException, so .NET would be gaining its first documented exception contract here.4. The tool-level handler has to change in the same PR, or this becomes a worse bug than it fixes.
FileAccessProvider.file_access_writecatches specific types and returns a string to the model. A newNotADirectoryErrorwould not be caught, so it would escape and fail the whole agent run — where today the model just sees a confusing listing. It also currently catchesFileExistsErrorand answers "write again with overwrite set to true", which cannot resolve a file/directory collision. So the handler needs to catch the new types and return something actionable, e.g. that the path is a directory and a different name is needed.Shape. Two PRs rather than one, matching how the harness changes have gone before (#6474/#6476, #6801/#6807, #7669/#7671) and because commit titles here carry a language prefix: Python first, defining the protocol on the abstraction with the
Raises:/"must" wording already used for theoverwrite=Falsecase, then .NET mirroring it with contract tests. Happy to do both.Two smaller scoping questions:
create_directoryis a no-op in the in-memory store ("directories are implicit from file paths"), so it cannot create a collision but also cannot reserve a name — in scope or not? And the descendant check is an O(n) key scan per write, which seems fine for a test-oriented store but I would rather say so than have it found in review.Also flagging that this is an intentional break of documented behaviour: the permissive collision is currently described as deliberate in both languages, in the same sentence — "Directory concepts are simulated using path prefixes — no explicit directory structure is maintained" — so those docstrings change too. Every previous paired FileAccess change in this area shipped as
[BREAKING](#6474/#6476, #6801/#6807, #7669/#7671), so I would expect these to carry the label as well; tell me if you would rather they did not.lakers-abing commentedon Sep 23, 2026
Hi 林黛玉倒拔垂杨柳 (@boshilin123), I saw the maintainer discussion and your note that you're happy to handle the Python and .NET work. I have a separate Python implementation with regression tests for both write orders and case-insensitive collisions. Are you planning to submit the Python-side PR yourself, or would you be comfortable with me opening a separate PR for that part? I want to avoid duplicating your work.
manjunathshiva commentedon Sep 23, 2026
lakers-abing that offer to handle the Python and .NET sides was mine rather than 林黛玉倒拔垂杨柳 (@boshilin123)'s — easy to miss in a long thread. Either way, please go ahead with the Python PR; you have code and tests already and I have not written a line, so there is nothing to duplicate.
Three things from scoping it that may save you a review cycle, since they are the parts that bite outside the in-memory store.
The tool-level handler needs changing in the same PR, or the fix is a regression.
FileAccessProvider.file_access_writecatches specific exception types and returns a string to the model (_file_access.py:1919). A newNotADirectoryErrororIsADirectoryErrorwould not be caught there, so it escapes and fails the whole agent run — where today the model just gets a confusing listing. That handler also currently answersFileExistsErrorwith "write again with overwrite set to true", which cannot resolve a file/directory collision, so it needs a branch that tells the model the path is a directory and to choose another name.FileExistsErroris already taken.AgentFileStore.writedocuments that an implementation "must perform an atomic exclusive create and raiseFileExistsError" whenoverwrite=False, so reusing it for a collision would put two unrelated meanings on one type.Case sensitivity is not settled, and your tests encode an answer to it. Both in-memory stores are case-insensitive — Python lowercases the key, .NET uses
StringComparer.OrdinalIgnoreCase— while a POSIX file system is case-sensitive. SoReportsplusreports/q1.txtcollides in the in-memory store and on Windows but is legal on Linux. westey (@westey-m) said "match a regular file system", which does not by itself decide this, and I asked him about it but have not had an answer yet. Worth pinning before your tests bake it in.One scope note in case it affects how you frame the PR:
FileSystemAgentFileStoreonly translatesOSErrorfor path validation, so write-time OS errors pass straight through. I measured it on Windows —ReportsthenReports/q1.txtraisesFileExistsError(WinError 183) out of theparent.mkdirat_file_access.py:1319, and the reverse order raisesPermissionError(Errno 13). So "each implementation matches the common exception types" reaches the filesystem store too, not just the in-memory one. Whether that belongs in your PR or a follow-up is worth asking westey (@westey-m).Happy to review when you open it.
ManoharPaturi commentedon Oct 22, 2026
The fix for this issue is resubmitted as #9608 (replacing the closed #8687, which was closed with a request to address the outstanding review feedback about preserving the file-only delete contract). The new PR contains the original path-collision fix plus that feedback:
delete()is now strictly file-only, and a newdelete_directory()method releases empty explicitly created directories so session-scoped cleanup still works.