Skip to content

Commit 5a07d2a

Browse files
authored
fix: root a glob in the workspace, not in the machine (#106)
`glob_command` passed its root through to `find`, and a caller naming the backend's root spells it `/`, `""` or `.`. For a backend addressing files by virtual path, `/` *is* the top of the namespace; for a shell it is the machine. So `find /` searched the whole container. Measured against a running `sandboxd`, in a session holding three files: pattern entries outside the workspace * 2540 2540 **/* 2540 2540 *.txt 25 22 ./**/* 0 - Two things follow, and neither announces itself. An agent's own `glob` tool reads the image it is running on - `/proc`, `/usr`, every path in the base layer - into its context, and answers a question about the workspace with 2540 paths that are not in it. And any caller diffing two globs to learn what changed during a turn is comparing two photographs of `/proc`: one such caller posts an agent's new files back into a chat channel. The root now resolves to `.`, which is the session's working directory, and an absolute path is still passed through - `/etc` is a root a caller may mean and this is not the place to argue with them. `**/*` is fixed with it, and it is the pattern that matters most: `find -path` matches with fnmatch, where `**` is no different from `*` and every `/` in the pattern must be present in the path, so `**/*` required two slashes and missed every file at the top level. A leading `**/` means "at any depth", which is exactly what the `*/` prefix already provides, so it is dropped rather than stacked. Verified on the same service after the change: pwd -> /workspace find . -path '*/*' -type f -> ./b.txt ./deep/deeper/c.txt ./uploads/a.txt Six new tests - the three spellings of the root, an absolute root left alone, and four patterns through the globstar rule. 1670 tests, 100% coverage. Found while tracking down why an agent's `ls` reported an empty workspace in a downstream product (vstorm-co/agenticos#1039). The attachment bug there was its own, but this one is why the same product's channel snapshots and the agent's `glob` tool were both reading the container's base image.
1 parent 590630c commit 5a07d2a

2 files changed

Lines changed: 60 additions & 3 deletions

File tree

src/pydantic_ai_backends/backends/_shell.py

Lines changed: 34 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -151,15 +151,46 @@ def parse_write(result: ExecuteResponse, path: str) -> WriteResult:
151151
return WriteResult(path=path)
152152

153153

154+
ROOT_SPELLINGS = frozenset({"", "/", "."})
155+
"""The three ways a caller spells "the root of this backend".
156+
157+
`normalize_path` collapses all three to `/` for a backend addressing files by
158+
virtual path, where `/` *is* the top of the namespace. A shell-backed one has a
159+
real filesystem under it and a working directory inside it, so the same three
160+
have to become `.` instead - see :func:`glob_command` for what passing `/`
161+
through cost.
162+
"""
163+
164+
154165
def glob_command(pattern: str, path: str) -> str:
155-
"""Match files with `find`.
166+
"""Match files with `find`, rooted where the caller's path means.
167+
168+
Two things to get right, and this got both wrong.
169+
170+
**The root.** A path naming the backend's root means the session's working
171+
directory here, not the filesystem's. Passed through, `find /` read it as the
172+
machine: a glob of `*` in a container answered 2540 paths from `/proc` and
173+
`/usr` and not one from the workspace. So the agent's own search tool read
174+
the image it was running on - into its context - and a channel's "what did
175+
the agent write this turn" snapshot diffed two photographs of `/proc`.
176+
Anything genuinely absolute is still passed through: `/etc` is a root a
177+
caller may mean.
178+
179+
**Globstar.** `find -path` matches with fnmatch, where `**` is no different
180+
from `*` and every `/` in the pattern must be present in the path. So `**/*`
181+
- what anything walking a tree reaches for, including that snapshot - needed
182+
two slashes and therefore missed every file at the top level. A leading
183+
`**/` means "at any depth", which is exactly what the `*/` prefix below
184+
already provides, so it is dropped rather than stacked.
156185
157186
The pattern is matched as `-path '*/{pattern}'` so a basename glob like
158187
`*.py` matches files anywhere under the root, since `find -path` tests the
159188
whole pathname.
160189
"""
161-
quoted_path = shlex.quote(path)
162-
quoted_pattern = shlex.quote(f"*/{pattern}")
190+
root = "." if path.strip() in ROOT_SPELLINGS else path
191+
anywhere = pattern[3:] if pattern.startswith("**/") else pattern
192+
quoted_path = shlex.quote(root)
193+
quoted_pattern = shlex.quote(f"*/{anywhere}")
163194
return f"find {quoted_path} -path {quoted_pattern} -type f 2>/dev/null"
164195

165196

tests/test_shell_commands.py

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -222,6 +222,32 @@ def test_a_basename_pattern_matches_at_any_depth(self):
222222
assert "-path '*/*.py'" in command
223223
assert "-type f" in command
224224

225+
@pytest.mark.parametrize("root", ["", "/", "."])
226+
def test_the_backend_s_root_is_the_working_directory(self, root: str):
227+
"""Not the machine's root, which is what `/` means to `find`.
228+
229+
A glob of `*` with `/` passed through answered 2540 paths from `/proc`
230+
and `/usr` on a container and none from the workspace - so the agent's
231+
own search tool read the image it runs on, and a channel's snapshot of
232+
"what did the agent write" diffed two photographs of `/proc`.
233+
"""
234+
assert _shell.glob_command("*.py", root).startswith("find . ")
235+
236+
def test_an_absolute_root_is_still_absolute(self):
237+
# `/etc` is a root a caller may mean, and this is not the place to argue.
238+
assert _shell.glob_command("*.conf", "/etc").startswith("find /etc ")
239+
240+
@pytest.mark.parametrize(
241+
("pattern", "expected"),
242+
[("**/*", "*/*"), ("**/*.py", "*/*.py"), ("*.py", "*/*.py"), ("src/*.py", "*/src/*.py")],
243+
)
244+
def test_a_leading_globstar_is_dropped_rather_than_stacked(self, pattern: str, expected: str):
245+
"""`find -path` matches with fnmatch: `**` is `*`, and every `/` in the
246+
pattern must be in the path. So `**/*` needed two slashes and missed
247+
every file at the top level - and `**/*` is what anything walking a tree
248+
reaches for."""
249+
assert f"-path '{expected}'" in _shell.glob_command(pattern, "/")
250+
225251
def test_matches_are_sorted_by_path(self):
226252
rows = _shell.parse_glob(_ok("/w/b.py\n/w/a.py\n"))
227253

0 commit comments

Comments
 (0)