Conversation
oss-maintainer
left a comment
oss-maintainer
left a comment
There was a problem hiding this comment.
Summary
Good catch on the containment hole, and the namespace-bypass reasoning in the comments is right: "/" does reach the UNRESTRICTED resolver unchanged. The concern is the chosen anchor — "" is also unprefixed (applyNamespacePrefix early-returns on blank keys), so in the default namespaced mode the root listing now walks the whole workspace root instead of {workspace}/{userId}, which trades an OS-root escape for a cross-tenant one. LocalFilesystem.glob already has the namespace-aware form of this normalisation (hasNamespace(rc) ? resolvePath(rc, ".") : cwd); mirroring "." here keeps both modes contained without a new convention.
The second point is consistency: ls normalises "/" and "." to "" while grep only rewrites "/", so grep(".") / grep(null) still resolve through the namespace and land on a different tree than ls("."). One shared normaliser for {null, "/", "."} across ls / grep / glob would close that.
Non-blocking: the PR only anchors the exact root spellings — arbitrary absolute paths (read_file("/etc/passwd"), list_files("/tmp")) still pass through the UNRESTRICTED default backend unchanged. That is pre-existing and out of scope for #3253, but it is the same threat model, so it is worth a follow-up issue (a ROOTED/SANDBOXED default backend for remote specs, like the shared-mode branch in #3247).
Findings
- [Warning]
CompositeFilesystem.java:204—""bypasses the per-user namespace too; prefer"."(namespace-aware) or gate the workspace-root view behind an opt-in. - [Warning]
CompositeFilesystem.java:285-289—ls/grepdisagree on what the root is;grep(".")andgrep(null)keep the old namespaced anchor. - [Info]
CompositeFilesystem.java:203— absolute-path pass-through on theUNRESTRICTEDdefault backend is unaddressed (pre-existing, suggest follow-up issue). - [Info]
RemoteFilesystemSpecTest.java:184-206— whitelist assertion locks in the namespace bypass and will break when a new shared route is added; a sibling-tenant-absent assertion would pin the isolation contract instead.
Tests: two new/extended test classes covering both specs and both modes — appreciated. The LocalFilesystemSpec shared-mode javadoc explicitly documents the multi-tenant unsafety, which is the right pattern; the CompositeFilesystem root change needs the same level of intent stated (or the "." anchor).
Automated review by github-manager-bot
| // A blank path anchors the default backend at its own root (LocalFilesystem's | ||
| // resolvePath resolves blank keys to the backend cwd, unprefixed); "/" would pass | ||
| // through the UNRESTRICTED resolver unchanged and enumerate the OS root (#3253). | ||
| LsResult defaultResult = defaultBackend.ls(runtimeContext, ""); |
There was a problem hiding this comment.
Anchoring the default backend at "" does close the OS-root escape, but for a namespaced default backend it also drops the namespace: LocalFilesystem.applyNamespacePrefix early-returns for blank keys (key == null || key.isBlank() -> return key), so resolvePath(rc, "") lands on cwd (the raw workspace root) instead of {workspace}/{userId}. Net effect for RemoteFilesystemSpec in its default (non-shared, namespaced) mode: list_files(".") enumerates every tenant's namespace directory (local-user/, other-user/, ...), while read_file("x") still resolves inside the caller's own namespace. That weakens tenant isolation for existing users by default, and rootListingAndGrepStayInsideTheWorkspace below bakes it in (the whitelist has no local-user entry, so the test asserts the bypass).
Suggest anchoring on the backend's own logical root the way LocalFilesystem.glob already does, i.e. namespace-aware:
// "." keeps the namespace prefix (applyNamespacePrefix -> "<ns>/."), "" does not
LsResult defaultResult = defaultBackend.ls(runtimeContext, ".");With a namespaced UNRESTRICTED backend "." resolves to {workspace}/{userId}; with the shared-mode backend (namespace factory is null) it resolves to cwd, so both modes get a contained root and the multi-tenant default is preserved. If exposing the workspace root at "." in namespaced mode is actually intended, it deserves its own opt-in (like sharedLocalWorkspace) plus a test asserting what a non-shared root listing may and may not see.
| // Same containment anchor as the ls root branch above: "/" must not reach the | ||
| // default backend, whose UNRESTRICTED resolver would scan from the OS root (#3253). | ||
| String defaultBackendPath = "/".equals(path) ? "" : path; | ||
| GrepResult defaultResult = | ||
| defaultBackend.grep(runtimeContext, pattern, defaultBackendPath, glob); |
There was a problem hiding this comment.
The grep branch only rewrites "/", so after this change ls and grep disagree, and grep disagrees with itself:
| call | root reached |
|---|---|
ls("/") / ls(".") |
"" -> default backend cwd (workspace root, namespace bypassed) |
grep("/…") |
"" -> cwd (workspace root) |
grep(".") |
"." -> <ns>/. -> {workspace}/{userId} |
grep(null) |
null -> <ns>/. -> {workspace}/{userId} |
So the same logical query (list_files(".") vs grep_files(".", …), or pattern with path=null vs path="/") walks two different trees, and the "." case is exactly the one a client gets when the tool layer normalises an empty path. Please normalise null, "/" and "." to one shared constant for both ls and grep (and keep it consistent with the already-correct glob handling) instead of a per-literal ternary — one small helper (e.g. rootAnchor(path)) would also keep the two comment blocks in sync.
| LsResult defaultResult = defaultBackend.ls(runtimeContext, "/"); | ||
| // A blank path anchors the default backend at its own root (LocalFilesystem's | ||
| // resolvePath resolves blank keys to the backend cwd, unprefixed); "/" would pass | ||
| // through the UNRESTRICTED resolver unchanged and enumerate the OS root (#3253). |
There was a problem hiding this comment.
Scope note: the literal "/" is fixed here, but the UNRESTRICTED default backend in RemoteFilesystemSpec still passes any absolute path straight through (resolveUnrestricted: if (target.isAbsolute()) return target;), so list_files("/etc") / read_file("/home/other/.aws/credentials") reach the host filesystem exactly as before — and the root branch only fires for the exact strings "/" and ".". This is pre-existing and arguably out of scope for #3253, but since this PR is the containment fix for that area, it is worth a follow-up issue: a remote-spec default backend probably wants SANDBOXED/ROOTED resolution (like the shared-mode branch added in #3247) rather than UNRESTRICTED, otherwise "contain to the backend root" only holds for one spelling of the root.
| Set<String> allowedTopSegments = | ||
| new HashSet<>( | ||
| List.of( | ||
| "uploads", | ||
| "memory", | ||
| "skills", | ||
| "subagents", | ||
| "knowledge", | ||
| "plans", | ||
| "agents")); | ||
| for (String root : new String[] {".", "/"}) { | ||
| LsResult ls = fs.ls(RT, root); | ||
| assertTrue(ls.isSuccess(), () -> "root listing '" + root + "': " + ls.error()); | ||
| for (FileInfo fi : ls.entries()) { | ||
| String top = fi.path().split("/")[0]; | ||
| assertTrue( | ||
| allowedTopSegments.contains(top), | ||
| "listing entry escaped the workspace: " + fi.path()); | ||
| } | ||
| } | ||
| assertTrue( | ||
| fs.ls(RT, ".").entries().stream().anyMatch(fi -> fi.path().startsWith("uploads")), | ||
| "workspace content must remain visible in the root listing"); |
There was a problem hiding this comment.
The whitelist-based assertion is a good containment check, but it couples the test to whatever happens to sit at the workspace root, and it never asserts the namespace expectation for the default (non-shared) mode — i.e. that a second tenant's directory is not listed. Consider (a) creating a sibling {workspace}/other-user/secret.md in the test and asserting it is absent from ls("."), which pins the isolation question raised on the ls hunk either way, and (b) replacing the hardcoded allowed-segment set with a startsWith(workspace.getFileName())-style containment check on the resolved path, so a future shared route (e.g. knowledge-v2/) does not fail the suite for the wrong reason.
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Codecov 对 PR agentscope-ai#3254 报 1 行 partial:defaultBackendPath 三元式的 "/"→"" 臂已有 grep("/") 测试,": path"("."/null 直通)臂没有触发。 在 rootListingAndGrepStayInsideTheWorkspace 中补充:经框架写入 notes/second.md 后 grep(".") 命中——同时钉住 "." 路径保持既有 命名空间扫描语义不变。
|
Added the missing branch: the "."-path passthrough arm of the grep ternary is now covered in rootListingAndGrepStayInsideTheWorkspace (write notes/second.md through the framework, then grep(".") finds it — also pinning that "." keeps its namespaced-scan semantics). Module suite 1057/1057 green. |
|
Thanks for the sharp review — the namespace-bypass point is exactly right, reworked in c04290c:
Module suite 1057/1057 green. |
oss-maintainer
left a comment
oss-maintainer
left a comment
There was a problem hiding this comment.
Summary
Re-review after the follow-up push (8e181706 → c04290c1). Switching the root anchor from "" to "." fixes the flaw raised last time: a blank key makes applyNamespacePrefix early-return, which traded the OS-root escape for a cross-tenant one. The rewritten tests are the right shape — asserting other-user/ does not leak into the root listing is exactly the property that matters here, and dropping the hard-coded shared-route whitelist in favor of behavioral assertions is an improvement. Not approving, because the "." anchor is a LocalFilesystem convention being applied at a backend-agnostic seam, and glob now disagrees with ls/grep.
Correctness — verified against the resolver: resolvePath(rc, ".") → applyNamespacePrefix prepends the namespace ("." is neither blank nor absolute per isAbsolutePathString), so a namespaced backend anchors at {workspace}/{userId} and a namespace-free one at its own cwd. The containment claim holds for the LocalFilesystem default backend, and the description's "shared mode anchors at the backend's own root" is accurate.
Compatibility (main concern) — rootAnchor is applied to defaultBackend, which is typed AbstractFilesystem and documented as taking a path starting with /. Two real callers install a non-local default: RoutedSandboxFilesystem:49 (primary is an AbstractSandboxFilesystem), whose ls/grep are shell-generated (for f in <path>/*, grep -r … <path>), so "." is expanded against the command cwd and entries come back as ./name instead of /name. That is a user-visible change in listing shape for sandbox-routed agents, and no test here covers a non-local backend. If that path is unreachable in practice, a comment saying so is enough; otherwise the normalization belongs in LocalFilesystem, or gated by backend type.
Consistency — glob's root branch still forwards the raw "/"/null and relies on LocalFilesystem.glob normalizing internally, and route backends keep receiving "/". So there is no longer one definition of "composite root" across the three scanning operations — the same class of drift that produced #3253.
Tests — good on namespaced/shared local modes. Missing: a non-LocalFilesystem default backend, and an equivalence assertion that "/", "." and null produce identical listings (containment is checked, cross-spelling equality is not).
Nits — the : path fallback in rootAnchor is unreachable from both call sites (both already tested null/"/"/"." before calling it), so the Codecov partial that motivated the follow-up commit will remain partial; and the PR description still documents the superseded "" anchor ("Pass a blank path instead of \"/\"", resolvePath blank→cwd), which will mislead anyone bisecting this file.
Findings
- [Warning]
CompositeFilesystem.java:528—"."is aLocalFilesystemconvention applied at a backend-agnostic seam; sandbox default backends shell-expand it to./nameand theAbstractFilesystemjavadoc mandates/-rooted paths. - [Warning]
CompositeFilesystem.java:201—globstill forwards raw"/"/null; the three root-scanning operations now have two different normalization owners. - [Info]
CompositeFilesystem.java:520-528— unreachable ternary fallback; a constant (or callingrootAnchorbefore the root branch) would be clearer and actually resolves the coverage partial.
Automated review by github-manager-bot
Additional notes (not anchored to a changed line)
- [INFO]
agentscope-harness/src/main/java/io/agentscope/harness/agent/filesystem/CompositeFilesystem.java:520— The: pathfallback arm looks unreachable from both call sites —lscallsrootAnchoronly inside"/".equals(path) || ".".equals(path)andgreponly insidepath == null || "/".equals(path) || ".".equals(path), so it always returns".". That also means the Codecov partial that motivated the follow-up test will stay partial:grep(".")takes the true arm, not the fallback. If the fallback is not meant to be reachable, a constant (ROOT_ANCHOR = ".", or a no-arg helper) is clearer and fully covered; if it is,rootAnchorshould be called before the root branch rather than inside it. (multi-line range invalid, anchoring single line)
| * namespace-free one (shared mode) at its own root — mirroring the normalization {@code | ||
| * LocalFilesystem.glob} already applies (#3253). | ||
| */ | ||
| private static String rootAnchor(String path) { |
There was a problem hiding this comment.
"." is only a root anchor for LocalFilesystem. The composite is backend-agnostic, and two of its callers install a non-local default backend:
RoutedSandboxFilesystem:49→new CompositeFilesystem(primary, routes)withprimaryanAbstractSandboxFilesystem;BaseSandboxFilesystem.lsbuildsfor f in <path>/*andgrepbuildsgrep -r … <path>, so"."is expanded by the shell against the command cwd (entries come back as./name, not/name), while"/"enumerated the sandbox root.RemoteFilesystemSpec:250/HarnessAgent:2485also pass through whatever the caller resolved.
AbstractFilesystem.ls documents @param path absolute path to the directory to list (must start with '/') (same wording on read/delete/exists), so this normalization leaks a LocalFilesystem-specific convention across a backend-agnostic seam. Safer shapes: normalize inside LocalFilesystem (where resolvePath semantics actually justify "."), or gate the substitution on the default backend being local, or teach the sandbox backend that "." means its own root and cover it with a test. Which of those matches the intent here?
| if ("/".equals(path) || ".".equals(path)) { | ||
| List<FileInfo> results = new ArrayList<>(); | ||
| LsResult defaultResult = defaultBackend.ls(runtimeContext, "/"); | ||
| LsResult defaultResult = defaultBackend.ls(runtimeContext, rootAnchor(path)); |
There was a problem hiding this comment.
ls and grep now rewrite the root spelling before calling the default backend; glob (lines ~345/349) still forwards the raw path ("/" or null) and depends on LocalFilesystem.glob normalizing it internally, and route backends keep receiving "/". So after this change there is no single answer to "what does the composite call root": ls/grep say ".", glob says "/", and the routes say "/". That is the same kind of per-method drift that produced #3253.
Could glob go through the same helper, or (per the other comment) could the normalization move into the backends so the composite always forwards "/"? Either way, a short note in the class javadoc stating the root spelling the composite guarantees to its backends would keep the next operation from re-introducing this.
| return route.backend().exists(runtimeContext, route.backendPath()); | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
The : path fallback arm looks unreachable from both call sites — ls calls rootAnchor only inside "/".equals(path) || ".".equals(path) and grep only inside path == null || "/".equals(path) || ".".equals(path), so it always returns ".". That also means the Codecov partial that motivated the follow-up test will stay partial: grep(".") takes the true arm, not the fallback. If the fallback is not meant to be reachable, a constant (ROOT_ANCHOR = ".", or a no-arg helper) is clearer and fully covered; if it is, rootAnchor should be called before the root branch rather than inside it.
|
Since the stacked base is in git fetch upstream
git rebase --onto upstream/main 169eed83 HEAD
git push --force-with-lease(plain CI on this PR is currently green ( Automated notification by github-manager-bot |
CompositeFilesystem 对 ls 的根分支("") 与 grep 的 "/" 向 default
backend 传绝对路径,UNRESTRICTED 解析器原样放行,操作锚定 OS 根——
默认配置下 list_files(".") 枚举盘根(agentscope-ai#3253)。
修复:rootAnchor(path) 助手将 {null, "/", "."} 统一归一为
命名空间感知的 "."(镜像 LocalFilesystem.glob 的既有归一化):
命名空间后端锚定 {workspace}/{userId}(保住租户隔离——空串会绕过
前缀造成跨租户列表),共享模式锚定自身根。ls 与 grep 共用,三种根
拼写走同一棵树。
测试:兄弟租户目录不泄漏进根列表、调用者自身树可见、三种根拼法
grep 一致;共享模式含工作区外标记不可见断言。
Fixes agentscope-ai#3253
c04290c to
b5d4977
Compare
oss-maintainer
left a comment
oss-maintainer
left a comment
There was a problem hiding this comment.
Summary
Re-reviewed after the new commits since my last pass (head now b5d4977e). The containment fix is correct and well-tested: routing default-backend ls/grep root enumerations through rootAnchor (. instead of //null) closes both the OS-root leak under UNRESTRICTED resolvers and the blank-key namespace bypass, and mirrors the normalization LocalFilesystem.glob already applies (#3253). CLA signed. Two minor informational notes inline; no blockers.
Stacked-PR note: this PR is stacked on #3247 — it should not be merged before #3247 lands (or is rebased onto main independently).
Automated review by github-manager-bot
| * LocalFilesystem.glob} already applies (#3253). | ||
| */ | ||
| private static String rootAnchor(String path) { | ||
| return (path == null || "/".equals(path) || ".".equals(path)) ? "." : path; |
There was a problem hiding this comment.
[Info] rootAnchor defensively handles null, but both call-site guards differ: ls only enters this branch for "/"/".", while grep also passes null. Behaviour is correct today; consider aligning the ls guard with grep (or a short comment) so the null case stays reachable if ls ever routes null paths.
| // ==================== | ||
|
|
||
| @Test | ||
| void rootListingAndGrepAnchorAtTheCallersNamespace() throws Exception { |
There was a problem hiding this comment.
[Info] Good multi-tenant isolation coverage (sibling-tenant leak + .. escape assertions, shared-mode escape check). One addition worth considering: an assertion that ls("/") and ls(".") return identical entry sets for a namespaced backend, locking in the anchor-equivalence guarantee that the fix relies on.
- ls 的根分支守卫补上 path == null:此前 ls(null) 经 routeForPath 落到
默认分支后,resolvePath 对 null key 短路返回裸 cwd——与空串相同的
命名空间旁路;对齐后 null 经 rootAnchor 归一为 ".",与 grep 一致
- 新增锚点等价断言:ls("/")、ls(null) 与 ls(".") 的条目集合逐一
相等,把修复依赖的"全部根拼写共享同一锚点"钉进测试
|
Thanks — both Info items addressed in edc4493:
Module suite 12/12 on the touched classes green. One clarification on the stacked-PR note: it's stale — #3247 merged on 2026-09-24, and this branch was rebased onto |
oss-maintainer
left a comment
oss-maintainer
left a comment
There was a problem hiding this comment.
Summary
The follow-up commit adds null to ls's root guard and tests that ls(null) returns the same entry set as ls(".") and ls("/"). It partially resolves the prior null-guard and test-coverage notes, so this round is a comment, not a block: the commit is a strict improvement and I am not requesting changes. Two follow-ups are flagged inline — one Warning (blank/whitespace paths still bypass the anchor) and one Info (a configured "/" route keeps ls(null) and grep(null) non-equivalent). CLA is signed (license/cla=pending was not observed for this PR) and GitHub reports the head as MERGEABLE.
Previous round's notes
ls(null)did not take the same namespace-aware root path asgrep(null)— partially resolved (CompositeFilesystem.java:201-204sends un-routednullthroughrootAnchor, butlsstill routes before this guard when a"/"route exists).- No direct regression assertion pinned
ls(null)to the other root spellings — partially resolved (RemoteFilesystemSpecTest.java:213-220comparesnulland"/"with".", but does not cover blank paths or a"/"route). - PR #3254 was stacked on #3247 — resolved (
view.jsonsays it was rebased;compare.jsonshowsb5d4977eparented by localmainHEAD6a056809, andRemoteFilesystemSpec.sharedLocalWorkspaceis present onmain).
Findings
- [Warning]
agentscope-harness/src/main/java/io/agentscope/harness/agent/filesystem/CompositeFilesystem.java:201— non-blocking: empty and whitespace paths bypass the namespace anchor inls,grep, and siblingglob, leaking the raw workspace/sibling tenant entries for a namespaced UNRESTRICTED backend. - [Info]
agentscope-harness/src/main/java/io/agentscope/harness/agent/filesystem/CompositeFilesystem.java:201— A permitted"/"route makesls(null)select that route whilegrep(null)takes root aggregation.
Verified good
CompositeFilesystem.java:201-204,530-532sends ordinary default-backendnull,"/", and"."root calls to the literal"."namespace anchor rather than an absolute root.CompositeFilesystem.java:282-285uses the same anchor forgrep; apart from the configured-root-route case,"./", absolute/relative subpaths,..paths, and Windows-separator paths all bypass both root guards consistently.RemoteFilesystemSpecTest.java:213-220executes the newls(null)branch and compares its sorted entry paths withls(".")andls("/"); reverting the newnullpredicate would fail this fixture.CompositeFilesystem.java:328-397is the only other enumeration surface (glob); there is nofind,walk,tree,stat, orreadDirentry point in this class. Rootnull/"/"globbing remains normalized by the defaultLocalFilesystem; the blank-path gap is reported above.
Stacked-PR note
The #3247 merge-order warning no longer applies: the supplied PR metadata says this branch was rebased after #3247 merged, compare.json places the PR base directly on 6a056809, and that is the checked-out main commit.
Automated review by github-manager-bot
| if ("/".equals(path) || ".".equals(path)) { | ||
| // Guard aligned with grep (and rootAnchor): a null path must reach the same | ||
| // namespaced anchor, not the backend's raw cwd via the blank-key shortcut (#3253). | ||
| if (path == null || "/".equals(path) || ".".equals(path)) { |
There was a problem hiding this comment.
The root predicate still excludes "" and whitespace. Those values fall through to the raw default backend in ls and grep, and glob has the same bypass. LocalFilesystem.applyNamespacePrefix leaves blank keys unchanged and resolvePath maps them to cwd, exposing sibling tenant directories in the namespaced UNRESTRICTED backend. FilesystemTool and WorkspacePathNormalizer preserve blank strings. Treat blank paths as the same root anchor (or reject them consistently) in all three enumeration methods, and test empty and whitespace paths alongside null.
| if ("/".equals(path) || ".".equals(path)) { | ||
| // Guard aligned with grep (and rootAnchor): a null path must reach the same | ||
| // namespaced anchor, not the backend's raw cwd via the blank-key shortcut (#3253). | ||
| if (path == null || "/".equals(path) || ".".equals(path)) { |
There was a problem hiding this comment.
ls and grep are still not strictly symmetric when a caller configures a "/" route: ls(null) calls routeForPath(null) before this guard and selects that route, whereas grep(null) skips routing and aggregates the default and root routes. HarnessAgent.Builder.filesystemRoute accepts such a prefix. If null is intended as a root spelling, skip route resolution for it in ls as grep does, then add a root-route regression test.
复审两条跟进:
- Warning: 空串/纯空白路径此前绕过聚合根锚点,直达默认后端原始
cwd(LocalFilesystem.applyNamespacePrefix 对空白键早退会剥掉命名空间,
在 namespaced UNRESTRICTED 后端泄漏工作区/兄弟租户)。ls/grep/glob
统一改用 isRootSpelling(null/blank/"/"/"."),空白与其他根拼写同走
聚合分支;LocalFilesystem 侧以 isRootPath 把所有根拼写锚定到
{workspace}/{userId}。
- Info: 配置了 "/" 路由时 ls(null)/glob(null) 会先被 routeForPath
捕获选中该路由,而 grep(null) 跳过路由走聚合,三者不对称。ls/glob
改为与 grep 相同的"仅非 null 路径参与路由解析"。
聚合分支转发统一为契约拼写 "/"(grep 的 null 是其文档化 cwd 形式,
按原样转发),Composite 对后端只发送有文档的拼写。新增测试:
- null/blank 根扫描转发契约拼写的 mock 断言(含空白)
- "/" 路由下 ls(null) 与 grep(null) 聚合对称的回归测试
- RemoteFilesystemSpecTest 锚点等价循环补 ""/空白拼写
- LocalFilesystem UNRESTRICTED 下四种根拼写 ls/grep/glob 等价且不泄漏
全模块 1060 个测试通过。
oss-maintainer
left a comment
oss-maintainer
left a comment
There was a problem hiding this comment.
Summary
Re-reviewed head 6e569be1 (one commit since my edc4493c review). The commit does what the follow-up asked: isRootSpelling now covers blank/whitespace, null no longer participates in route resolution for ls/glob, the aggregate branches forward the contract spelling "/", and LocalFilesystem gained isRootPath so a root-spelled scan cannot reach the OS root or drop the per-user namespace. The new tests — including the sibling-tenant leak assertion — are the right shape. I am not approving this round: the "/" vs "." asymmetry through routeForPath is still real when a "/" route is configured, and forwarding "/" widens the blast radius to default backends that were not updated. CI (build ubuntu-latest/windows-latest) was still pending when this review was posted.
Findings
- [Warning]
CompositeFilesystem.java:210— default backends other thanLocalFilesystemmay treat"/"as an absolute/container root; onlyLocalFilesystemanchors it here. - [Warning]
CompositeFilesystem.java:190— with a configured"/"route,ls("/")takes the routed branch whilels("."),ls(null)andls("")take the aggregate branch, so the four root spellings are not equivalent. - [Info]
LocalFilesystem.java:239— enumeration surfaces normalize blank/null; mutation surfaces do not. Document the contract.
Suggestions
Check isRootSpelling(path) before routeForPath(path) in ls/grep/glob, and extend rootScans_forwardContractSpellingToNonLocalDefaultBackend / nullRootScan_aggregatesDefaultAndRootRoute_symmetricWithGrep to cover a non-local default backend and the "/" spelling with a root route present.
Automated review by github-manager-bot
| // strip the per-user namespace from the anchor and expose sibling tenants. | ||
| if (isRootSpelling(path)) { | ||
| List<FileInfo> results = new ArrayList<>(); | ||
| LsResult defaultResult = defaultBackend.ls(runtimeContext, "/"); |
There was a problem hiding this comment.
[Warning] Forwarding the contract spelling "/" instead of the internal "." anchor is right for the seam, but only LocalFilesystem was taught to treat "/" as "my own root" in this commit. OverlayFilesystem, BakedContextFilesystem, AbstractSandboxFilesystem and RemoteFilesystem can all be installed as defaultBackend (the CompositeFilesystemTest comment calls out RoutedSandboxFilesystem doing exactly that), and for them "/" is an absolute path — a sandbox default backend now enumerates the container root, which is the same class of leak #3253 reported. Please audit the other AbstractFilesystem implementations, or keep passing the anchor the previous default backend understood, and add a contract test that every default backend roots "/" inside its own workspace.
| // below, exactly like grep(null) (review follow-up, #3253). | ||
| if (path != null) { | ||
| RouteResult route = routeForPath(path); | ||
|
|
There was a problem hiding this comment.
[Warning] Route resolution still precedes the root check, so the four root spellings are not equivalent when a "/" route is configured. stripLeadingSlash("/") is "", which matches the canonicalized "/" prefix in routeForPath and takes the routed branch (only that backend's entries), while "." matches nothing and null/blank now skip routing entirely, so all three take the aggregated branch. nullRootScan_aggregatesDefaultAndRootRoute_symmetricWithGrep pins this: ls(null) aggregates default + root route, but ls("/") on the same composite returns only the route. Moving isRootSpelling(path) above the if (path != null) block (and asserting "/" and "." in that test alongside null/"") makes the equivalence real; otherwise the javadoc claim that all root spellings mean "this filesystem's own root" does not hold.
| @Override | ||
| public LsResult ls(RuntimeContext runtimeContext, String path) { | ||
| Path dirPath = resolvePath(runtimeContext, path); | ||
| Path dirPath = resolvePath(runtimeContext, isRootPath(path) ? "." : path); |
There was a problem hiding this comment.
[Info] Root anchoring is applied to the enumeration surfaces (ls/grep/glob) but read/write/edit/delete/exists still hand "/" and "" straight to resolvePath, so ls("") and read("") disagree about what a blank path means. Defensible, but worth one sentence in AbstractFilesystem stating that blank/null are root spellings only for enumeration, so the next backend does not have to guess.
- ls/grep/glob 三处把 isRootSpelling(path) 提到 routeForPath 之前:
配置了 "/" 路由时,ls("/") 此前被路由捕获只返回该路由的条目,
而 ls(".")/ls(null)/ls("") 走聚合分支——四种根拼写并不等价。
前移后统一走聚合根视图,测试断言四种拼写的条目集合逐一相等
- 非本地默认 backend 的 "/" 语义审计:RemoteFilesystem 的 ls 对
blank path 会在 endsWith 上 NPE(合并短路修复),null/blank 归一为
"/"(虚拟 store 根,与 grep/glob 的 normalizePath 一致);
Overlay/BakedContext 委托传递;契约测试把根拼写收集次数提升以
覆盖 "/" 路由存在时的场景(routed backend never 收到 ls)
- AbstractFilesystem.ls 补充根拼写契约文档:枚举面(null/blank/
"/"/"." 一律锚定自身根)与变更面(read/write 等不承诺)的区别
|
Both warnings addressed in 71941a4, plus the Info:
Module suite matches the upstream baseline (2462 tests, 0 failures, only the two known Windows symlink-privilege environment errors). |
oss-maintainer
left a comment
oss-maintainer
left a comment
There was a problem hiding this comment.
Summary
This PR normalizes CompositeFilesystem enumeration root spellings and anchors LocalFilesystem root scans through the caller namespace. The new head resolves the previous route-order asymmetry and documents the enumeration-only scope, but it does not make the canonical "/" safe for sandbox default backends. Request changes: routed sandbox grep can now recursively scan the container root, and root-equivalent path forms still bypass the local namespace.
Previous round's findings
- Default backends other than LocalFilesystem may treat
"/"as an absolute root — unresolved (RoutedSandboxFilesystem delegates its default backend to BaseSandboxFilesystem, which directly shells"/"). - A configured
"/"route made root spellings take divergent paths — resolved (the root predicate now runs before route resolution inls,grep, andglob). - The enumeration-only contract needed documentation — resolved (AbstractFilesystem now distinguishes enumeration from mutation surfaces).
Findings
- [Warning]
agentscope-harness/src/main/java/io/agentscope/harness/agent/filesystem/CompositeFilesystem.java:299— Canonicalizinggrep(".")to"/"makes routed sandbox defaults recursively search the container root. - [Warning]
agentscope-harness/src/main/java/io/agentscope/harness/agent/filesystem/CompositeFilesystem.java:553— Root-equivalent paths such as"/."and"//"bypass the textual root predicate and reach the OS root. - [Warning]
agentscope-harness/src/main/java/io/agentscope/harness/agent/filesystem/remote/RemoteFilesystem.java:234— Direct RemoteFilesystem listings still do not honor the newly documented"."and whitespace root spellings.
Checks
- CLA: signed (
license/cla= success on head71941a4e). - CI at review time:
Check License,Check Module Sync,build (ubuntu-latest),codecov/patchpass;build (windows-latest)still pending.
Verified good
- Root checks precede route resolution in all three CompositeFilesystem enumeration methods.
- LocalFilesystem maps null, blank, and
"/"to"."beforels,grep, andglobresolution, preserving its namespace prefix. - The new composite test verifies that root spellings bypass a configured
"/"route and aggregate its entry with the default view. - The namespaced local test asserts that a sibling tenant is absent from root listings, grep results, and glob results.
- The shared-workspace test checks that a marker outside the workspace is not returned by a root listing.
Automated review by github-manager-bot
Additional notes (not anchored to a changed line)
- [WARNING]
agentscope-harness/src/main/java/io/agentscope/harness/agent/filesystem/remote/RemoteFilesystem.java:234— The newAbstractFilesystemcontract says"."and blank spellings mean the backend root, but directRemoteFilesystem.ls(".")still queries"./"and whitespace queries a whitespace prefix, rather than the store root. The composite tests hide this because they forward"/"to the mock/default backend. Normalize null, blank,"/", and"."to"/"in the remote implementation and add direct remotelscoverage. (line outside diff)
| List<GrepMatch> allMatches = new ArrayList<>(); | ||
| GrepResult defaultResult = defaultBackend.grep(runtimeContext, pattern, path, glob); | ||
| GrepResult defaultResult = | ||
| defaultBackend.grep(runtimeContext, pattern, path == null ? null : "/", glob); |
There was a problem hiding this comment.
Canonicalizing non-null root spellings to "/" is unsafe for the production RoutedSandboxFilesystem default backend. It delegates to BaseSandboxFilesystem, which shells the supplied grep path directly, so grep(..., ".") changes from the sandbox working directory to recursive grep -r ... /, exposing and scanning the container root. The same backend cannot satisfy the new contract for root ls/glob either. Make sandbox backends normalize the contract root to their workspace before this forwarding, and add an integration test through RoutedSandboxFilesystem.
| * meaning of its own root — namespaced local backends anchor {@code "/"} at {@code | ||
| * {workspace}/{userId}} via {@code LocalFilesystem#isRootPath}. | ||
| */ | ||
| private static boolean isRootSpelling(String path) { |
There was a problem hiding this comment.
This recognizes only four raw strings. Valid paths such as "/.", "//", and "/tmp/.." do not match, so an UNRESTRICTED namespaced LocalFilesystem receives an absolute path and resolves it to the OS root, bypassing the namespace for ls, grep, and glob. Normalize paths before the root/route decision (and apply the same rule in LocalFilesystem.isRootPath), then cover these root-equivalent forms in the containment tests.
- AbstractFilesystem 新增共享 denotesRootPath():折叠重复分隔符并按段 解析 ./..,"/."、"//"、"/tmp/.." 等根等价形式不再绕过文本四串 判定直达 OS 根;CompositeFilesystem.isRootSpelling 与 LocalFilesystem.isRootPath 均委托它 - BaseSandboxFilesystem ls/grep/glob 增加 rootAnchor:根拼写锚定沙箱 工作目录"."——修复转发契约拼写 "/" 后 RoutedSandboxFilesystem 的 grep -r 会递归容器根的回归 - RemoteFilesystem normalizePath 认识四种根拼写(null/blank/"/", "."),ls 改用 normalizePath(顺带修 null NPE 与 "./" 前缀查询) - 测试:根等价拼写的聚合断言(composite)、沙箱命令形态罐头测试 +经 RoutedSandboxFilesystem 的集成测试、remote 直连 ls 四拼写 返回 store 根的新测试类
|
All three warnings addressed in aa49b43:
Module suite matches the upstream baseline (2462 tests, 0 failures, only the two known Windows symlink-privilege environment errors). |
oss-maintainer
left a comment
oss-maintainer
left a comment
There was a problem hiding this comment.
Summary
The new head aa49b433 closes all three points from the previous round: grep(".")/ls no longer recurse the container root (BaseSandboxFilesystem.rootAnchor), root-equivalent spellings are handled by one shared canonical check (AbstractFilesystem.denotesRootPath), and RemoteFilesystem now honours null/blank///. consistently across ls/grep/glob, with direct tests for each backend.
That still leaves one blocking regression: routing ls through normalizePath removed the trailing / the rest of the method depends on, so listing a non-root directory now drops its files and can surface sibling prefixes. Keeping the request for changes on that one point — the fix is a couple of lines plus one test case.
Findings
- [Critical]
remote/RemoteFilesystem.java:160— non-rootls()loses its trailing-separator prefix: direct children are emitted as a self-referencing directory entry instead of files, andstartsWith/listByPrefixnow over-match sibling prefixes. - [Info]
remote/RemoteFilesystem.java:725—normalizePathis shared with the mutation surfaces, so"."is accepted there too; either scope the root spelling to enumeration or document it.
Verified good
denotesRootPathcollapses duplicate separators and./..textually, so/.,//and/tmp/..reach the root branch instead of the OS root;LocalFilesystemandCompositeFilesystemnow share the single implementation.BaseSandboxFilesystemanchorsls/grep/globat.(the sandbox working directory) for every root spelling, which is what the routed-sandbox default backend needed.globalready used thelength() + 1form, confirming thelsasymmetry above is unintended rather than a contract change.- Test coverage is real and behavioural:
CompositeFilesystemTestassertstimes(9)contract-spelling forwarding including the canonical forms,RemoteFilesystemRootSpellingTestpins the store-root spellings, andBaseSandboxFilesystemTestasserts the emitted shell commands anchor at'.'.
Checks
- CLA: signed (
license/cla= success onaa49b433). - CI on the current head:
Check License,Check Module Sync,build (ubuntu-latest),build (windows-latest),codecov/patch— all passing.
Automated review by github-manager-bot
| String normalizedPath = path.endsWith("/") ? path : path + "/"; | ||
| // Root spellings ("." and blank included) normalize to the store root, matching | ||
| // grep/glob; this also null-proofs the downstream prefix handling (#3253 review). | ||
| String normalizedPath = normalizePath(path); |
There was a problem hiding this comment.
[Critical] Switching ls to normalizePath(path) drops the trailing separator the rest of this method still depends on, so a non-root directory listing loses its files.
The removed line built path + "/" unconditionally for non-root paths; normalizePath does the opposite (normalized.substring(0, length - 1) strips a trailing /, and none is added). With a store key /memory/notes.md and ls("/memory"):
relative = key.substring(normalizedPath.length())is now"/notes.md", not"notes.md";relative.contains("/")is therefore true, so the file is never emitted as a file — it becomessubdirs.add("/memory" + "" + "/"), i.e. the directory re-lists itself as its own child and the entry is lost;item.key().startsWith(normalizedPath)in the fallback (andindex.listByPrefixin the fast path) also lost its separator guard, so/memory-backup/x.mdnow matches als("/memory")request and surfaces as a bogus child.
glob in this same class already handles it the other way (key.substring(normalizedPath.length() + 1), with "/".equals(normalizedPath) special-cased) — ls needs the same treatment. Suggested fix, keeping the root case intact:
String normalizedPath = normalizePath(path);
String prefix = "/".equals(normalizedPath) ? "/" : normalizedPath + "/";then use prefix for hasPrefix / listByPrefix / startsWith / substring, or reuse matchesPathPrefix(key, normalizedPath) like grep/glob do.
This is not covered by CI: RemoteFilesystemRootSpellingTest only exercises root spellings, and the existing non-root ls assertions go through a LocalFilesystem route. A RemoteFilesystem.ls("/memory") case asserting the child file is returned (plus a sibling-prefix key that must not appear) would pin both halves.
| if (path == null || path.isBlank()) { | ||
| // Enumeration root spellings per the AbstractFilesystem contract: null, blank, "/" | ||
| // and "." all mean the store root (#3253 review). | ||
| if (path == null || path.isBlank() || "/".equals(path) || ".".equals(path)) { |
There was a problem hiding this comment.
[Info] normalizePath is shared by the mutation surfaces too (exists, delete, move), so adding "." here also changes how those interpret it — exists(rc, ".") now asks the index about / and returns true where it previously returned false. Harmless in practice (delete/move still call validatePath first and the /-prefix match hits nothing because keys never start with //), but it does contradict the enumeration-only framing this PR documents for root spellings. Either scope the root spelling to ls/grep/glob with a dedicated helper, or state in the javadoc that "." is also accepted on the mutation surfaces.
There was a problem hiding this comment.
Correction on my own point above: I re-traced exists/delete/move and the "." change is not an actual behaviour change there — before this commit "." normalised to "/.", now to "/", and neither form matches any key, so all three surfaces still no-op. Please read this as a documentation nit only (the root-spelling set is worth stating explicitly if mutation surfaces share the helper), not as a correctness concern. The ls finding above stands.
There was a problem hiding this comment.
Scrapping my own point: I re-checked the mutation callers and the premise is wrong. With "." now normalizing to "/", exists(rc, ".") still returns false (the store lookup becomes get(ns, "/") and the child scan becomes startsWith("//"), neither of which any /-prefixed key matches), and delete/move behaved the same before ("/."). So the shared normalizePath does not change mutation behaviour - please ignore the [Info] above; only the ls() prefix finding at line 160 stands.
Automated review by github-manager-bot
ls 改用 normalizePath 后丢掉了方法其余部分依赖的尾部分隔符:
非根目录列表中 relative 变成 "/notes.md"(带前导斜杠),文件被误判为
子目录、目录自引用成子项;startsWith/listByPrefix 还会过匹配兄弟前缀
(/memory-backup 命中 ls("/memory"))。
- ls 改为 normalizePath 得到规范化路径后,再按需补尾分隔符构造 prefix:
"/" 根情形保持 "/",非根为 normalizedPath + "/";fast path 与
fallback 的 hasPrefix/listByPrefix/startsWith/substring 全部改用 prefix
- 根拼写判定改用共享 denotesRootPath(mutation 面的 normalizePath 恢复
原样,"." 不再被变更面接受——复审 Info 一并解决)
- 新增非根目录回归测试:ls("/memory") 返回直接子项(notes.md 文件 +
sub/ 目录),不出现自引用,兄弟前缀 /memory-backup 不泄漏
|
Fixed in 3a84af8:
Module suite matches the upstream baseline (2462 tests, 0 failures, only the two known Windows symlink-privilege environment errors). |
|
Thanks for the re-trace — and a status note: the current head (3a84af8, pushed as the fix for the ls finding) already supersedes this hunk. So with 3a84af8: the ls trailing-separator Critical is fixed (pinned by |
|
Acknowledged — the Info is fully withdrawn, which matches the current head anyway: 3a84af8 reverted |
oss-maintainer
left a comment
oss-maintainer
left a comment
There was a problem hiding this comment.
Summary
The new commit fixes the trailing-separator regression in RemoteFilesystem.ls: non-root directories now build a child prefix (normalizedPath + "/") so siblings like /memory-backup no longer leak into ls("/memory"), and root spellings are anchored via denotesRootPath. The previous review left no recorded findings, so there are no outstanding items to resolve. However, the same commit narrowed normalizePath to reject "." for mutation surfaces, and RemoteFilesystem.grep/glob were not updated to match ls; direct calls with . violate the enumeration root-spelling contract. Overall verdict: comment (non-blocking) — align grep/glob root handling with ls before merging.
Previous round's findings
- No prior findings were recorded by the bot at commit
aa49b433.
Findings
- [Warning]
agentscope-harness/src/main/java/io/agentscope/harness/agent/filesystem/remote/RemoteFilesystem.java:727—normalizePathno longer anchors.for enumeration, so directgrep/globcalls with.search under/.and miss matches;grep/globshould use the samedenotesRootPathguard asls.
Verified good
RemoteFilesystem.lsnow usesAbstractFilesystem.denotesRootPathand a trailing-separator prefix; the newnonRootDirectoryListingKeepsTrailingSeparatorSemanticstest pins the sibling-prefix leak.CompositeFilesystemchecks root spellings before routing and forwards the contract spelling/to the default backend; the mock test verifies all root spellings bypass a configured/route.LocalFilesystemdelegates root detection todenotesRootPathand anchors root spellings viaresolvePath(rc, "."), keeping the per-user namespace in UNRESTRICTED mode.BaseSandboxFilesystem.rootAnchormaps every root spelling to., so shellls,grep -r, andfindtarget the sandbox cwd instead of the container root.AbstractFilesystem.denotesRootPathcorrectly collapses separators and resolves./..segments, catching/.,//, and/tmp/..as root spellings.
Automated review by github-manager-bot
Additional notes (not anchored to a changed line)
- [WARNING]
agentscope-harness/src/main/java/io/agentscope/harness/agent/filesystem/remote/RemoteFilesystem.java:727—normalizePathno longer maps"."to"/", butgrepandglobstill call it directly for enumeration. A directRemoteFilesystem.grep(..., ".", ...)orglob(..., ".")therefore searches under the meaningless prefix"/."and misses every match, violating theAbstractFilesystemroot-spelling contract for enumeration surfaces. KeepnormalizePathmutation-only (null/blank →"/") and changegrep/globto use the same root guard asls:denotesRootPath(path) ? "/" : normalizePath(path). (line outside diff)
This branch has not been deployed
2 participants
Fixes #3253
Problem
CompositeFilesystemroutes root-level operations to the default backend with an absolute"/"path —ls(both"."and"/") andgrep(the"/"case) — and the UNRESTRICTED resolver passes absolute paths through unchanged, so the operation anchors at the OS root:list_files(".")enumerates the drive root (#3253).globis unaffected (already anchored).Fix
A shared
rootAnchor(path)helper normalizes{null, "/", "."}to the namespace-aware"."— mirroring the normalizationLocalFilesystem.globalready applies:{workspace}/{userId}— tenant isolation preserved (a blank anchor would bypass the per-user prefix and list every tenant's directory)lsandgrepshare the helper, so all root spellings walk the same tree.Tests
rootListingAndGrepAnchorAtTheCallersNamespace— sibling tenant directory (other-user/) never appears in the root listing; the caller's own tree stays visible;grepwith".","/"andnullall walk the same treesharedModeRootListingStaysInsideTheWorkspace— shared root view with an outside-the-workspace marker file that must never appearModule suite matches upstream baseline (only the two known Windows symlink-privilege errors). The arbitrary-absolute-path gap (read/write/edit through the same backend) is tracked separately as #3258.