-
Notifications
You must be signed in to change notification settings - Fork 396
feat(chat): support user shell env in shell command tool #1043
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
feat(chat): support user shell env in shell command tool #1043
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1043 +/- ##
==========================================
- Coverage 13.69% 13.69% -0.01%
==========================================
Files 2361 2361
Lines 203706 203745 +39
Branches 184070 184109 +39
==========================================
+ Hits 27894 27895 +1
- Misses 174425 174462 +37
- Partials 1387 1388 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
d2b3cb5 to
121d495
Compare
mschrage
left a comment
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.
We should consider launching the process using --login and --interactive to emulate the behavior of the user's shell.
Pros:
- Will source relevant dotfiles automatically.
Cons:
- May add additional latency to shell execution as all dotfiles need to be sourced
- Introduces environment dependency which can result in inconsistent behavior that is difficult to debug.
|
Some interesting discussion here:
|
|
Yes I tried all those but didn't work. Instead, I will launch a PTY. |
f3f1d63 to
daf2218
Compare
|
zsh, bash, fish. How will automated tests work for this? Docker based, or can you do it with chroot/mocks? |
crates/q_cli/Cargo.toml
Outdated
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 should be a workspace dependency
crates/q_cli/Cargo.toml
Outdated
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.
Can you make this a workspace dependency and update figterm as well? (figterm already depends on 0.8)
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.
nit - move this after all of the imports, it's declaring a new function, also update the name to something more readable preferably, like set_terminal_winsize or something
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.
Since the above lines are copied straight from figterm (and they're particularly complicated) I wonder if this could be generalized in a library, e.g. in fig_util
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 doesn't work with fish, we'd need to get the current shell and fallback to bash otherwise
also, probably best to use https://github.com/aws/amazon-q-developer-cli/blob/main/crates/fig_util/src/shell.rs#L90
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 method seems to break our (admittedly already poor support of) interactive commands, e.g. npx create-react-app helloworld
My thought was that we would spawn the child with a pty, and set our terminal to raw mode. This way, we can copy all bytes directly to the child which can then handle sigint etc. which should support more interactive commands.
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.
removing stderr?
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.
With the PTY mode it's hard to differentiate stderr, but we can already infer from status code if the resulting message is error or not. The output of this function is to give some context to the LLM to inform it of the execution result.
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 was also thinking about how we can handle interactive commands since some (e.g. vim) shouldn't have their stdout returned back to the model, still an open question to solve
daf2218 to
ecce6f7
Compare

Issue #, if available:
#1030
Description of changes:
execute_bashtool toexecute_shell_commandszshandbash) from user config filesBy submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.