Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unset ZDOTDIR before standard scripts run and add VSCODE_SHELL_INTEGRATION #145610

Merged
merged 2 commits into from
Mar 21, 2022

Conversation

Tyriar
Copy link
Member

@Tyriar Tyriar commented Mar 21, 2022

Fixes #145587
Fixes #145583
Fixes #145582

@Tyriar Tyriar added this to the March 2022 milestone Mar 21, 2022
@Tyriar Tyriar requested a review from meganrogge March 21, 2022 19:09
@Tyriar Tyriar self-assigned this Mar 21, 2022
@Tyriar Tyriar changed the title Unset ZDOTDIR before standard scripts run Unset ZDOTDIR before standard scripts run and add VSCODE_SHELL_INTEGRATION Mar 21, 2022
@Tyriar Tyriar merged commit 9b92cfe into main Mar 21, 2022
@Tyriar Tyriar deleted the tyriar/145587 branch March 21, 2022 19:50
@romkatv
Copy link

romkatv commented Mar 22, 2022

Would it be possible to suppress the message? I don't think it's worth keeping it even with an option. In general, shells don't print things during initialization.

@Tyriar
Copy link
Member Author

Tyriar commented Mar 22, 2022

@romkatv I understand your perspective, but then shell integration silently fails and that would result in "bugs" getting reported to us forever. We could have the line be a link with a hover that includes some documentation explaining it?

@romkatv
Copy link

romkatv commented Mar 23, 2022

My goal is for shell integration in vscode to work with powerlevel10k. If it worked out of the box, I would be more than happy. Unfortunately, it doesn't work and I don't think it's feasible to fix that from within vscode. Here are some of the features of powerlevel10k that pose a challenge when it comes to shell integration:

