feat(tools): execute_bash uses PTY for shell integrations #1340

hayemaxi · 2025-04-24T05:50:56Z

Create a PTY and spawn the command inside that
Reads from user input for text and key events, allowing some use for interactive commands
Uses current program env vars, and users custom shell integrations (dot files e.g. .zshrc), user defined aliases and functions
Works for both execute_bash tool and context hooks
Moved PTY creation code from figterm to fig_util instead.

NOTES:

terminal is set to dumb mode as explained in the code. we should find a way around this.
we provde all command output which is expensive for some interactive commands that re-draw. Our next steps should involve writing more than a max output to a local file, then prompting Q to read from this file if it wants more context about what the tool did.
Not tested on windows (testable?) or other shells besides zsh (yet)

Issue #, if available:
#1030

Related: #1043

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

codecov-commenter · 2025-04-24T05:52:29Z

Codecov Report

❌ Patch coverage is 45.73171% with 89 lines in your changes missing coverage. Please review.
✅ Project coverage is 13.92%. Comparing base (76ce78d) to head (95957dd).
⚠️ Report is 645 commits behind head on main.

Files with missing lines	Patch %	Lines
crates/q_cli/src/cli/chat/tools/execute_bash.rs	52.63%	45 Missing and 9 partials ⚠️
crates/fig_util/src/terminal.rs	21.42%	33 Missing ⚠️
crates/figterm/src/main.rs	0.00%	1 Missing ⚠️
crates/q_cli/src/cli/chat/hooks.rs	85.71%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1340      +/-   ##
==========================================
- Coverage   14.36%   13.92%   -0.44%     
==========================================
  Files        2368     2350      -18     
  Lines      206342   203008    -3334     
  Branches   186706   183372    -3334     
==========================================
- Hits        29633    28270    -1363     
+ Misses     175251   173369    -1882     
+ Partials     1458     1369      -89

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

hayemaxi · 2025-04-24T05:53:21Z

crates/q_cli/src/cli/chat/tools/execute_bash.rs

Does this need to_str_lossy ?

Where? it's already a UTF-8 type here though?

- Create a PTY and spawn the command inside that - Reads from user input for text and key events, allowing some use for interactive commands - Uses current program env vars, and users custom shell integrations (dot files e.g. .zshrc) - Works for both `execute_bash` tool and context hooks - Moved PTY creation code from `figterm` to `fig_util` instead. NOTES: - terminal is set to dumb mode as explained in the code. we should find a way around this. - we provde all command output which is expensive for some interactive commands that re-draw. Our next steps should involve writing more than a max output to a local file, then prompting Q to read from this file if it wants more context about what the tool did. - Not tested on windows (testable?) or other shells besides zsh (yet)

jsamuel1 · 2025-04-27T02:35:03Z

crates/fig_util/src/terminal.rs

+            // Regular character
+            c.to_string().into_bytes()
+        },
+        KeyCode::Enter => vec![b'\r'],


This looks like a non-exhaustive list. Is there an API can that do this for us?

jsamuel1 · 2025-04-27T02:39:49Z

crates/q_cli/src/cli/chat/tools/execute_bash.rs

