Skip to content

allow spawning a shell after kitten ssh exit first step - #8807

Closed
barr-israel wants to merge 1 commit into
kovidgoyal:masterfrom
barr-israel:kitten_ssh_exit_shell
Closed

allow spawning a shell after kitten ssh exit first step#8807
barr-israel wants to merge 1 commit into
kovidgoyal:masterfrom
barr-israel:kitten_ssh_exit_shell

Conversation

@barr-israel

@barr-israel barr-israel commented Jul 11, 2025

Copy link
Copy Markdown
Contributor

I have successfully gotten the behavior I wanted in #8806 , but I am having trouble adding a flag for it, this fork currently does not build due to

Cant map fields: ssh_spawn_shell for cmd: launch
Generating go code failed with exit code: 1
The following build command failed: /home/barr/Documents/code/kitty/dependencies/linux-amd64/bin/python setup.py develop
exit status 1

After adding ssh_spawn_shell to the launch.py protocol_spec.
I have also added it to clone_safe_opts expecting it to be necessary.
Additionally, there is a little parameter drilling here from launch to modify_argv_for_launch_with_cwd that im not sure if I can avoid.

When this is resolved I will add the same flag to new_windows and its variants

Any help would be appreciated

@kovidgoyal

Copy link
Copy Markdown
Owner

You need to add it to options_spec() in kitty/launch.py and implement it in the _launch() function

@kovidgoyal

Copy link
Copy Markdown
Owner

Once you are happy with your implementation please squash it down to one commit and force push, then I will review.

@barr-israel

Copy link
Copy Markdown
Contributor Author

it was already implemented in _launch when you made that comment, but i have now added it to options_spec and LaunchKwds and the build still fails, Ill keep investigating

@kovidgoyal

Copy link
Copy Markdown
Owner

You need two blank lines before --ssh-spawn-shell

@barr-israel

Copy link
Copy Markdown
Contributor Author

Thanks, it seems to be working now.
I dont see any options available on the new_X_with_cwd commands, so I guess I should just use launch and set the relevant type.
Ill play around with it more to make sure nothing breaks.
Are any new tests needed for this?

@kovidgoyal

Copy link
Copy Markdown
Owner

No, this functionality is pretty simple, it shouldnt need tests.

  1. argv[:] = ... should be argv = to avoid an extra copy

  2. I dont like the name ssh-spawn-shell as it is not clear what that means? It could mean spawn a shell on the remote machine. Instead something like, --hold-after-ssh this matches the usage in kitty of --hold

  3. In the docs for the option instead of cwd=current use something like :option:--cwd=current so that it becomes a link and also add a similar link to this option in the docs for the --cwd option.

  4. Rebase into a single commit

@barr-israel
barr-israel force-pushed the kitten_ssh_exit_shell branch from f6693d1 to ca2232e Compare July 12, 2025 07:10
@barr-israel

Copy link
Copy Markdown
Contributor Author
  1. at least one argv[:] is required because the caller does not get it back as a response but instead expects the value inside it to be changed(I've tried with only argv= and it does not work). Ive minimized it to one instead of two
  2. done
  3. done
  4. done

…itten ssh disconnection

new option to the launch command, the new shell will be in the directory
that launched kitten ssh
@barr-israel
barr-israel force-pushed the kitten_ssh_exit_shell branch from ca2232e to 55c51aa Compare July 12, 2025 07:16
@barr-israel

barr-israel commented Jul 12, 2025

Copy link
Copy Markdown
Contributor Author

fixed a ruff lint error

@kovidgoyal

Copy link
Copy Markdown
Owner

merged.

@kovidgoyal kovidgoyal closed this Jul 12, 2025
@barr-israel

Copy link
Copy Markdown
Contributor Author

Great, thanks for all the help

@barr-israel
barr-israel deleted the kitten_ssh_exit_shell branch July 21, 2025 13:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants