feat(agent/reconnectingpty): allow selecting backend type #17011
Conversation
| @@ -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.
Suggestion: can we call this plain? Otherwise it feels like we're leaking implementation details.
There was a problem hiding this comment.
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.
Suggestion: It'd be nice to see this as a CLI flag too, to (auto, plain, screen).
Side-note: I guess screen could even be called multiplexer to allow to swap out for e.g. tmux in the future.
There was a problem hiding this comment.
Yeah, I considered having it a CLI flag, but hesitated exposing this internal implementation detail. It does make sense, especially with your vim comment below.
| return false | ||
| } | ||
| return true | ||
| } |
There was a problem hiding this comment.
This has some interesting repercussions when it's neither a shell and not a one-shot command. Imagine doing vim /my/file.txt. Here we would return true but vim is one of the reasons we have the screen 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.)
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.