fix: handle missing fortune binary without raising - #292
Merged
Merged
Conversation
FileNotFoundError (OSError) from subprocess.check_output escaped the except subprocess.CalledProcessError handler, so a fortune binary that was removed between setup() and process() propagated an uncaught exception. TimeoutExpired (hung binary) had the same symptom. Catch (TimeoutExpired, OSError) and answer with a graceful "Fortune command failed" reply instead. Addresses review finding in docs/code-review-2026-09-18.md.
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
1 participant
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.
Summary
FortunePlugin.process()only caughtsubprocess.CalledProcessError. A fortunebinary that is removed between
setup()andprocess()makessubprocess.check_outputraiseFileNotFoundError(anOSError), which was notcaught and propagated out of the plugin. The same symptom applied to a hung
binary raising
subprocess.TimeoutExpired(note:timeout=3is already set).This was found during the 2026-09-18 code review (LOW severity finding) —
docs/code-review-2026-09-18.md. Plugin manager containment keeps the daemonalive either way, but the operator lost the graceful APRS reply.
Change
(subprocess.TimeoutExpired, OSError)separately (neither is aCalledProcessError) and answer"Fortune command failed: ..."instead.ex.outputonly exists onCalledProcessError, so the new branch formats{ex}(e.g.
[Errno 2] No such file or directory: 'fortune').Tests
test_fortune_missing_binary_returns_graceful_reply—FileNotFoundErrorproducesa reply, does not raise.
test_fortune_timeout_returns_graceful_reply—TimeoutExpiredproduces a reply.Full suite: 751 passed.
ruff check aprsd/ tests/: clean.