Skip to content

ls() silently swallows errors in LocalFilesystem and BaseSandboxFilesystem, leading to misleading list_files messages #2411

Description

@chcodex

Summary

Three layers of bugs cascade: LocalFilesystem.ls() and BaseSandboxFilesystem.ls() silently return success(empty list) when the path does not exist or is not a directory. This makes it impossible for FilesystemTool.listFiles() to distinguish "not found", "not a directory", and "empty directory" — it returns the same misleading "Empty or not a directory: ..." message for all three.


Bug 1 — FilesystemTool.listFiles() line 184: ambiguous error message

FilesystemTool.java:183-184:

if (infos == null || infos.isEmpty()) {
    return "Empty or not a directory: " + path;
}

Three different scenarios (path does not exist / path is a file / directory is empty) all produce the same string. An agent consuming this tool cannot determine what to do next.

Bug 2 — LocalFilesystem.ls() lines 240-241: silently swallows errors

LocalFilesystem.java:238-241:

if (!Files.exists(dirPath) || !Files.isDirectory(dirPath)) {
    return LsResult.success(List.of());
}

When the path does not exist or is a regular file (not a directory), it returns success(empty list) instead of fail(...). This prevents the caller from distinguishing these cases from an empty directory.

Bug 3 — BaseSandboxFilesystem.ls() lines 75-113: shell glob + 2>/dev/null swallows all errors

BaseSandboxFilesystem.java:74-87 constructs a shell command:

String cmd = "for f in " + escapedPath + "/*; do "
    + "  if [ -d \"$f\" ]; then ... "
    + "  elif [ -f \"$f\" ]; then ... "
    + "  fi; "
    + "done 2>/dev/null";

When the path does not exist, the glob path/* expands to the literal string /path/*, none of the [ -d "$f" ]/[ -f "$f" ] conditions match, output is empty, and the method returns LsResult.success(empty list). The 2>/dev/null and unchecked exit code hide the failure entirely.


Reproduction tests

Run mvn test -pl agentscope-harness after enabling any of the @Disabled tests below to see the failures.

Tests for Bug 2 (LocalFilesystem.ls)

Add to LocalFilesystemModeTest.java:

@org.junit.jupiter.api.Disabled(
        "Bug #TODO: LocalFilesystem.ls returns success(empty) for non-existent/file paths")
@Test
void ls_nonExistentPath_shouldReturnFail(@TempDir Path workspace) {
    LocalFilesystem fs = new LocalFilesystem(workspace);
    LsResult r = fs.ls(RuntimeContext.empty(), "/this/path/does/not/exist");
    assertFalse(r.isSuccess(), "ls on non-existent path should fail");
}

@org.junit.jupiter.api.Disabled(
        "Bug #TODO: LocalFilesystem.ls returns success(empty) for file paths")
@Test
void ls_filePath_shouldReturnFail(@TempDir Path workspace) throws IOException {
    Path file = workspace.resolve("foo.txt");
    Files.writeString(file, "content");
    LocalFilesystem fs = new LocalFilesystem(workspace);
    LsResult r = fs.ls(RuntimeContext.empty(), file.toAbsolutePath().toString());
    assertFalse(r.isSuccess(), "ls on a file path should fail");
}

Both fail with: expected: <false> but was: <true> — proving ls() returns success(empty).

Tests for Bug 3 (BaseSandboxFilesystem.ls)

Add to BaseSandboxFilesystemTest.java (inside LocalShellIntegrationTests):

@org.junit.jupiter.api.Disabled(
        "Bug #TODO: BaseSandboxFilesystem.ls shell glob + 2>/dev/null swallows errors")
@Test
void ls_nonExistentPath_shouldReturnFail() {
    LocalShellSandboxFilesystem fs = new LocalShellSandboxFilesystem();
    LsResult r = fs.ls(RT, "/this/path/does/not/exist/at/all");
    assertFalse(r.isSuccess(), "ls on non-existent path should fail");
}

@org.junit.jupiter.api.Disabled(
        "Bug #TODO: BaseSandboxFilesystem.ls shell glob + 2>/dev/null swallows errors")
@Test
void ls_filePath_shouldReturnFail() throws IOException {
    Path file = tmpDir.resolve("file.txt");
    Files.writeString(file, "content");
    LocalShellSandboxFilesystem fs = new LocalShellSandboxFilesystem();
    LsResult r = fs.ls(RT, file.toAbsolutePath().toString());
    assertFalse(r.isSuccess(), "ls on a file path should fail");
}

Both fail with: expected: <false> but was: <true> — proving the shell-based ls() returns success(empty).

Tests for Bug 1 (FilesystemTool.listFiles)

Add to FilesystemToolTest.java:

@org.junit.jupiter.api.Disabled(
        "Bug #TODO: need distinct messages for empty-dir / not-found / not-a-dir")
@Test
void listFiles_ambiguousMessageForNonExistentPath() {
    when(filesystem.ls(RT, "/nonexistent")).thenReturn(LsResult.success(List.of()));
    String result = tool.listFiles(RT, "/nonexistent");
    assertEquals("Empty or not a directory: /nonexistent", result);
}

@org.junit.jupiter.api.Disabled(
        "Bug #TODO: need distinct messages for empty-dir / not-found / not-a-dir")
@Test
void listFiles_ambiguousMessageForFilePath() {
    when(filesystem.ls(RT, "/path/to/file.txt")).thenReturn(LsResult.success(List.of()));
    String result = tool.listFiles(RT, "/path/to/file.txt");
    assertEquals("Empty or not a directory: /path/to/file.txt", result);
}

@org.junit.jupiter.api.Disabled(
        "Bug #TODO: need distinct messages for empty-dir / not-found / not-a-dir")
@Test
void listFiles_ambiguousMessageForEmptyDirectory() {
    when(filesystem.ls(RT, "/empty/dir")).thenReturn(LsResult.success(List.of()));
    String result = tool.listFiles(RT, "/empty/dir");
    assertEquals("Empty or not a directory: /empty/dir", result);
}

These pass today (they assert the current buggy behaviour) but will need to be updated once Bugs 2 and 3 are fixed, because ls() will then return fail() for invalid paths and listFiles will return a proper "Error: ...".


Fix approach

  1. LocalFilesystem.ls() — replace the single || check with two separate conditions: !Files.exists(dirPath)fail("Path does not exist"), !Files.isDirectory(dirPath)fail("Not a directory").

  2. BaseSandboxFilesystem.ls() — prefix the shell command with if [ ! -e path ]; then echo __NOT_EXISTS__; elif [ ! -d path ]; then echo __NOT_A_DIR__; else .... Parse the sentinel values in Java and return LsResult.fail(...) accordingly. Also update FakeSandboxFilesystem.execute() command matching.

  3. FilesystemTool.listFiles() — once (1) and (2) are done, success(empty) only occurs for genuine empty directories. Change the message from "Empty or not a directory" to "Empty directory".

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions