fix(sandbox): stop list_dir from reporting failures as empty

[wutongyuonce]

Fixes #5263

Why

Remote sandbox list_dir swallowed find/client errors as [], and find ... 2>/dev/null on a missing path produced empty stdout. ls_tool then told the agent the directory was (empty).

That is a lying result, not an empty folder: a dead sandbox, a closed client, or a path that does not exist all look like a writable empty directory. The agent can then write into a tree that already has files, or skip recovery.

LocalSandbox had the same missing-path lie (list_dir returned [] when the root was not a directory), so the abstract contract was inconsistent across providers.

What changed

• Remote list_dir (AIO, E2B, Boxlite, OpenSandbox, Tenki) raises OSError on command/client failure and FileNotFoundError when find prints nothing, so ls returns Error: ....
• find -H dereferences only the start point, so a symlink-to-dir (E2B /mnt/acp-workspace) still lists. BusyBox find (Boxlite/Tenki) supports -H.
• AIO treats a missing result.data as OSError, not FileNotFoundError, and groups \( -type f -o -type d \) so -type d is bound by -maxdepth.
• Local list_dir raises FileNotFoundError when the path is not a directory. A truly empty existing directory can still return (empty).
• The raise contract is documented on abstract Sandbox.list_dir and in sandbox/AGENTS.md.

Surface area

• Frontend UI — page / component / setting / interaction under frontend/
• Backend API — endpoint / SSE event / request-response shape under backend/app
• Agents / LangGraph — agent node, graph wiring, langgraph.json, or prompt change
  • Prompt-layer self-check: for every data source in the new text, what is its trust level, and which channel should it ride? Model-supplied or user-influenceable values belong on the untrusted, sanitized data channel (e.g. the task HumanMessage) — never interpolated into framework-owned system text, even neutralized.
• Sandbox — docker/ or sandboxed execution
• Skills — change under skills/
• Dependencies — new/upgraded entry in backend/pyproject.toml or frontend/package.json (say what it buys us)
• Default behavior change — changes existing behavior without the user opting in (default model, default setting, data shape)
• Docs / tests / CI only — no runtime behavior change

Screenshots / Recording

Not applicable; this is a backend sandbox bug fix.

Bug fix verification

• Test path that reproduces the bug:
  • tests/test_e2b_sandbox_provider.py::test_list_dir_raises_when_command_fails
  • tests/test_e2b_sandbox_provider.py::test_list_dir_raises_when_find_returns_no_entries
  • tests/test_aio_sandbox.py::TestListDirSerialization::test_list_dir_raises_when_exec_fails
  • tests/test_local_sandbox_provider_mounts.py::TestSymlinkEscapes::test_list_dir_raises_when_path_is_missing
• Did it go red on main and green on this branch? yes (the original remote empty-stdout / swallowed-exception cases; local missing-path was added in review follow-up)
• Companion coverage: AIO result.data is None → OSError; AIO find type predicates grouped; E2B/Boxlite find -H; OpenSandbox/Tenki empty-find → FileNotFoundError.

Validation

• ruff check and ruff format --check on the touched sandbox/test files
• pytest on the list_dir cases in test_e2b_sandbox_provider.py, test_aio_sandbox.py, test_boxlite_provider.py, test_opensandbox_provider.py, test_tenki_provider.py, test_local_sandbox_provider_mounts.py — 33 targeted tests passed

AI assistance

Tool(s) used: Pi Grok4.6

How you used it: Source tracing, test-first implementation, review follow-up, and PR writing.

• I've read and understand every line of this change and take responsibility for it — it's not unreviewed AI output.

[willem-bd]

The direction is right — swallowing find/client errors into [] made ls_tool report "(empty)" for broken sandboxes, and raising is the fix. ls_tool already handles both new failure modes (FileNotFoundError -> "Directory not found", OSError/other -> generic error), and list_dir has no other production caller, so the blast radius is contained. Two things worth a second look: (1) the "empty stdout == missing path" invariant is not true when the start point is a symlink to a directory, which E2B actually creates for /mnt/acp-workspace — a path the lead prompt explicitly tells the model to ls; (2) the default LocalSandbox still reports a missing path as "(empty)", so the bug this PR targets remains on the local provider and the abstract contract is now inconsistent across providers.

[willem-bd]

The new invariant in the comment above is not always true: find defaults to -P, so it does not dereference the start point. When resolved is a symlink to a directory, the start point has type l, neither -type f nor -type d matches, stdout is empty — and this now raises FileNotFoundError for a directory that exists and is listable.

This is not hypothetical in this repo: the E2B bootstrap creates /mnt/acp-workspace as a symlink (sudo ln -sfn {home_dir}/acp-workspace /mnt/acp-workspace, e2b_sandbox_provider.py ~line 1927), and _resolve_path only rewrites /mnt/user-data, so /mnt/acp-workspace reaches find verbatim as a symlink. The lead-agent prompt explicitly instructs the model: "ACP agent results are accessible at /mnt/acp-workspace/ (read-only) — use ls ..." (backend/packages/harness/deerflow/agents/lead_agent/prompt.py:990). With this change that ls returns "Error: Directory not found: /mnt/acp-workspace" instead of a listing.

Suggested fix: find -H {path} ... — dereference only the start point. Verified locally that -H still prints an existing empty directory itself (so the empty-dir case stays distinguishable) and still yields empty stdout for a genuinely missing path. Any agent-created symlink to a directory inside the workspace hits the same thing, and the same applies to the other four providers (for tenki please confirm the busybox find applet supports -H, or use -follow).

[wutongyuonce]

Agreed — find defaults to -P, so a symlink start point has type l and the new empty-stdout check would raise FileNotFoundError for a directory that exists. That is exactly E2B /mnt/acp-workspace.

Switched all five remote list_dir invocations to find -H (dereference only the start point). BusyBox find documents find [-HL], so the same flag is portable on Boxlite/Tenki. Empty-dir vs missing-path still holds: -H still prints an existing directory itself.

[willem-bd]

The default provider still has the exact bug this PR fixes: deerflow/sandbox/local/list_dir.py:22 returns [] when the root is not a directory, so on LocalSandbox ls /mnt/user-data/missing still renders as "(empty)" — a misleading answer, and now inconsistent with the five remote providers that say "Directory not found".

Two small additions would make this change coherent:

1. Make the local list_dir helper raise FileNotFoundError when root_path is not a directory (it already has the distinguishing information in hand).
2. Document the raise contract on the abstract Sandbox.list_dir (backend/packages/harness/deerflow/sandbox/sandbox.py:170). download_file right above it sets the precedent: "Both local and remote implementations must raise OSError...". Right now a third-party provider implementing the interface has no way to know empty-list-is-not-allowed is now part of the contract, and backend/packages/harness/deerflow/sandbox/AGENTS.md describes the interface without it.

[wutongyuonce]

Raised FileNotFoundError from the local helper when root_path is not a directory, documented the raise contract on abstract Sandbox.list_dir (same shape as download_file), and noted empty-list-is-not-allowed in sandbox/AGENTS.md.

ls /mnt/user-data/missing now returns Error: Directory not found on LocalSandbox too. A truly empty existing directory still returns [] → (empty).

[willem-bd]

result.data being None is a transport/SDK anomaly (the shell call succeeded but returned no payload), not a missing directory — yet it now surfaces as FileNotFoundError(path), which ls_tool renders as "Error: Directory not found". Consider raising OSError for that specific branch so the two failure classes stay distinguishable, e.g. only treat empty stdout as missing-path when result.data is present.

Minor consistency nit: this provider formats the exception with the caller-supplied path while the other four use resolved. Harmless today (only ls_tool consumes it and it overrides the message), but worth aligning.

[wutongyuonce]

result.data is None now raises OSError (transport/SDK anomaly), and empty stdout with a present payload still raises FileNotFoundError. Exception text uses resolved; AIO has no path mapping, so resolved = path.

Follow-up: AIO find now groups \( -type f -o -type d \) the same way as the other providers. Without parentheses, -type d was not bound by -maxdepth and files could be dropped from the listing.

[willem-bd]

