search

cantino / mcfly

Fly through your shell history. Great Scott!
MIT License
6.87k stars 177 forks source link

mcfly 0.9.2 conflict with zoxide in WarpTerminal #435

Open aminya opened 1 month ago

aminya commented 1 month ago

I just upgraded mcfly to 0.9.2, but when zoxide is enabled, the bash prompt doesn't work anymore. Here's the error I get when I open a new shell

bash: PROMPT_COMMAND: line 706: syntax error near unexpected token `;;'
bash: PROMPT_COMMAND: line 706: `__zoxide_hook;;mcfly_prompt_command'

.bashrc

eval "$(zoxide init bash)"
eval "$(mcfly init bash)"
> echo $PROMPT_COMMAND
__zoxide_hook;;mcfly_prompt_command __bp_trap_string="$(trap -p DEBUG)" trap - DEBUG __bp_install
bash --version
GNU bash, version 5.1.16(1)-release (x86_64-pc-linux-gnu)

This seems to be happening only in Warp. I can't reproduce it using KDE Konsole.

This looks like the faulty commit: https://github.com/cantino/mcfly/commit/b8e86c5556e923e30e7916866033bb08bb6364ad https://github.com/cantino/mcfly/pull/430

akinomyoga commented 1 month ago

I'll take a look.

aminya commented 1 month ago

I made a PR that reverts the change, and it works as expected. The proposed change for using arrays needs more testing in my opinion.
https://github.com/cantino/mcfly/pull/436

akinomyoga commented 1 month ago

I don't use WarpTerminal, but the problem doesn't reproduce in my environment with several other terminals. In addition, I don't have an idea about the reason why the change in #430 would cause the reported behavior.

What I can tell from the content of PROMPT_COMMAND are

I suspect some settings in your environment performs PROMPT_COMMAND="${PROMPT_COMMAND[*]}". If that is the case, I have to say such a setting is broken. In Bash >= 5.1, Bash officially supported the array PROMPT_COMMAND, but such a setting would be broken for any configurations using the officially supported feature.

I first suspected WarpTerminal's shell integration (if any). WarpTerminal seems to be closed-source, so I don't have a way to quickly check (without installing WarpTerminal) whether WarpTerminal has a shell integration and, if any, whether it is related to the present problem. They seem to declare that they will gradually make the codebase open-source (see https://github.com/warpdotdev/Warp/discussions/400 and https://github.com/warpdotdev/Warp/commit/876cca9fdce41f5d59a3adca7ca9281aef219da2), but there seems to be no progress after two years.

@aminya I have questions:

$ declare -p PROMPT_COMMAND
PROMPT_COMMAND=('(echo foo)' '(echo bar)')
akinomyoga commented 1 month ago
aminya commented 1 month ago

Q1: Does the problem happen within other terminals with the same .bashrc?

No, I could not reproduce it in KDE Konsole.

Q2: What is the output of the following command? I'd like to get a precise state of the shell variable PROMPT_COMMAND

The output with only zoxide and mcfly in Warp

declare -p PROMPT_COMMAND
declare -- PROMPT_COMMAND="__zoxide_hook;;mcfly_prompt_command
__bp_trap_string=\"\$(trap -p DEBUG)\"
trap - DEBUG
__bp_install"

The output with only zoxide and mcfly in KDE Console:

> declare -p PROMPT_COMMAND
declare -a PROMPT_COMMAND=([0]="__zoxide_hook;" [1]="mcfly_prompt_command")

The above shows that the check for the array doesn't seem to be enough. Warp, uses a more complex entry in its PROMPT_COMMAND hooks.

Q3: Could you put the following lines in ~/.bashrc instead of zoxide and mcfly initialization and see the behavior? Are the lines foo and bar shown before every prompt as expected?

Yes

foo
bar
  • Q4: Maybe even another setting is interfering. Do you have anything else in your .bashrc/.bash_profile settings? Does the problem reproduce with just the two lines eval "$(zoxide init bash)" and eval "$(mcfly init bash)"?

Yes, I tested with only those two lines, and the issue happened to me.

akinomyoga commented 1 month ago

Thank you for the information! From the answers to Q1, Q4, and Q2 for KDE Konsole, it seems Warp Terminal is at least related. I'm currently trying to install it in my Arch (I'm not sure, but Warp Terminal hangs with the startup window of "Sign up" in my environment). While I attempt the installation, could you also check the behavior with the following setting?