Powerlevel10k already supports shell integration with terminals that respect OSC 133 and/or OSC 7. I can implement support for the protocol used by vscode, too (although I would very much prefer if vscode used OSC 133; I don't see any feasibly extensions to that protocol that could give any additional functionality to zsh users). The logic I'm envisioning is as follows.

When powerlevel10k sees VSCODE_SHELL_INTEGRATION=1 during initialization, it unsets this variable and enables shell integration according to the protocol used by vscode.

This will work fine but unfortunately vscode will print a misleading message saying that shell integration is disabled. I see two ways how I can work around this issues. If instant prompt is enabled, stdout is already redirected to a file, so I can find this message and remove it. If intant prompt is not enabled, I can replace echo with a function that will special-case the message and avoid printing it. Both of these are ugly hacks.

Would it be possible to not print the message?

@Tyriar
Copy link
Member Author

Tyriar commented Mar 23, 2022

When powerlevel10k sees VSCODE_SHELL_INTEGRATION=1 during initialization, it unsets this variable and enables shell integration according to the protocol used by vscode.

Oh I see, I thought you would be disabling it outright. That sounds great, in that case we'll exit the script without warning and if possible warn to the devtools console which is only visible if the user is investigating a problem.

If you wanted to adopt the sequences as described below, the chances they will change is pretty low.

/**
* VS Code-specific shell integration sequences. Some of these are based on common alternatives like
* those pioneered in FinalTerm. The decision to move to entirely custom sequences was to try to
* improve reliability and prevent the possibility of applications confusing the terminal.
*/
const enum VSCodeOscPt {
/**
* The start of the prompt, this is expected to always appear at the start of a line.
* Based on FinalTerm's `OSC 133 ; A ST`.
*/
PromptStart = 'A',
/**
* The start of a command, ie. where the user inputs their command.
* Based on FinalTerm's `OSC 133 ; B ST`.
*/
CommandStart = 'B',
/**
* Sent just before the command output begins.
* Based on FinalTerm's `OSC 133 ; C ST`.
*/
CommandExecuted = 'C',
/**
* Sent just after a command has finished. The exit code is optional, when not specified it
* means no command was run (ie. enter on empty prompt or ctrl+c).
* Based on FinalTerm's `OSC 133 ; D [; <ExitCode>] ST`.
*/
CommandFinished = 'D',
/**
* Explicitly set the command line. This helps workaround problems with conpty not having a
* passthrough mode by providing an option on Windows to send the command that was run. With
* this sequence there's no need for the guessing based on the unreliable cursor positions that
* would otherwise be required.
*/
CommandLine = 'E',
/**
* Similar to prompt start but for line continuations.
*/
ContinuationStart = 'F',
/**
* Similar to command start but for line continuations.
*/
ContinuationEnd = 'G',
/**
* The start of the right prompt.
*/
RightPromptStart = 'H',
/**
* The end of the right prompt.
*/
RightPromptEnd = 'I',
/**
* Set an arbitrary property: `OSC 633 ; P ; <Property>=<Value> ST`, only known properties will
* be handled.
*/
Property = 'P'
}

Comments on some of them:

  • CommandLine = 'E' - This is currently only used for pwsh and the behavior when used on non-Windows is somewhat undefined right now. We'll support this on Linux/macOS if we need it for more reliable command extraction
  • Property = 'P' - Here are the current properties, you'll want to send OSC 633 ;Cwd=/path/in/this/format ST on each prompt. If you want sending it only when it changes should be sufficient. It's not necessary to send IsWindows as we assume non-Windows hosts by default.

@romkatv
Copy link

romkatv commented Mar 23, 2022

/**
* VS Code-specific shell integration sequences. Some of these are based on common alternatives like
* those pioneered in FinalTerm. The decision to move to entirely custom sequences was to try to
* improve reliability and prevent the possibility of applications confusing the terminal.
*/
const enum VSCodeOscPt {
/**
* The start of the prompt, this is expected to always appear at the start of a line.
* Based on FinalTerm's `OSC 133 ; A ST`.
*/
PromptStart = 'A',
/**
* The start of a command, ie. where the user inputs their command.
* Based on FinalTerm's `OSC 133 ; B ST`.
*/
CommandStart = 'B',
/**
* Sent just before the command output begins.
* Based on FinalTerm's `OSC 133 ; C ST`.
*/
CommandExecuted = 'C',
/**
* Sent just after a command has finished. The exit code is optional, when not specified it
* means no command was run (ie. enter on empty prompt or ctrl+c).
* Based on FinalTerm's `OSC 133 ; D [; <ExitCode>] ST`.
*/
CommandFinished = 'D',
/**
* Explicitly set the command line. This helps workaround problems with conpty not having a
* passthrough mode by providing an option on Windows to send the command that was run. With
* this sequence there's no need for the guessing based on the unreliable cursor positions that
* would otherwise be required.
*/
CommandLine = 'E',
/**
* Similar to prompt start but for line continuations.
*/
ContinuationStart = 'F',
/**
* Similar to command start but for line continuations.
*/
ContinuationEnd = 'G',
/**
* The start of the right prompt.
*/
RightPromptStart = 'H',
/**
* The end of the right prompt.
*/
RightPromptEnd = 'I',
/**
* Set an arbitrary property: `OSC 633 ; P ; <Property>=<Value> ST`, only known properties will
* be handled.
*/
Property = 'P'
}

Comments on some of them:

  • CommandLine = 'E' - This is currently only used for pwsh and the behavior when used on non-Windows is somewhat undefined right now. We'll support this on Linux/macOS if we need it for more reliable command extraction

I'm positive that it's impossible to extract the command line from Zsh Line Editor (zle) in general. I hope you can support this on Linux.

  • Property = 'P' - Here are the current properties, you'll want to send OSC 633 ;Cwd=/path/in/this/format ST on each prompt. If you want sending it only when it changes should be sufficient.

How should I escape \007 in cwd? (I see that your code doesn't escape it; you probably want to fix that.)

What about SSH? If I send cwd from the remote host, will vscode mistakenly treat it as if this was cwd from the local host?

Other terminals solve this problem by requiring hostname to be supplied together with cwd.

@Tyriar
Copy link
Member Author

Tyriar commented Mar 23, 2022

I'm positive that it's impossible to extract the command line from Zsh Line Editor (zle) in general. I hope you can support this on Linux.

On pwsh we use Get-History to do this, history in zsh seems to give the full entry with \n replacing new lines. You can only get this once the command has finished running but that's a fine compromise if it's very accurate.

How should I escape \007 in cwd? (I see that your code doesn't escape it; you probably want to fix that.)

Good call, this is how we escape the command line sequence:

commandLine = (args[0]
.replace(/<LF>/g, '\n')
.replace(/<CL>/g, ';'));

Following this I guess we can do for now to align with that. This was a temporary/good enough solution to get things up and running but if you have a more robust idea for escaping we can make those improvements soon.

What about SSH? If I send cwd from the remote host, will vscode mistakenly treat it as if this was cwd from the local host?

Well subshell support isn't there currently and how to handle that was going to get figured out there. We assume all terminals are "local" to the window currently (which may be remote depending on the type of window). For subshells my thinking was to add a unique shell id sequence to every prompt so we are able to detect when each shell starts and begins, including ones on the same hostname.

We also have a special way of defining URIs for remote connected windows which we may use to be able to identify files that are available in the workspace, eg. vscode-remote://wsl+ubuntu/home/daniel/

Currently the cwd feeds into the tab's title as well as resolving links relative to it, if you send cwd for an SSH the folder in a tab should show up like this as I don't think it's validated against the FS:

image

The only other side effect is that links clicked in the terminal will try resolve which shouldn't cause much harm.

So if you're able to hook into ssh sessions and integrate with them you can either do nothing until we get #144358 done or send them as normal paths.

@romkatv
Copy link

romkatv commented Mar 23, 2022

history in zsh seems to give the full entry with \n replacing new lines.

The standard way to get the command line in zsh is from a preexec hook: $1 is the orginal command, $2 is after alias expansion.

How should I escape \007 in cwd? (I see that your code doesn't escape it; you probably want to fix that.)

Good call, this is how we escape the command line sequence:

commandLine = (args[0]
.replace(/<LF>/g, '\n')
.replace(/<CL>/g, ';'));

How should <LF> be escaped?

Following this I guess we can do for now to align with that. This was a temporary/good enough solution to get things up and running but if you have a more robust idea for escaping we can make those improvements soon.

OSC 7 uses percent encoding. It's a natural fit because the payload is a URL. It would be fantastic if vscode used OSC 7 instead of creating another OSC code. Is there something in OSC 7 that won't work for you?

What about SSH? If I send cwd from the remote host, will vscode mistakenly treat it as if this was cwd from the local host?

Well subshell support isn't there currently and how to handle that was going to get figured out there.

My code will have subshell support out of the box. More specifically, if you are using powerlevel10k and requesting shell integration, it'll work as long as your terminal has some form of shell integration support. It doesn't matter whether the shell is a shubshell and on which machine it runs.

We assume all terminals are "local" to the window currently (which may be remote depending on the type of window).

I'm confused by the word "terminal". Terminal is the thing that displays pixels and accepts keyboard input. It's always local. I'm probably misunderstanding what you are trying to say.

For subshells my thinking was to add a unique shell id sequence to every prompt so we are able to detect when each shell starts and begins, including ones on the same hostname.

Why would you need unique shell id? Do you need to know whether something is a subshell or not? No other terminal with shell integration makes this distinction and I cannot think of a feature that would require it.

Currently the cwd feeds into the tab's title as well as resolving links relative to it

I noticed that.

Is there an option to respect the title set by the application in the terminal? All other terminals have this option. This allows shell users to set better title.

So if you're able to hook into ssh sessions and integrate with them you can either do nothing until we get #144358 done or send them as normal paths.

There is no reliable way to detect whether a shell is running over SSH or not, so I cannot send CWD only when shell is not running over SSH. So I'll send it always. It's unfortunate that vscode will try to resolve this directory locally. I hope you can adopt OSC 7 or similar to solve this problem.

@Tyriar
Copy link
Member Author

Tyriar commented Mar 23, 2022

The standard way to get the command line in zsh is from a preexec hook: $1 is the orginal command, $2 is after alias expansion.

Good to know 👍

How should be escaped?

That's the major flaw with the current hacky solution, you're coming in a bit early is all.

OSC 7 uses percent encoding. It's a natural fit because the payload is a URL. It would be fantastic if vscode used OSC 7 instead of creating another OSC code. Is there something in OSC 7 that won't work for you?

I looked into using the existing ones in #140514 and decided against it because OSC 7 isn't sufficient on Windows, you're meant to use OSC 9;9 there and the definition on OSC 7 didn't seem completely clear. The idea of moving to our own set of custom sequences allows us to have full control of everything as we own the client and the scripts, additionally I found other tools were confusing VS Code's integration as multiple levels of shell integration happening, this was especially important on Windows which is very finicky due to how conpty renders.

We assume all terminals are "local" to the window currently (which may be remote depending on the type of window).

I'm confused by the word "terminal". Terminal is the thing that displays pixels and accepts keyboard input. It's always local. I'm probably misunderstanding what you are trying to say.

I meant shell here, by local to the window I mean some windows may be connected via ssh, to a container or to wsl.

Why would you need unique shell id? Do you need to know whether something is a subshell or not? No other terminal with shell integration makes this distinction and I cannot think of a feature that would require it.

The main thing I think we need this for is knowing when one command ends and another begins. When subshells we will would need to understand the concept of nested commands, for example:

image

The output of the bash command, is all content of the subshell.

We also check the process tree of the terminal to determine whether it's safe to close without a warning, with shell integration we could avoid this and instead check whether a command is active or a sub shell is active. We may end up not needing to do this after it's thought through more.

Is there an option to respect the title set by the application in the terminal? All other terminals have this option. This allows shell users to set better title.

Yes you can do this, you can configure both the title and the description (the greyed out part):

image

@romkatv
Copy link

romkatv commented Mar 23, 2022

I looked into using the existing ones in #140514 and decided against it because OSC 7 isn't sufficient on Windows

Zsh integration code in vscode sends $PWD via OSC 9;9. As far as I can tell, there are no advantages to doing that instead of OSC 7.

the definition on OSC 7 didn't seem completely clear.

Yes, dealing with de facto standards (rather than formal standards) can be tricky. Ditching them is not the solution though.

The idea of moving to our own set of custom sequences allows us to have full control of everything as we own the client and the scripts

I understand that it makes your life easier in the short term. Users of your software have different incentives though.

Imagine if the next mobile phone you bought used a different format of SIM cards because it was easier for the manufacturer to control the new protocol. That's the position your users are in when you create new OSC codes where the existing ones would suffice.

I don't presume to know all your constraints but I hope you can see the trade off from my perspective. I have to implement shell integration in zsh for all terminals out there.

The main thing I think we need this for is knowing when one command ends and another begins. When subshells we will would need to understand the concept of nested commands, for example:

image

If you could infer the tree of commands (I don't think it's possible but let's imagine it was), what would you do with it?

The approach that other terminals take is to not deal with the tree at all. In your example the terminal would see a linear sequence of commands: bash, echo abc, and exit. This is enough to implement the important shell-aware terminal features such as navigation up/down by commands, selecting the output of a chosen command, etc. Commands and their outputs look like a linear sequence to the user of the terminal, so it's natural if the terminal also treats them like that. The user doesn't see or deal with the tree.

Is there an option to respect the title set by the application in the terminal? All other terminals have this option. This allows shell users to set better title.

Yes you can do this, you can configure both the title and the description (the greyed out part):

image

Thanks! ${sequence} does what I want.

@Tyriar
Copy link
Member Author

Tyriar commented Mar 23, 2022

I understand that it makes your life easier in the short term. Users of your software have different incentives though.

How good of a solution it is depends on how successful we are in making our script the defacto script where all you need to do to get shell integration is to make sure the script runs. powerlevel10k certainly poses some special challenges because of its complexity though. As I mentioned before we're trying to get this enabled by default eventually so a huge amount of users who have not yet configured their shell get to experience the feature with zero config, even if their shell has zero customizations.

We also want more capabilities than the existing finalterm/iterm2 shell integration sequences allow, this is essential to get it working on Windows as we're in brand new territory there having rich pwsh integration on Windows and we want to keep our options open. I can imagine we will be adding more sequences over time to add more capabilities. If we accept existing shell integration used by other terminals that will inevitably lead to flaky and inconsistent results. For example one of the end goals of shell integration is to get native autocomplete and we may need special sequences to make that feature really shine.

We also have the option if it would help to add some special handling to our script for any shell or powerlevel10k if that's useful.

The user doesn't see or deal with the tree.

Yeah you might have convinced me, bit busy to think it through thoroughly atm. I'll make some notes on #144358

@romkatv
Copy link

romkatv commented Mar 23, 2022

How good of a solution it is depends on how successful we are

I'll buy that. If your new shell integration protocol allows you to implement new cool features, then it's justified. I don't know which features you could implement that cannot be implemented on top of OSC 133 (plus potentially an extension to send the command line) but I'll be happy to see them. The current version of zsh integration in vscode doesn't have anything that cannot be done with OSC 133 and OSC 7 though.

As I mentioned before we're trying to get this enabled by default eventually so a huge amount of users who have not yet configured their shell get to experience the feature with zero config

Zsh users on Windows don't run with empty zshrc. They all have user rc files. It's really easy for them to clone a git repo and source a file from it. They all know how to do it. Having the terminal injecting code into shell is something most users never expect to happen though. Given that shell injection from the terminal cannot be made to work reliably, you'll want to provide a git repo with a standalone file at some point. Once you do that, consider removing shell injection altogether. Instead, you can add a button that modifies ~/.zshrc. This would be in line with how zsh ecosystem works.

@Tyriar
Copy link
Member Author

Tyriar commented Mar 23, 2022

Once you do that, consider removing shell injection altogether. Instead, you can add a button that modifies ~/.zshrc. This would be in line with how zsh ecosystem works.

Our approach is somewhat atypical as most terminals are opt-in with an install step. If I remember right kitty auto installs it and some of the feedback they got was that it's intrusive, which I agree with, so we were trying to be as unintrusive as possible. As soon as you have a manual step the amount of users that would engage with the feature would drop significantly. This is an important part of why we too this approach, we want it to just work for the majority of users and fail unobtrusively for more advanced cases. Imagine connecting VS Code to a container or a new machine, opening a terminal and shell integration/autocomplete just works with zero config, that's the vision.

In the off chance that shell integration fails to inject before the shell starts, or the shell disables it, or something else goes wrong, we want to fail early and continue, then the user has the option to disable injection all together and go with the manual setup route we'll provide microsoft/vscode-docs#5219


This discussion is all super valuable so thank you so much for that! It was probably obvious that I'm not that familiar with zsh, I'm mostly a bash and pwsh user these days. The more we chat, the more I think that it's probably not ready for you to try to integrate since things are still in flux. The best approach to make sure you don't waste time is to just force disable shell integration with our new environment variable and then revisit in a few months?

@romkatv
Copy link

romkatv commented Mar 23, 2022

If I remember right kitty auto installs it and some of the feedback they got was that it's intrusive

Kitty is the only other terminal that does what vscode does -- shell injection via ZDOTDIR override. And yes, it's intrusive. As a member of zsh community I cannot stress enough how negatively this approach is perceived. It looks as abuse of power by the terminal and a violation of the established contract. It's not the job of the terminal to inject code into shell, especially when the same result can be accomplished via the existing means.

This is an important part of why we too this approach, we want it to just work for the majority of users

This is a noble goal but I doubt it's achievable. If integration works only until the user restarts shell with exec zsh, users won't be able to rely on it being there and won't establish habits that utilize shell integration. If I'm to start using a shortcut that copies the output of the last command into clipboard or opens it in $PAGER, I'll want that shortcut to actually work rather than be best effort.

I think I've made my opinion clear. It's your product, so your choice of course.

The best approach to make sure you don't waste time is to just force disable shell integration with our new environment variable and then revisit in a few months?

That sounds good to me. Feel free to @-mention me on anything related to integration with zsh.

romkatv added a commit to romkatv/powerlevel10k that referenced this pull request Apr 4, 2022
AdrianTM pushed a commit to AdrianTM/powerlevel10k that referenced this pull request Apr 27, 2022
@github-actions github-actions bot locked and limited conversation to collaborators May 5, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
3 participants