opensandbox and tenki get the same behavior change but no equivalent coverage — only aio, boxlite and e2b were extended. Since the raise now depends on each provider's exact find invocation and stdout parsing (and tenki also maps every line through _virtual_path before the emptiness check), a pytest.raises(FileNotFoundError) case in backend/tests/test_opensandbox_provider.py and backend/tests/test_tenki_provider.py would pin the contract per provider, per the TDD rule in AGENTS.md.

[wutongyuonce]

Added pytest.raises(FileNotFoundError) for empty find on OpenSandbox and Tenki. The fake find parsers skip the new -H/-L/-P flags so existing listing tests still resolve the root path. Tenki still maps lines through _virtual_path before the emptiness check.

[wutongyuonce]

Thanks @willem-bd — addressed in 9749c6f and 5f2ccf1:

1. find -H on all five remote providers so a symlink start point (E2B /mnt/acp-workspace) still lists. BusyBox find supports -H.
2. Local list_dir raises FileNotFoundError when the root is not a directory; raise contract documented on abstract Sandbox.list_dir and in sandbox/AGENTS.md.
3. AIO result.data is None → OSError; empty stdout with a payload still → FileNotFoundError.
4. FileNotFoundError coverage added for OpenSandbox and Tenki.
5. AIO find now groups \( -type f -o -type d \) like the other providers, so -type d is bound by -maxdepth and files are not dropped.

PR description is filled in against #5263.

[willem-bd]

Thanks for the fast turnaround — all four prior findings check out at 760a3f7: find -H is applied to all five remote list_dir calls, LocalSandbox raises on a non-directory root, AIO maps a missing result.data to OSError, and OpenSandbox/Tenki now have the FileNotFoundError case covered. I re-validated -H empirically (symlink start point lists, a truly empty dir still prints itself, a missing path stays empty) and the touched suites pass (518 tests; the one test_local_sandbox_provider_mounts failure I hit is pre-existing on origin/main, unrelated to this PR). One remaining consistency gap below, non-blocking.

[willem-bd]

Non-blocking: this branch treats any empty stdout as "missing path", but 2>/dev/null also swallows an invocation that never produced output — sh -lc exiting 127 (no find binary) or an SDK-level error lands in the same FileNotFoundError(resolved) for a live sandbox.