# bashrc
PROMPT_COMMAND=('(echo foo);' '(echo bar)')

edit:

akinomyoga commented 1 month ago

Although the suffix ; in PROMPT_COMMAND should be harmless, it seems to trigger the problem when someone (probably Warp Terminal) tries to join the elements of PROMPT_COMMAND by a semicolon ; (which is actually too naive).

When PROMPT_COMMAND is empty at the point when Zoxide is initialized, the suffix ; seems to be added by the Zoxide integration:

if [[ ${PROMPT_COMMAND:=} != *'__zoxide_hook'* ]]; then
    PROMPT_COMMAND="__zoxide_hook;${PROMPT_COMMAND#;}"
fi
aminya commented 1 month ago

Here's the output with the above in bashrc. I face the same error. I don't get foo/bar

bash: PROMPT_COMMAND: line 706: syntax error near unexpected token `;;'
bash: PROMPT_COMMAND: line 706: `(echo foo);;(echo bar)'
> declare -p PROMPT_COMMAND
declare -- PROMPT_COMMAND="(echo foo);;(echo bar)
__bp_trap_string=\"\$(trap -p DEBUG)\"
trap - DEBUG
__bp_install"
akinomyoga commented 1 month ago

Thank you! Then, I'll have to conclude that this is a bug of Warp Terminal (or a shell setting loaded by Warp Terminal).

Although I still cannot launch the Warp Terminal in my environment (probably because of some X11 Forwarding issue with the SSH connection), but I could extract the Bash integration script from the warp binary file by the following command:

$ strings /path/to/warp | sed -n '/# We need to prepend a space/,/^ *warp_bootstrapped/p' > warp.bash

This script has 1307 lines, but I'll examine suspicious parts related to PROMPT_COMMAND.

akinomyoga commented 1 month ago

I found the following section starting on line 1211 of the Warp shell integration script:

    # If the user's rc files turned PROMPT_COMMAND into an array, we must undo that.
    # Since Bash 5.1, the PROMPT_COMMAND variable can be an array, see:
    #   https://tiswww.case.edu/php/chet/bash/NEWS
    # Unfortunately, doing so will break Warp because of the way it interacts with bash-preexec.
    # When PROMPT_COMMAND is an array, the DEBUG signal fires for each array element, and since
    # bash-preexec uses a DEBUG trap to trigger the preexec functions, it will run our preexec
    # functions before the command at PROMPT_COMMAND[1], PROMPT_COMMAND[2], etc. This means our
    # Preexec hook gets called without the user submitting a command, putting the input block into
    # a broken state, e.g. see https://github.com/warpdotdev/Warp/issues/2636
    # If they end up fixing this, we may be able to remove this at some point, check this:
    #   https://github.com/rcaloras/bash-preexec/issues/130
    #
    # If PROMPT_COMMAND is set and it's an array, "join" the array into a semicolon-delimited string,
    # then overwrite PROMPT_COMMAND
    if [[ -n $PROMPT_COMMAND && "$(declare -p PROMPT_COMMAND)" =~ "declare -a" ]]; then
        PROMPT_COMMAND_FLATTENED=$(IFS="; "; echo "${PROMPT_COMMAND[*]}")
        unset PROMPT_COMMAND
        PROMPT_COMMAND=$PROMPT_COMMAND_FLATTENED
    fi

I have to say the line PROMPT_COMMAND_FLATTENED=$(IFS="; "; echo "${PROMPT_COMMAND[*]}") is wrong. Shell commands cannot be always connected by ;. In the present case, we may drop ; from the first element. However, we cannot connect the commands with ; also when the element ends with &, but we cannot drop & because it changes the meaning.

I searched for the first line of the extracted script on the internet and in the GitHub code search, but nothing was found. This means that the Bash ingtegration script of Warp is indeed closed-source, yet we may ask the upstream Warp project about this issue.

akinomyoga commented 1 month ago

https://github.com/warpdotdev/Warp/issues/5219

akinomyoga commented 1 month ago

@aminya For the time being, could you put the following line at the end of ~/.bashrc?

PROMPT_COMMAND=("${PROMPT_COMMAND[@]%;}")