fix(harness): make ls() report errors instead of silently returning empty results - #2413
fix(harness): make ls() report errors instead of silently returning empty results#2413chcodex wants to merge 2 commits into
Conversation
…mpty results (agentscope-ai#2411) LocalFilesystem.ls() and BaseSandboxFilesystem.ls() returned success(empty list) when the path did not exist or was a file, making it impossible for callers to distinguish these cases from an empty directory. Changes: - LocalFilesystem.ls(): return fail() with descriptive error for non-existent paths and file paths instead of success(empty) - BaseSandboxFilesystem.ls(): add shell-level path checks before the glob loop, parse sentinel values in Java, return fail() - FilesystemTool.listFiles(): change message from "Empty or not a directory" to "Empty directory" since the other two cases are now caught as errors at the fs layer - Add reproduction/regression tests for all three layers
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
oss-maintainer
left a comment
There was a problem hiding this comment.
Code Review — Approved ✅
Overall: Good fix for #2411. The PR correctly changes ls() to return proper error results instead of silently returning empty lists when the path doesn't exist or is not a directory.
Key Changes:
-
LocalFilesystem.ls(): Now returnsLsResult.fail(...)with descriptive messages for non-existent paths and non-directory paths, instead ofLsResult.success(List.of()). -
BaseSandboxFilesystem.ls(): Added shell-level checks ([ ! -e ... ]and[ ! -d ... ]) with sentinel values (__NOT_EXISTS__,__NOT_A_DIR__) to detect error conditions in the sandbox environment. -
FilesystemTool.listFiles(): Updated the empty result message from "Empty or not a directory" to "Empty directory" since the error cases are now handled upstream. -
Tests: Added two new test cases verifying that
ls()returns failure for non-existent paths and file paths.
Code Quality:
- ✅ Correct error handling — distinguishes between "path doesn't exist" and "not a directory"
- ✅ Good test coverage
- ✅ Sandbox implementation uses sentinel values to communicate errors across the execution boundary
- ✅ Backward compatible for valid directory paths
CI Status: ✅ All checks passed
Verdict: Well-implemented fix that correctly addresses the silent error swallowing issue. The error messages are clear and actionable.
Fixes #2411
Summary
Three-layer cascade:
LocalFilesystem.ls()andBaseSandboxFilesystem.ls()silently returnsuccess(empty list)when a path does not exist or is a file (not a directory). This makesFilesystemTool.listFiles()unable to distinguish "not found", "not a directory", and "empty directory" — it returns the same misleading"Empty or not a directory: ..."message for all three.Changes
LocalFilesystem.ls()Split the single
||check into two separate conditions:!Files.exists(dirPath)→LsResult.fail("Path does not exist: ...")!Files.isDirectory(dirPath)→LsResult.fail("Not a directory: ...")BaseSandboxFilesystem.ls()Added shell-level path existence/directory checks before the glob loop:
Java parses the sentinel values and returns
LsResult.fail(...)accordingly.Also updated
FakeSandboxFilesystem.execute()command matching to match the new command prefix.FilesystemTool.listFiles()Changed the empty-entries message from
"Empty or not a directory"to"Empty directory"— since the other two cases are now caught at the filesystem layer as errors.Tests
Added 7 reproduction/regression tests across all three layers (previously
@Disabled, now active). All 36 tests pass with 0 failures.