Repository navigation
fix(memory-tool): compare resolved path rather than raw string in delete root guard - #1957
agarwal-tanmay-work wants to merge 2 commits into
Conversation
chrikrah
left a comment
There was a problem hiding this comment.
@agarwal-tanmay-work on 4421d56 the bare /memories raises, while /memories/, /memories/. and /memories/./ each delete the store and the files under it. On a822890 all four raise. Approve.
non-blocking: /memories/sub/.. is a live bypass on the base too, and a822890 already catches it, but the parametrize list stops at /memories/.. #1906 parametrizes /memories/subdir/.. already, so that entry is worth copying here only if this branch lands first.
#1906 by @Kayvan-Zahiri changes the same two guards and the rename guard as well, and #1914 changes the /memories prefix check in the same file.
Verification
Two clean exports, a822890 and the same tree with src/anthropic/lib/tools/_beta_builtin_memory_tool.py rolled back to 4421d56. uv sync --all-extras in each, anthropic.__file__ asserted inside the export.
$ uv run pytest tests/lib/tools/memory_tools -q -p no:randomly
73 passed in 6.84s
# rolled-back source, the new tests kept
$ uv run pytest tests/lib/tools/memory_tools -q -p no:randomly
4 failed, 69 passed in 6.32s
E Failed: DID NOT RAISE <class 'anthropic.lib.tools._beta_functions.ToolError'>
# BetaLocalFilesystemMemoryTool.delete over six spellings, rolled-back source
$ uv run python probe.py
'/memories' -> ToolError; memories/ exists=True
'/memories/' -> no error; memories/ exists=False; keep.txt exists=False
'/memories/.' -> no error; memories/ exists=False; keep.txt exists=False
'/memories/sub/..' -> no error; memories/ exists=False; keep.txt exists=False
'/memories/./' -> no error; memories/ exists=False; keep.txt exists=False
'//memories' -> ToolError 'Path must start with /memories'; memories/ exists=True
# same probe on a822890
$ uv run python probe.py
'/memories' -> ToolError Cannot delete the /memories directory itself; memories/ exists=True; keep.txt exists=True
'/memories/' -> ToolError Cannot delete the /memories directory itself; memories/ exists=True; keep.txt exists=True
'/memories/.' -> ToolError Cannot delete the /memories directory itself; memories/ exists=True; keep.txt exists=True
'/memories/sub/..' -> ToolError Cannot delete the /memories directory itself; memories/ exists=True; keep.txt exists=True
'/memories/./' -> ToolError Cannot delete the /memories directory itself; memories/ exists=True; keep.txt exists=True
'//memories' -> ToolError Path must start with /memories, got: //memories; memories/ exists=True; keep.txt exists=True
# not run: anything outside tests/lib/tools, and any run on Windows
@dtmeadows-ant two open branches fix this delete guard, this one and #1906, and only you can say which should carry it. Which do you want @agarwal-tanmay-work and @Kayvan-Zahiri to build on?
|
Thanks @chrikrah for the thorough verification and approval! I've pushed an update adding both @dtmeadows-ant — happy to defer to your preference here. If you prefer to land this focused fix on the delete guard, it's ready. If you'd rather consolidate or have this build on/alongside @Kayvan-Zahiri's #1906 (or add the rename guard here as well), just let me know and I'm glad to adjust. |
chrikrah
left a comment
There was a problem hiding this comment.
@agarwal-tanmay-work still an approve at be88869. The two spellings you added fail on the old guard, so all four that got through on 4421d56 are now pinned, sync and async.
# Python 3.12.3, pytest 9.1.1, tree from git archive be88869
$ PYTHONPATH=$HEAD/src python -m pytest tests/lib/tools/memory_tools -q -p no:randomly \
-o addopts="--tb=line -p tests._alias_httpx"
77 passed in 4.07s
# _beta_builtin_memory_tool.py from 4421d56, tests kept
8 failed, 69 passed in 3.83s
E Failed: DID NOT RAISE ToolError
FAILED ...TestBetaLocalFilesystemMemoryTool::test_delete_not_allow_deleting_memories_directory[/memories/./]
FAILED ...TestBetaLocalFilesystemMemoryTool::test_delete_not_allow_deleting_memories_directory[/memories/sub/..]
# the other six: /memories/ and /memories/. in this class, all four in the async class
Nothing more from me on this branch. If the guard moves again once the #1906 question is settled, I will rerun these eight cases on the new head.
Summary
In
BetaLocalFilesystemMemoryToolandBetaAsyncLocalFilesystemMemoryTool, thedelete()method prevents deletion of the root directory using an exact string check (if command.path == "/memories":).Because
_validate_pathresolves trailing slash and relative dot variations (such as/memories/or/memories/.) toself.memory_root, these variants bypass the raw string check and cause the root memory directory to be deleted viashutil.rmtree().Changes
delete()to checkif full_path == self.memory_root.resolve():delete()to checkif full_path == await self.memory_root.resolve():test_filesystem.pyto parametrize and assert that/memories,/memories/, and/memories/.are all rejected.