fix: redirect stdin to /dev/tty for interactive read commands in pipe…#36
Open
fix: redirect stdin to /dev/tty for interactive read commands in pipe…#36
Conversation
Contributor
Summary by CodeRabbit
WalkthroughAll interactive Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@setup_payram.sh`:
- Around line 3716-3718: The current stdin redirection line ([[ -t 0 ]] || exec
< /dev/tty) can abort the script in non-TTY environments; change it to first
check that /dev/tty exists and is readable before attempting the exec so
non-interactive CI/cron/systemd runs won't fail. Replace the unconditional exec
branch with a guarded test (e.g., ensure /dev/tty exists and is a character
device or is readable) and only run exec < /dev/tty when that check and the
existing [[ -t 0 ]] test indicate a real tty; update the line containing [[ -t 0
]] || exec < /dev/tty accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
setup_payram.sh
Outdated
Comment on lines
+3716
to
+3718
| # When the script is piped (curl | bash), stdin is the pipe — not the terminal. | ||
| # Redirect stdin to /dev/tty so all interactive read commands work correctly. | ||
| [[ -t 0 ]] || exec < /dev/tty |
Contributor
There was a problem hiding this comment.
Guard /dev/tty redirection to avoid hard failure in non-TTY environments.
Line 3718 can terminate the script under set -e when /dev/tty is unavailable (common in CI/cron/systemd). That breaks otherwise non-interactive paths.
Suggested fix
- [[ -t 0 ]] || exec < /dev/tty
+ if [[ ! -t 0 ]]; then
+ # When piped, restore interactive stdin only if a controlling TTY exists.
+ if [[ -r /dev/tty ]]; then
+ exec < /dev/tty
+ fi
+ fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # When the script is piped (curl | bash), stdin is the pipe — not the terminal. | |
| # Redirect stdin to /dev/tty so all interactive read commands work correctly. | |
| [[ -t 0 ]] || exec < /dev/tty | |
| # When the script is piped (curl | bash), stdin is the pipe — not the terminal. | |
| # Redirect stdin to /dev/tty so all interactive read commands work correctly. | |
| if [[ ! -t 0 ]]; then | |
| # When piped, restore interactive stdin only if a controlling TTY exists. | |
| if [[ -r /dev/tty ]]; then | |
| exec < /dev/tty | |
| fi | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@setup_payram.sh` around lines 3716 - 3718, The current stdin redirection line
([[ -t 0 ]] || exec < /dev/tty) can abort the script in non-TTY environments;
change it to first check that /dev/tty exists and is readable before attempting
the exec so non-interactive CI/cron/systemd runs won't fail. Replace the
unconditional exec branch with a guarded test (e.g., ensure /dev/tty exists and
is a character device or is readable) and only run exec < /dev/tty when that
check and the existing [[ -t 0 ]] test indicate a real tty; update the line
containing [[ -t 0 ]] || exec < /dev/tty accordingly.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
setup_payram.sh (1)
775-3898:⚠️ Potential issue | 🟠 MajorGuard
/dev/ttyreads to avoid hard exits when no controlling TTY exists.All the new
read ... </dev/ttycalls will abort the script underset -eif/dev/ttyis unavailable (common in CI/cron/systemd or detached sudo sessions). Please centralize this with a guarded helper and fail with a clear message instead of redirection errors.Suggested fix pattern
+tty_read() { + if [[ ! -r /dev/tty ]]; then + log "ERROR" "Interactive input required, but no controlling TTY is available." + return 1 + fi + read "$@" </dev/tty +} ... - read -e -p "Select option (1-2): " choice </dev/tty + tty_read -e -p "Select option (1-2): " choice#!/bin/bash # Verify all direct /dev/tty reads and whether a guard/helper exists. rg -n 'read\b.*</dev/tty' setup_payram.sh rg -n 'tty_read\(|\[\[ *-r */dev/tty *\]\]|exec *< */dev/tty' setup_payram.sh🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@setup_payram.sh` around lines 775 - 3898, Many interactive reads use direct redirection "read ... </dev/tty" which will cause the script to abort under set -e when /dev/tty is unavailable (CI/cron/daemon). Add a centralized guarded helper (e.g. check_tty_or_fail and tty_read wrapper) that verifies /dev/tty is readable and either performs the read or prints a clear error and exits non-zero; then replace all raw uses of "read ... </dev/tty" across interactive entry points (e.g. configure_database, configure_external_database, configure_internal_database, configure_ssl, configure_ssl_letsencrypt, configure_ssl_custom, configure_ssl_update*, generate_aes_key, show_interactive_menu, show_network_selection, reset_payram_environment, display_firewall_notice, display_access_urls, and any other functions) to call the wrapper; ensure non-interactive helper functions keep returning graceful error codes (not causing an unexpected hard exit) so scripts invoked in non-TTY contexts can fail with a clear message.
🧹 Nitpick comments (1)
setup_payram.sh (1)
3923-3924: Propagatemainexit status instead of unconditional success.With
set -euo pipefailat script level (line 2) and no interveningset +eat script scope, the unconditionalexit 0masks the actual exit status ofmain, which can return non-zero for error cases (e.g., unknown options at line 3760). This breaks proper error signaling to calling processes. Useexit $?to propagate the status.Suggested fix
main "$@" -exit 0 +exit $?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@setup_payram.sh` around lines 3923 - 3924, The script currently calls main "$@" and then unconditionally exits with exit 0 which masks main's return code; change the script so that after invoking main "$@" it exits with main's actual status (propagate "$?" or use an immediate exit with the command's status) so failures from main (e.g., unknown option handling in main) are signaled to callers; update the lines invoking main and exit to capture and re-exit with main's exit code while keeping existing shell options like set -euo pipefail intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@setup_payram.sh`:
- Around line 775-3898: Many interactive reads use direct redirection "read ...
</dev/tty" which will cause the script to abort under set -e when /dev/tty is
unavailable (CI/cron/daemon). Add a centralized guarded helper (e.g.
check_tty_or_fail and tty_read wrapper) that verifies /dev/tty is readable and
either performs the read or prints a clear error and exits non-zero; then
replace all raw uses of "read ... </dev/tty" across interactive entry points
(e.g. configure_database, configure_external_database,
configure_internal_database, configure_ssl, configure_ssl_letsencrypt,
configure_ssl_custom, configure_ssl_update*, generate_aes_key,
show_interactive_menu, show_network_selection, reset_payram_environment,
display_firewall_notice, display_access_urls, and any other functions) to call
the wrapper; ensure non-interactive helper functions keep returning graceful
error codes (not causing an unexpected hard exit) so scripts invoked in non-TTY
contexts can fail with a clear message.
---
Nitpick comments:
In `@setup_payram.sh`:
- Around line 3923-3924: The script currently calls main "$@" and then
unconditionally exits with exit 0 which masks main's return code; change the
script so that after invoking main "$@" it exits with main's actual status
(propagate "$?" or use an immediate exit with the command's status) so failures
from main (e.g., unknown option handling in main) are signaled to callers;
update the lines invoking main and exit to capture and re-exit with main's exit
code while keeping existing shell options like set -euo pipefail intact.
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.
…d execution