+
+        let stdout = stdout_lines.into_iter().collect::<String>();
+        Ok(CommandResult {
+            exit_status: exit_status.exit_code(),


often the last few lines are important.

Consider:
First N lines
a sample of "important lines" -- . eg. with ERROR, WARNING, etc on them.
Last N lines

Which may then look like.

Started app blah lots of startup stuff [ skipped X rows of content ] WARN: blah blah is wrong [ skipped X rows of content ] application is now existing with 49 warnings exited

also note, using the [ ] type of tagged lines, per how Claude likes to spit them out itself.

brandonskiser · 2025-05-01T16:10:09Z

crates/fig_util/Cargo.toml

 default = []

 [dependencies]
+anyhow.workspace = true


Not really a fan of adding all of these dependencies to a foundational crate in our workspace like fig_util, e.g. nix doesn't make sense here considering it's not for Windows

brandonskiser · 2025-05-01T16:12:57Z

crates/q_cli/src/cli/chat/tools/execute_bash.rs

-    pub async fn invoke(&self, updates: impl Write) -> Result<InvokeOutput> {
-        let output = run_command(&self.command, MAX_TOOL_RESPONSE_SIZE / 3, Some(updates)).await?;
+    // Note: _updates is unused because `impl Write` cannot be shared across threads, so we write to
+    // stdout directly. A type refactor is needed to support this.


We can now use SharedWriter - https://github.com/aws/amazon-q-developer-cli/blob/main/crates/q_chat/src/shared_writer.rs

brandonskiser · 2025-05-01T16:14:06Z

crates/q_cli/src/cli/chat/tools/execute_bash.rs

+        let output = self.execute_pty_with_input(MAX_TOOL_RESPONSE_SIZE / 3, true).await?;
        let result = serde_json::json!({
-            "exit_status": output.exit_status.unwrap_or(0).to_string(),
+            "exit_status": output.exit_status.to_string(),


Removing stderr?

brandonskiser · 2025-05-01T17:27:05Z

crates/fig_util/src/pty/cmdbuilder.rs

Would rather move this to a new crate for pty operations, e.g. fig_pty since it'll prevent adding a bunch of terminal deps to fig_util, and really a lot of it seems to just be copied from WezTerm

brandonskiser · 2025-05-01T17:50:56Z

crates/q_cli/src/cli/chat/tools/execute_bash.rs

+        const LINE_COUNT: usize = 1024;
+
+        // Open a new pseudoterminal
+        let pty_pair = open_pty(&get_terminal_size()).map_err(|e| eyre!("Failed to start PTY: {}", e))?;


wondering why we can't just use the portable-pty crate here instead

brandonskiser · 2025-05-01T17:53:37Z

crates/q_cli/src/cli/chat/tools/execute_bash.rs

+        // This is all but required because otherwise the stdout from the PTY gets cluttered
+        // with escape characters and shell integrations (e.g. current directory, current user, hostname).
+        // We can clean the escape chars but the shell integrations are much harder. Is there a better way?
+        // What happens if we don't use this: Q can get confused on what the output is actually saying.


I was seeing a lot of random junk when executing nvim for instance, and it wasn't being rendered properly.

Q can get confused on what the output is actually saying

It looks like we clean the output before sending to the model, why doesn't this work?

brandonskiser · 2025-05-01T18:04:22Z

crates/q_cli/src/cli/chat/tools/execute_bash.rs

+        // Create a command builder for the shell command
+        let shell = Shell::current_shell().map_or("bash", |s| s.as_str());
+        let mut cmd_builder = CommandBuilder::new(shell);
+        cmd_builder.args(["-cli", &self.command]);


what are these args? I believe -i is the only required option, we really just need an interactive shell.

This also seems to break with fish in my testing

brandonskiser · 2025-05-01T18:27:42Z

crates/q_cli/src/cli/chat/tools/execute_bash.rs

+                        // Note: this reads one character a time basically. Which is fine for
+                        // everything unless the user pastes a large amount of text.
+                        // Could use an upgrade to avoid this (maybe read from stdin and events at the same time)
+                        event = tokio::task::spawn_blocking(crossterm::event::read) => {


Why do we use crossterm events instead of just reading straight from stdin? We're in raw mode, so reading from stdin and copying straight to the master fd should be sufficient right?

There's probably edge cases like with handling resizes but we could probably hold off on that for now

brandonskiser · 2025-05-01T18:31:50Z

crates/q_cli/src/cli/chat/tools/execute_bash.rs

Where? it's already a UTF-8 type here though?

hayemaxi commented Apr 24, 2025

View reviewed changes

hayemaxi force-pushed the pty-final branch 2 times, most recently from 76424a7 to 6487fac Compare April 24, 2025 05:56

hayemaxi force-pushed the pty-final branch from 6487fac to 95957dd Compare April 24, 2025 05:57

hayemaxi mentioned this pull request Apr 24, 2025

Run commands in user's shell environment #1030

Open

hayemaxi marked this pull request as ready for review April 24, 2025 06:02

hayemaxi requested a review from a team April 24, 2025 06:02

jsamuel1 reviewed Apr 27, 2025

View reviewed changes

brandonskiser requested changes May 1, 2025

View reviewed changes

chaynabors force-pushed the main branch from 3311ed2 to bed5a9b Compare June 30, 2025 17:21

chaynabors requested a review from a team as a code owner June 30, 2025 17:21

dingfeli closed this Oct 17, 2025

feat(tools): execute_bash uses PTY for shell integrations #1340

feat(tools): execute_bash uses PTY for shell integrations #1340

Uh oh!

Conversation

hayemaxi commented Apr 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented Apr 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

hayemaxi commented Apr 24, 2025 •

edited

Loading

codecov-commenter commented Apr 24, 2025 •

edited

Loading