The status is already on the result object and this file reads it elsewhere (execute_command's result.exit_code check), so the same distinction this PR drew for AIO's result.data is None → OSError is available here. Boxlite and OpenSandbox share the gap: box.py's _sh result carries exit_code, and opensandbox/sandbox.py already reads execution.exit_code / execution.error in format_execution.

Concretely: raise OSError when the exit status is outside (0, 1) — 1 legitimately means find ran and the start point is absent — and reserve FileNotFoundError for the find-ran-but-empty case. Otherwise the OSError half of the contract just added to Sandbox.list_dir is unreachable in these three providers, and the new "empty stdout is the missing-path case" comment overclaims.

[wutongyuonce]

Addressed in add15b7.

list_dir on Tenki, Boxlite, and OpenSandbox now raises OSError when find exit status is outside (0, 1) (OpenSandbox also maps execution.error to OSError). FileNotFoundError is reserved for the find-ran-but-empty case. Added a 127-exit coverage case in each of the three suites.

[willem-bd]

Reviewed at 62baa32. The six affected provider suites passed with 524 tests and 1 skip. An additional shell-level reproduction exposes a remaining error-classification issue in the listing pipeline, detailed inline.

[willem-bd]

[P2] Check find's status, not the pipeline's status

The command above ends with | head -500, and _sh() invokes sh -lc without enabling pipeline failure propagation. Its returned exit code is therefore normally head's status, not find's. If find fails with 127 while head succeeds, this check sees 0 and raises FileNotFoundError for an existing directory, so ls_tool incorrectly reports "Directory not found" instead of a command failure.

I reproduced this through the BoxLite adapter by executing its actual shell pipeline with a shell function standing in for find and returning 127: the pipeline returned 0 and the adapter raised FileNotFoundError. The same pipeline pattern is used by all five remote providers. The new tests inject exit_code=127 directly into SDK results, which bypasses the shell behavior and cannot catch this.

Please capture find's exit status independently while retaining bounded output, distinguish real missing paths from command failures, and add a shell-level regression test. If using pipeline failure propagation, also account for intentional truncation/SIGPIPE from head rather than turning large successful listings into errors.

[wutongyuonce]

Thanks @willem-bd — addressed in 2dd09fc.

find ... | head under sh -lc was reporting head's 0, so find 127 looked like a missing directory. Shared helper sandbox/remote_list_dir.py now:

1. Writes find's own $? to a tempfile on the find side of the pipe, then prints __DF_FIND_STATUS__:N after head, so a 500-line listing cannot truncate the marker.
2. Treats exit 0/1 as before, and 141 (SIGPIPE from head closing the pipe) as successful truncation.
3. Starts with set +e so a login-profile errexit cannot skip the status write.
4. Shell-level regression: a stub find exiting 127 through the real pipeline is OSError, not FileNotFoundError. A 5000-line listing truncated by head still returns 500 entries.

Provider suites + the new helper tests: 484 passed, 1 skipped.

[willem-bd]

Re-reviewed the delta since 62baa32 (2dd09fc plus the CI test fix in b172d9f). The prior P2 is genuinely fixed: the generated script records find's own $? after head, 141 is treated as successful truncation, set +e defeats a login-profile errexit, and the shell-level regression in tests/test_remote_list_dir.py reproduces the head-0-masks-find-127 case. All six touched suites pass at the head (484 passed, 1 skipped). One residual gap in the new fallback path, inline.

[willem-bd]

[P2] The pipeline_exit_code fallback can never report a failure, because the script's exit status is rm's, not the pipeline's.

Every provider forwards the sh -lc process status as pipeline_exit_code, and this script's last command is rm -f "$_st". I ran the generated script both ways: a normal run exits 0, and when the status file cannot be written it exits 1 (rm -f still reports EACCES). Both are in _FIND_OK, so whenever the marker is absent or unparsable the parser always lands on "success" and an empty listing becomes FileNotFoundError(resolved).

That silently reinstates exactly the 127 -> "Directory not found" misclassification this commit exists to fix, with no signal. Triggers are plausible: /tmp read-only or absent in a hardened sandbox image, an exec layer that truncates or drops the tail of stdout (OpenSandbox's execution_stdout reassembles streamed log events), or any SDK that discards the final line.

Two ways out, either is fine:

• Make the two signals agree, reserving a non-OK status for "status unknown": capture once with st=$(cat "$_st" 2>/dev/null), keep the printf, then end with rm -f "$_st"; exit "${st:-126}". 126 is outside _FIND_OK, so a missing marker now raises OSError, while a real 0/1/141 still parses.
• Or drop the fallback in parse_remote_list_dir_output and raise OSError whenever no marker was found. Loud beats silently wrong here, since the whole point of the helper is to distinguish the two failure classes.

Worth a sibling of test_parse_marker_127_is_command_failure_not_missing_path that passes a real subprocess exit code with no marker, so the fallback cannot regress back to always-OK.

[wutongyuonce]

@willem-bd you're right — the script ended on rm -f, so process status was 0/1 (both in _FIND_OK) whenever the marker was missing. The fallback then turned an empty listing into FileNotFoundError, which is the 127 misclassification this helper was supposed to close.

Fixed both sides in 5bf2490:

• Script now does st=$(cat "$_st" 2>/dev/null); …; rm -f "$_st"; exit "${st:-126}" so the process status is find's (126 if unknown).
• Parser no longer treats a missing marker as success. Non-OK pipeline_exit_code still surfaces as OSError with that code; 0/1/141/None without a marker is OSError (marker missing).

Added test_parse_without_marker_real_subprocess_status_is_not_always_ok (sh -c 'exit 0' / exit 1') and pinned proc.returncode == 127 on the generated script.

[willem-bd]

Re-reviewed the delta since 7bf5f4b (5bf2490). The round-3 P2 is genuinely fixed, and I verified it empirically rather than by inspection: the generated script now exits with find's own status (reproduced 0 for an existing dir, 1 + FileNotFoundError for a missing path, 127 and 141 through the real pipeline), an unwritable status file yields exit 126 with an empty marker, and a stdout tail that loses the marker raises OSError for every process status including 0 — so the 127 -> "Directory not found" misclassification can no longer come back through the fallback. One new problem, and it is blocking: this push leaves 18 tests in this PR's own touched suites failing at head. Inline.

[willem-bd]

[P1] This branch now reds its own provider suites at 5bf2490: 18 tests fail with OSError: ... find status marker missing, all because the provider fixtures still stub bare find stdout (no __DF_FIND_STATUS__: line) with exit_code=0. The parser change itself is right — marker-less output must not read as success — but the fixtures were not moved to the new contract, so the suites cannot validate this commit.

Reproduced at the head worktree (backend tests, 19 failed / 597 passed / 1 skipped):

• test_aio_sandbox.py x5 — TestListDirSerialization::test_list_dir_uses_lock (:601), ::test_list_dir_raises_when_find_returns_no_entries (:623), ::test_list_dir_uses_find_H (:641), TestNoChangeTimeout::test_list_dir_passes_no_change_timeout (:687), test_list_dir_preserves_trailing_space_in_filename (:937)
• test_boxlite_provider.py x3 — :1307, :1323, :1346
• test_e2b_sandbox_provider.py x3 — test_list_dir_preserves_trailing_space_in_filename, test_list_dir_raises_when_find_returns_no_entries (:5224), test_list_dir_uses_find_H_to_dereference_start_point
• test_opensandbox_provider.py x3 — test_list_glob_and_grep_return_virtual_paths (:603) and the two test_list_dir_* cases
• test_tenki_provider.py x4 — :548, :630, :1023, :1039

(The 19th failure, test_local_sandbox_provider_mounts.py::TestReadOnlyPath::test_bash_write_to_projected_copy_does_not_mutate_source, also fails on origin/main — pre-existing, not this PR.) The same six suites pass at 7bf5f4b (484 passed, 1 skipped), so the red is introduced by this push; the helper additions in test_remote_list_dir.py are the only tests that were updated.

Fix: make each fixture emit the marker and keep exit_code in sync — stdout="...\n\n__DF_FIND_STATUS__:0\n" for listings, "\n__DF_FIND_STATUS__:1\n" for the empty-listing/missing-path cases — or better, drive the fixtures through the real remote_list_dir_command(...) executed against a tmp dir the way test_remote_list_dir.py does, so the generated command and its stubs cannot drift apart on the next contract change.

[wutongyuonce]

Thanks @willem-bd — confirmed. The parser change is right; these fixtures were still stubbing bare find stdout, so they all died on find status marker missing.

Fixed in 30b86f4:

• AIO / Boxlite / E2B stubs now emit __DF_FIND_STATUS__:0 on listings and :1 (exit 1) on empty/missing-path cases.
• OpenSandbox / Tenki in-memory find fakes append the marker only when the list_dir command is in play (__DF_FIND_STATUS__: in the script), so glob still gets raw find stdout.

Re-ran the six suites at 30b86f4: 485 passed, 1 skipped.

[willem-bd]

Round-5 re-review at 30b86f4. The round-4 blocker is resolved: all 18 provider fixtures (aio x5, boxlite x3, e2b x3, opensandbox x3, tenki x4) now emit __DF_FIND_STATUS__ with a matching exit code, and I re-ran this PR's seven touched suites at this head — 536 passed, 1 skipped, 1 failed, and that one (test_local_sandbox_provider_mounts.py::TestReadOnlyPath::test_bash_write_to_projected_copy_does_not_mutate_source) reproduces identically on origin/main, so it is pre-existing and unrelated. Remote-only run: 485 passed, 1 skipped. I also ran the seven untouched suites that call list_dir (test_ensure_sandbox_initialized, test_local_sandbox_virtual_path_contract, test_sandbox_authorization, test_sandbox_leases, test_sandbox_middleware, test_sandbox_search_tools, test_three_way_skills_mount_e2e) — 194 passed, so the stricter marker-required parser breaks no other test fake. Every prior finding from rounds 1-4 stays fixed at this head (find-vs-head status, the non-rm exit status, the six exit paths, find -H, local FileNotFoundError, AIO result.data is None -> OSError). One remaining item, minor but it will red CI, inline.

[willem-bd]

[minor, CI-blocking] ruff format --check . fails on this new file with the pinned ruff 0.15.12 (backend/uv.lock), and the CI lint-backend job runs exactly that gate via backend/Makefile's make lint, so the lint workflow will red on this PR. ruff format wants the implicit string concatenation collapsed to one literal:

     fake_find.write_text(
-        "#!/bin/sh\n"
-        "i=1\n"
-        "while [ \"$i\" -le 5000 ]; do\n"
-        "  echo \"/dir/f$i\"\n"
-        "  i=$((i+1))\n"
-        "done\n"
-        "exit 0\n",
+        '#!/bin/sh\ni=1\nwhile [ "$i" -le 5000 ]; do\n  echo "/dir/f$i"\n  i=$((i+1))\ndone\nexit 0\n',
         encoding="utf-8",
     )

Verified empirically: ruff format --check . is clean on main (1363 files) and flags only this file, so the drift is introduced by this branch; ruff format tests/test_remote_list_dir.py fixes it. ruff check is clean on every touched file.

[wutongyuonce]

@willem-bd Confirmed and fixed in c1cab769. backend/tests/test_remote_list_dir.py now passes ruff format --check and ruff check. The focused suite also passes: 13 tests passed.

[willem-bd]

Round-6 re-review at c1cab76. The round-5 blocker is genuinely fixed: test_remote_list_dir.py is now clean under ruff format --check (pinned ruff 0.15.12) and the focused suite passes 13/13 at this head. The delta is a pure string-literal reformat with no behavior change. One remaining CI-gating item that my round-5 pass missed because I only checked the new test file — a second file still fails the same gate, detailed inline.

[willem-bd]

[CI-blocking] ruff format --check . still fails in backend/ at c1cab76 — on this file, not the test file. backend/Makefile's make lint (the lint-backend CI job) runs exactly that gate, so this blocks merge just like the round-5 finding did. I missed it last round by scoping the check to the new test file; this drift has actually been here since 2dd09fc.

Verified with the pinned ruff 0.15.12 (backend/uv.lock) against backend/ruff.toml (line-length = 240, quote-style = double):

$ cd backend && ruff format --check .
Would reformat: packages/harness/deerflow/sandbox/remote_list_dir.py
1 file would be reformatted, 1367 files already formatted

Two spots:

1. Here (L79-81): with line-length 240 the joined form fits on one line and there is no magic trailing comma, so ruff collapses the parenthesized call to raise OSError(f"Failed to list_dir {resolved}: command exited with code {pipeline_exit_code}").
2. L40: the single-quoted f-string with escaped single quotes around the printf newlines gets rewritten to a double-quoted literal, dropping the escapes.

ruff format packages/harness/deerflow/sandbox/remote_list_dir.py fixes both; ruff check is already clean on it.

[wutongyuonce]

@willem-bd Confirmed and fixed in 86518cc1. Reformatted backend/packages/harness/deerflow/sandbox/remote_list_dir.py; the full backend passes ruff format --check (1368 files), the target file passes ruff check, and test_remote_list_dir.py passes 13/13.

[willem-bd]

Round-7 re-review at 86518cc; the delta since 30b86f4 is exactly c1cab76 + 86518cc. The round-5/round-6 CI blocker is now fully resolved: ruff format --check . over backend/ with the pinned ruff 0.15.12 reports 1368 files already formatted, and ruff check on both touched files is clean. The delta is pure formatting with no behavior change — in sandbox/remote_list_dir.py one string literal switches from single to double quotes (identical content) and a multi-line raise OSError(...) collapses onto one line; in tests/test_remote_list_dir.py the inline fake_find script is joined into a single literal. I re-ran this PR's seven touched suites at this head with PYTHONPATH pinned to a worktree of 86518cc (deerflow verified to resolve there): 536 passed, 1 skipped, 1 failed, and the single failure is test_local_sandbox_provider_mounts.py::TestReadOnlyPath::test_bash_write_to_projected_copy_does_not_mutate_source, which I confirmed also fails on the current main lineage, so it is not introduced here. Nothing new to flag; the lint-backend gate should pass at this head.