Contain backend OSErrors at the command-execution boundary (closes #9) - #11
Open
ninowalker wants to merge 1 commit into
Open
Contain backend OSErrors at the command-execution boundary (closes #9)#11ninowalker wants to merge 1 commit into
ninowalker wants to merge 1 commit into
Conversation
A custom filesystem backend that raises anything outside the three errors the commands recognize propagates out of bash.exec() as a Python exception instead of a nonzero exit. Network-backed backends have no exception to fail through at all. Add one except OSError clause to the try/finally in _execute_simple_command, which spans builtin dispatch, command dispatch and output redirection. It reports "<cmd>: <filename>: <strerror>" and exit 1, and only sees errors no command handled, so existing messages are unchanged. OSError only: the interpreter's control-flow exceptions derive from InterpreterError and cannot be swallowed, and a non-OSError means the backend is broken and should crash rather than become exit 1. Document the expected exception taxonomy for backend authors in the README.
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
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 #9. Companion to #10 — both come from writing a network-backed
IFileSystem, and neither depends on the other.The problem
Commands catch a narrow set:
catcatches exactlyFileNotFoundError,IsADirectoryError,PermissionError, and across the command tree there are only a handful ofexcept OSError. A backend that raises anything else propagates out ofbash.exec()as a Python exception rather than a nonzero exit.For a network-backed filesystem there is no exception to fail through at all — a connect timeout mid-
catbecomes a traceback out of the sandbox instead ofcat: /x: Connection timed out, exit 1. For the agent use case that breaks containment: a sandbox that raises through its own boundary isn't one, and every caller needs its owntry/exceptaroundexec()to avoid crashing on what should have been a failed command.The redirect path already gets this right —
_apply_redirectionswraps each redirect broadly and yields exit 1 — so the command path is the inconsistency, not the new behaviour.The change
One
except OSErrorclause on the existingtry/finallyin_execute_simple_command. That block already spans builtin dispatch, command dispatch and output redirection, so the clause needs no new structure and sees only errors no command handled — existing messages are unchanged. It reports<cmd>: <filename>: <strerror>and exit 1, matching how the coreutils phrase it.OSErrorand notException, deliberately. Three things follow from that choice:TimeoutErrorandConnectionErrorareOSErrorsubclasses, so network-backed backends get a natural way to fail.ExitError,BreakError,ReturnError, …) derives fromInterpreterError, notOSError, so it cannot be swallowed here.OSErroris a bug in the backend rather than a condition a shell can report, and should keep crashing loudly.The README gains a Custom Filesystems section stating the taxonomy, since #9 asked for either the fix or the documentation and the fix is only usable if implementers know the contract.
Tests
tests/test_interpreter/test_fs_error_boundary.py— 10 tests over a backend that raises on demand: each of the three recognized errors still produces its existing message (no regression in phrasing), a bareOSError/TimeoutError/ConnectionErrorbecomes exit 1 with the detail on stderr, a non-OSErrorstill propagates, and control-flow exceptions are unaffected.Full suite on this branch: 700 failed, 4548 passed, 183 skipped.
Full suite on
main(48d111c): 700 failed, 4538 passed, 183 skipped.Same 700 pre-existing failures (the
awk/jqspec files), +10 from this PR.ruff checkon the touched source file reports the same 35 pre-existing findings before and after, so nothing new is added there either.