fix: improve non-interactive TTY detection in install.sh#3315
Open
mango766 wants to merge 1 commit intoatuinsh:mainfrom
Open
fix: improve non-interactive TTY detection in install.sh#3315mango766 wants to merge 1 commit intoatuinsh:mainfrom
mango766 wants to merge 1 commit intoatuinsh:mainfrom
Conversation
The `{ true </dev/tty; }` check only verifies that /dev/tty can be
opened, not that it is a functional terminal. In containers, CI, and
headless environments, /dev/tty may exist as a device node but not be
backed by a real terminal, causing subsequent `read </dev/tty` calls
to fail with "cannot open /dev/tty" errors or hang indefinitely.
Replace the check with a subshell that opens /dev/tty as stdin and
verifies it is a terminal via `test -t 0`. Also add `2>/dev/null` to
the read calls as a safety net against noisy errors if the detection
is ever bypassed.
Fixes atuinsh#3294
Contributor
Greptile SummaryFixes false-positive interactive detection in
Important Files Changed
Last reviewed commit: "fix: improve non-int..." |
Contributor
|
Thank so much for taking care of this! |
|
|
||
| printf "Would you like to import your existing shell history into Atuin? [Y/n] " | ||
| read -r import_answer </dev/tty || import_answer="n" | ||
| read -r import_answer </dev/tty 2>/dev/null || import_answer="n" |
Contributor
There was a problem hiding this comment.
I recommend not suppressing errors from the two reads.
Think about it this way: those error messages were the reason why we were able to diagnose the issue in my bug so quickly.
Without them, it would just be an unexplained hang.
My issue is fixed with a better TTY check, but if we hit some other problem during read, the error messages would be helpful.
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.
Fixes #3294
Checks
Problem
The install script's non-interactive detection uses
{ true </dev/tty; }to check if a terminal is available. This only tests whether/dev/ttycan be opened, not whether it is actually a functional terminal. In containers, CI, and headless environments (like the Ona setup described in #3294),/dev/ttymay exist as a device node that the shell can open, but isn't backed by a real terminal. This causes:read </dev/ttycalls to fail withcannot open /dev/tty: No such device or addressatuin setup </dev/ttycall to hang indefinitelyFix
Two changes, both in
install.sh:1. Stronger TTY detection — Replace
{ true </dev/tty; }with( exec </dev/tty && [ -t 0 ] ). This opens/dev/ttyas stdin inside a subshell, then checks it's actually a terminal withtest -t. The subshell ensures the redirect doesn't affect the parent process.2. Suppress stderr on
readfallbacks — Add2>/dev/nullto the tworead </dev/ttycalls. They already have|| var="n"fallbacks, but without stderr suppression, failed redirects produce noisy error messages on shells likedash.