- Notifications
You must be signed in to change notification settings - Fork914
feat(agent/reconnectingpty): allow selecting backend type#17011
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Cool, thanks for adding this. Having this configurable is very nice!
| @@ -64,13 +66,20 @@ func New(ctx context.Context, logger slog.Logger, execer agentexec.Execer, cmd * | |||
| // runs) but in CI screen often incorrectly claims the session name does not | |||
| // exist even though screen -list shows it. For now, restrict screen to | |||
| // Linux. | |||
| backendType := "buffered" | |||
| autoBackendType := "buffered" | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Suggestion: can we call thisplain? Otherwise it feels like we're leaking implementation details.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'll do this as a follow-up refactor 👍
| // If the user specified a command, we'll prefer to use the buffered method. | ||
| // The screen backend is not well suited for one-shot commands. | ||
| backend = "buffered" | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Suggestion: It'd be nice to see this as a CLI flag too, to (auto,plain,screen).
Side-note: I guessscreen could even be calledmultiplexer to allow to swap out for e.g.tmux in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yeah, I considered having it a CLI flag, but hesitated exposing this internal implementation detail. It does make sense, especially with yourvim comment below.
| return false | ||
| } | ||
| return true | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This has some interesting repercussions when it's neither a shell and not a one-shot command. Imagine doingvim /my/file.txt. Here we would return true butvim is one of the reasons we have thescreen backend.
This is probably fine though, but I'm flagging this behavior as it's not ideal. At the very least worth a comment.
(The flag would alleviate this as you can enforce the behavior then.)
6862409 intomainUh oh!
There was an error while loading.Please reload this page.
In#8640 we added an alternative 'screen' backend for our reconnectingpty endpoint used by our web terminal.
This opportunistically checks for the presence of
screenin$PATHand selects the screen backend if screen is available, falling back to the default "buffered" backend.This is fantastic for long-lived shell sessions, but another use case for rpty is just executing "one-off" commands (basically, anything that isn't a shell).
In these cases, the screen backend is actually kind of annoying, as you get the following output:
I considered the alternative of stripping the extraneous output, but decided it made more sense to skip the overhead of executing screen if it won't be needed anyway for a simple one-shot command.
The definition of a "one-shot" command is somewhat open to interpretation.
This should not affect any existing usage, as omitting
BackendTypeshould result in the same behaviour as before.Note: this also loses the extra "Connected to agent" and "reconnect ID" output from the
exp rptycommand. I didn't find them especially useful, but can easily add these back if needed.