fix(sdk): surface backend errors in SandboxBackend.grep - #3471
Closed
Nikhil Reddy (Nik-Reddy) wants to merge 3 commits into
Closed
fix(sdk): surface backend errors in SandboxBackend.grep#3471Nikhil Reddy (Nik-Reddy) wants to merge 3 commits into
SandboxBackend.grep#3471Nikhil Reddy (Nik-Reddy) wants to merge 3 commits into
Conversation
`grep()` builds a shell pipeline that appends `2>/dev/null || true`, which masks grep's own non-zero exit codes. When the backend (e.g., a container exec) fails before the shell starts, the `||` never runs; the backend's error text lands in `result.output` with a non-zero `exit_code`, and the parser then crashes with `ValueError: invalid literal for int() with base 10: ' exec failed'` while trying to read the second `:`-delimited field as a line number. This mirrors the fix landed for ls/read/edit/glob: check `exit_code` first and surface a structured `GrepResult(error=...)` consistent with the other backend operations. As a defense-in-depth measure, the match-line parser also catches `ValueError` from the int conversion and returns the same structured error rather than raising. Closes langchain-ai#3441
github-actions
Bot
added
deepagents
langchain-oss-automated-triage
Bot
added
new-contributor
This comment has been minimized.
This comment has been minimized.
17 tasks
Author
|
Closing this in favor of #3637, which landed on main with the same exit_code-first plus structured GrepResult(error=...) approach. The additional grep -Z NUL-delimited filename change in #3637 is a nice orthogonal hardening for paths containing colons. Thanks for picking up the design and for the in-branch refinements on 7480fd6. The original root-cause writeup and test plan from 2026-05-19 lives on issue #3441 for the audit trail. Nothing left to merge here. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #3441
grep()masks its own non-zero exit codes with2>/dev/null || true, but when the backend (e.g., a container exec) fails before the shell starts, that redirection never takes effect: the backend's error text lands inresult.outputwith a non-zeroexit_code, and the match parser then crashes withValueError: invalid literal for int() with base 10: ' exec failed'while reading the second:-delimited field as a line number.This mirrors the convention already used by
ls/read/edit/glob(after #3359): checkexit_codefirst and return a structuredGrepResult(error=...)instead of raising.As defense in depth, the match parser also rejects any line that doesn't fit the
path:line:textshape — whether the line-number field isn't an integer or the line has no colons at all — surfacing a structured error instead of silently dropping the line (which would mask the failure as a no-match). When this happens, any matches accumulated before the malformed line are discarded, matchingglob()'s preference for a clean error over ambiguous partial output.LocalSubprocessSandboxis intentionally unaffected: its shell always runs, so|| truecontinues to force exit 0 on grep-side failures (no behavior change for the no-match-on-missing-path case there). The fix targets exactly the failure surface described in the issue — backend exec failures that produce output before the shell is reached.Note: Pierre Larochelle (@pierrel) mentioned on the issue that they had a fix ready. Happy to defer if their work is further along — closing this PR if so. Posting in parallel given the issue is two weeks old and the bug is currently blocking grep against any Docker-style sandbox with an unreachable path.