Skip to content

fix(interactive): only print shoutout when stdout is a terminal#349

Open
drmikecrowe wants to merge 2 commits into
bevry:masterfrom
drmikecrowe:fix/shoutouts-tty-guard
Open

fix(interactive): only print shoutout when stdout is a terminal#349
drmikecrowe wants to merge 2 commits into
bevry:masterfrom
drmikecrowe:fix/shoutouts-tty-guard

Conversation

@drmikecrowe

Copy link
Copy Markdown
Contributor

The shoutout line ran on every interactive shell load, including non-terminal contexts such as git hooks, piped commands, and TERM=dumb sessions, where it polluted output. Guard it with [ -t 1 ] and a TERM check so it only prints to a real terminal.

The shoutout line ran on every interactive shell load, including
non-terminal contexts such as git hooks, piped commands, and
TERM=dumb sessions, where it polluted output. Guard it with `[ -t 1 ]`
and a TERM check so it only prints to a real terminal.
@balupton

balupton commented Jun 18, 2026

Copy link
Copy Markdown
Member

lgtm, good find

Todos:

  • the new checks should go before the command existence check, as the command existence check is a slower operation; we want to slower operations last so earlier faster operations that are false avoid doing the slower operations

  • I've also moved the warnings into this too, as that makes sense.

  • Other interactive.* need updating too:

Notes:

For reference there are also these which use some bash.bash vars (bash.bash is not sourced in the interactive shell, as it enables strict mode):

  • https://github.com/bevry/dorothy/blob/master/docs/scripting/terminals-and-tty.md
  • https://github.com/bevry/dorothy/blob/master/commands/debug-terminal
  • dorothy/sources/bash.bash

    Lines 693 to 746 in d33a5d5

    # Whether the terminal supports the `/dev/tty` device file
    if (: </dev/tty >/dev/tty) &>/dev/null; then
    # This applies to:
    # - normal execution: <cmd>
    # - background execution: <cmd> &
    # - stdin execution: echo | <cmd>
    TERMINAL_OUTPUT_TARGET="${TERMINAL_OUTPUT_TARGET:-"/dev/tty"}" # allow custom value for testing
    TERMINAL_INPUT_TARGET='/dev/tty'
    TERMINAL_POSITION_INPUT_TARGET='/dev/tty'
    CAN_QUERY_TERMINAL_SIZE='yes'
    IS_STDIN_LINE_BUFFERED='no'
    IS_TTY_AVAILABLE='yes'
    else
    # This applies to:
    # - `ssh -T execution: ssh -T localhost <cmd>`
    # - GitHub Actions execution
    TERMINAL_OUTPUT_TARGET="${TERMINAL_OUTPUT_TARGET:-"/dev/stderr"}" # allow custom value for testing
    # trunk-ignore(shellcheck/SC2034)
    TERMINAL_INPUT_TARGET='/dev/stdin'
    # trunk-ignore(shellcheck/SC2034)
    TERMINAL_POSITION_INPUT_TARGET=''
    # trunk-ignore(shellcheck/SC2034)
    CAN_QUERY_TERMINAL_SIZE='no'
    # trunk-ignore(shellcheck/SC2034)
    IS_STDIN_LINE_BUFFERED='yes' # @todo this not should not apply to CI
    # trunk-ignore(shellcheck/SC2034)
    IS_TTY_AVAILABLE='no'
    fi
    if [[ ${CI-} =~ ^(yes|YES|true|TRUE|1)$ ]]; then
    CI='yes'
    else
    CI=''
    fi
    if [[ -n $CI ]]; then
    ALTERNATIVE_SCREEN_BUFFER_SUPPORTED='no'
    else
    # trunk-ignore(shellcheck/SC2034)
    ALTERNATIVE_SCREEN_BUFFER_SUPPORTED='yes'
    fi
    if [[ -t 0 ]]; then
    # This applies to:
    # - normal execution: <cmd>
    IS_STDIN_OPENED_ON_TERMINAL='yes'
    TERMINAL_THEME_INPUT_TARGET='/dev/tty' # /dev/stdin also supported, but /dev/tty is always available in this case
    else
    # This applies to:
    # - stdin execution: echo | <cmd>
    # - background execution: <cmd> &
    # - ssh -T execution: ssh -T localhost <cmd>
    # - GitHub Actions execution
    IS_STDIN_OPENED_ON_TERMINAL='no'
    # trunk-ignore(shellcheck/SC2034)
    TERMINAL_THEME_INPUT_TARGET=''
    fi
  • checks if stdin and stdout are attached to TTY:
  • checks if stdin is attached to TTY:

Happy to merge as is, then revise later, up to you.

@balupton

Copy link
Copy Markdown
Member

The shoutout line ran on every interactive shell load, including non-terminal contexts such as git hooks, piped commands, and TERM=dumb sessions, where it polluted output.

I'm trying to figure out how init.sh allowed Dorothy to load within those contexts. As interactive.sh should only have been loaded by init.sh if the shell is login and interactive. So the situation that requires this workaround should not be the case. Can you do some debugging of what the init.sh flow is in those cases, you can use exec 5>>debug.log; BASH_XTRACEFD=5; set -xv at the start of init.sh.

@drmikecrowe

Copy link
Copy Markdown
Contributor Author

I'm trying to figure out how init.sh allowed Dorothy to load within those contexts.

Ack -- this might have been me. In a couple of cases I had issues with my scripts not starting and it's entirely possible I had a -l to force the login script or something.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants