Repository navigation
fix(sandbox): surface local grep file read failures - #7004
ranjianghong526-hash wants to merge 2 commits into
Conversation
github-actions
Bot
added
area:docs
ShenAC-SAC
left a comment
ShenAC-SAC
left a comment
There was a problem hiding this comment.
The underlying unreadable-file problem is real: the added regressions reproduce 11 failures on the pinned base, including actual non-root POSIX permission failures, and pass on this head. However, a real Unix-domain socket inside the searched workspace exposes the regression below. The related search suites pass (368 passed, 2 Windows-only skips); native Windows and full application E2E were not run in this review.
| except OSError as exc: | ||
| # A candidate may disappear after enumeration. Other failures leave | ||
| # the search incomplete and must not become a false no-match result. | ||
| if exc.errno in {errno.ENOENT, errno.ENOTDIR, errno.ELOOP}: | ||
| continue | ||
| raise |
There was a problem hiding this comment.
[P2] Exclude non-regular entries before propagating read errors
os.walk includes Unix-domain sockets in files, and the candidate path only checks size before is_binary_file tries to open it. With a readable target.txt containing needle and a real bound service.sock in the same workspace, the base returns the text match, but this head makes the public local grep tool return Error: Unexpected error searching file contents: OSError: [Errno 102] Operation not supported on socket on macOS. A service socket is not unreadable text, so its presence should not prevent searching the surrounding workspace. Check the candidate stat mode and skip non-regular files before binary sampling, while retaining the new error propagation for ordinary files; add a real-socket regression alongside the permission-error cases.
There was a problem hiding this comment.
Fixed in 776000b.
Thanks for catching this. Independently reproduced three times on Linux with a real bound Unix-domain socket: the existing PR head raises ENXIO while searching the surrounding directory. The new real-socket tests also fail on that head for both a matching and a no-match query.
The fix checks stat.S_ISREG using the stat result already needed for the size limit, and skips non-regular entries before binary sampling. Ordinary file read errors still propagate. The regression uses a real socket alongside a readable text file and verifies both the public grep tool and the underlying search; a short relative bind path avoids the Unix socket pathname limit. Updated the scoped guidance accordingly.
Validation: Linux related local/remote search suites 441 passed / 6 skipped; non-root Linux I/O regressions 28 passed / 2 Windows-only skips (including real socket and mode-000 permission tests); Windows related suites 221 passed / 226 POSIX/platform skips, including actual exclusive-share failures. The three standalone socket probes pass after the fix. Full backend Ruff and formatting checks passed (2,133 files); guidance validation has zero errors and two existing soft-budget warnings. No full-backend test-suite, macOS, live-provider or browser E2E run is claimed for this follow-up.
willem-bd
left a comment
willem-bd
left a comment
There was a problem hiding this comment.
The core fix is sound and well-tested: binary-sampling and text-read OSErrors now reach the tool handler instead of masquerading as binary exclusions or empty results, the ENOENT/ENOTDIR/ELOOP skip set keeps mid-scan disappearances best-effort, and the S_ISREG check (which also resolves the earlier socket finding) with the already-needed stat keeps syscall count flat. Three follow-ups below: the new PermissionError path reports only the requested root rather than the unreadable file (unlike the EIO path, whose sanitized message names the file), the disappearance set misses ESTALE/EISDIR races, and unreadable directories remain silently skipped so a mode-000 directory still yields a successful partial search.
| # the search incomplete and must not become a false no-match result. | ||
| if exc.errno in {errno.ENOENT, errno.ENOTDIR, errno.ELOOP}: | ||
| continue | ||
| raise |
There was a problem hiding this comment.
[P2] The surfaced PermissionError names the search root, not the unreadable file. This raise routes per-file PermissionError into grep_tool's except PermissionError: return f"Error: Permission denied: {requested_path}" branch, so when a child file is unreadable the message blames the readable root and names no file. Compare the non-permission OSError branch: its _sanitize_error-ed message includes the failing file (OSError: [Errno 5] ...: '/mnt/user-data/workspace/target.txt'), so the model can exclude it via glob or read_file it directly. Here the model gets zero matches, no offending name, and every retry over the tree fails identically — one mode-000 or foreign-owned file makes grep unusable on the whole workspace with no recovery hint. Consider surfacing the failing candidate's masked path in the PermissionError result (e.g. re-raise with the virtual path, or reuse _sanitize_error in that branch).
| except OSError as exc: | ||
| # A candidate may disappear after enumeration. Other failures leave | ||
| # the search incomplete and must not become a false no-match result. | ||
| if exc.errno in {errno.ENOENT, errno.ENOTDIR, errno.ELOOP}: |
There was a problem hiding this comment.
[P3] Two more races land on the raise path. The disappearance set is right for local filesystems, but: ESTALE — on NFS-mounted local workspaces a server-side delete mid-scan surfaces as a stale handle and now aborts the whole search; and EISDIR — a file replaced by a directory between stat and open (same window the ENOENT skip covers). Consider adding both; note errno.ESTALE is documented as Unix-only, so guard it (e.g. getattr(errno, "ESTALE", None)) to stay Windows-safe.
| continue | ||
| except OSError as exc: | ||
| # A candidate may disappear after enumeration. Other failures leave | ||
| # the search incomplete and must not become a false no-match result. |
There was a problem hiding this comment.
[P3] The no-false-no-match rationale stops at files. os.walk in candidate_files() still uses the default onerror=None, so an unreadable directory (mode-000, or a root the caller cannot list) silently truncates the walk and the tool returns a successful partial or empty result — exactly the failure mode this change removes for files. Consider an onerror handler applying the same skip/raise split, or at least documenting the asymmetry in the sandbox AGENTS.md note so the surfaced-errors claim doesn't over-promise.
3 participants
Why
Local
grepcan reportNo matches foundwhen it could not read the requested text.is_binary_file()converts any samplingOSErrorinto a binary-file exclusion; the surrounding search also suppresses later I/O failures. This hides permission/sharing errors and disk read failures as a successful empty or partial result.Reproduced against unchanged main
2464566942ace4ffe71d347fde3dd9868e2ecd5dwith actual Windows exclusive-share handles and non-root Linux mode-000 files. A direct open fails withPermissionError, while the existing search returns zero matches andtruncated=False. Both platforms reproduce 3/3; restoring access finds the same matching text. This is an offline filesystem reproduction, not a reported production incident.What changed
ENOENT,ENOTDIR,ELOOP); propagate other failures to the existing tool error handler.Surface area
Bug fix verification
backend/tests/test_local_grep_io_errors.py: final suite on unchanged Linux main, run as a real non-root user: 11 failed, 15 passed, 2 Windows skips. Windows initial regression version: 10 failed, 11 passed, 2 POSIX skips; it preceded the extra disappearance and mid-read cases. The same production tools and real OS errors are exercised. Portable error injection additionally covers deterministic sampling/text-open/I/O paths. No assertion or production-platform spoofing is used.Validation
2464566942ace4ffe71d347fde3dd9868e2ecd5d; Linux Python 3.12.15, uv 0.11.1, Node 24.16.0, Redis 7.4.11 and PostgreSQL 17. Dependencies synchronized withuv sync --frozen --all-packages --group dev --extra redis --extra postgres.make test-shard SPLITS=4 GROUP=<1..4>groups ran sequentially before and after, with the original exclusions and no modified assertions/timeouts. Baseline: 32,365 passed. Patch: 32,389 passed, zero failures. The baseline has 256 raw skip records (238 unique IDs); the patch has 260 (242 unique IDs). Each includes 18 repeated module-collection skip records. The four extra unique skips are the two native Windows/non-root POSIX pairs in the root Linux suite, exercised on their proper hosts below. Existing optional/provider/platform skips are not claimed as executed.make test-blocking-io: 256 passed, 1 skipped, before and after.make lint: passed, including format check of 2,133 Python files.git diff --check: passed. Guidance check: exit 0, no errors, two existing soft-budget warnings (below hard limits).9c74b1529b6e424b9c2e3c1b1fb6d25fdcea0e9awith an unrelated streaming-usage change. The patch applies cleanly there; the complete test counts above belong to the pinned baseline, not to that later commit's newly added tests.AI assistance
Tool(s) used: Codex.
How you used it: Source/related-work investigation, real OS reproduction, regression tests, minimal fix and local validation, and this draft.