fix: restore interactive sudo input and suppress duplicate bash output#186
Merged
F16shen merged 3 commits intoMay 15, 2026
Merged
Conversation
Contributor
|
Thanks for the pull request. A maintainer will review it when available. Please keep the PR focused, explain the why in the description, and make sure local checks pass before requesting review. Contribution guide: https://github.com/AI-Shell-Team/aish/blob/main/CONTRIBUTING.md |
Contributor
|
This pull request description looks incomplete. Please update the missing sections below before review. Missing items:
|
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThis PR enhances interactive bash command execution by adding detection, display control, and event enrichment. It exposes interactive-command detection to the shell, adds PTY display control with authentication-noise filtering, enriches tool events with command arguments, and coordinates shell loops with interactive input status to avoid interference during password prompts. ChangesInteractive Bash Tool Execution
Sequence DiagramsequenceDiagram
participant Shell as ShellApp
participant Bash as BashTool
participant PTY as PersistentPty
participant EventLoop as EventCallback
Shell->>Bash: execute bash tool command
Bash->>Bash: detect if command needs interactive
Bash->>Bash: set INTERACTIVE_INPUT_ACTIVE = true
Bash->>PTY: execute_command(..., display_output)
PTY->>PTY: clean output, strip auth noise
PTY-->>Bash: cleaned output + exit code
Bash->>Bash: reset INTERACTIVE_INPUT_ACTIVE = false
Bash-->>Shell: command result
Shell->>EventLoop: emit ToolExecutionEnd with tool_args
EventLoop->>EventLoop: suppress preview if interactive
EventLoop->>Shell: event handled
🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
crates/aish-pty/src/persistent.rs (1)
3571-3579: ⚡ Quick winNarrow auth-prompt filtering trigger to command token, not substring.
command_may_prompt_for_authmatches with broadcontains(...), so unrelated commands that merely include words likesudo/sshin arguments can trigger auth-line stripping and lose valid output. Parsing first executable token (same style as interactive detection) would reduce false positives.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/aish-pty/src/persistent.rs` around lines 3571 - 3579, The auth prompt detector command_may_prompt_for_auth currently uses substring checks and should instead inspect only the command's first token (executable) to avoid false positives; change it to split_whitespace() (or the same token parsing used by the interactive detection) and run checks against that first token (e.g., token == "sudo" or token.starts_with("su") or token == "ssh") rather than using contains()/contains(" ssh "), so only the actual invoked program triggers auth-line stripping.crates/aish-shell/src/app.rs (1)
1708-1713: ⚡ Quick winUse
lock_pty()here for consistent poison recovery.This path bypasses the helper introduced earlier in the file, so a poisoned PTY mutex will panic here even though the normal execution paths recover and keep going.
♻️ Suggested change
- let _ = self.pty.lock().unwrap().execute_command( + let _ = self.lock_pty().execute_command( command, std::time::Duration::from_secs(5), None, false, );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/aish-shell/src/app.rs` around lines 1708 - 1713, The call uses self.pty.lock().unwrap().execute_command(...) which will panic on a poisoned mutex; replace this direct lock with the helper lock_pty() used elsewhere to get the PTY guard (so poison recovery is applied) and then call execute_command on that guard with the same arguments; specifically, locate the occurrence of self.pty.lock().unwrap().execute_command and change it to use lock_pty() to obtain the guard before calling execute_command.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/aish-llm/src/session.rs`:
- Around line 1463-1467: The new test block has formatting drift around the
let-binding for result at session.execute_tool_external(&tool_call).await and
the seen_event lock/unwrap clone lines; run rustfmt (cargo fmt) to reformat the
file so the let-binding wrapping and subsequent assertions match project style,
or manually adjust the indentation/line-wrapping in the test (around
session.execute_tool_external,
seen_event.lock().unwrap().clone().expect("missing ToolExecutionEnd event"), and
the assert_eq! block) to match rustfmt output.
In `@crates/aish-pty/src/persistent.rs`:
- Around line 292-300: The display_output branch currently calls libc::write and
ignores its return value (involving tmp, n, and libc::STDOUT_FILENO), which can
drop data on short writes; replace that call with a small write-all loop that
retries on EINTR and advances the buffer by the number of bytes written until
all n bytes are written (handling partial writes and returning or breaking on
unrecoverable errors). Implement the loop around libc::write in the same unsafe
block used now, check for negative returns to map errno, treat EINTR as retry,
subtract the bytes_written from the remaining count and advance the pointer (or
index) accordingly, and only exit when total bytes written equals n or an
unrecoverable error occurs.
In `@crates/aish-shell/src/app.rs`:
- Around line 3417-3418: The regex initializer for TOOL_XML_RETURN_CODE_RE
(assigned to re_return_code via get_or_init and calling
regex::Regex::new(r"(?s)<(?:return_code|exit-code)>.*?</(?:return_code|exit-code)>").unwrap())
is not formatted to Rustfmt standards; run rustfmt (or `cargo fmt`) to reformat
this block so the expression, method chaining, and indentation meet `cargo fmt
--check`, then commit the formatted change so CI passes.
- Around line 3416-3419: The current regex TOOL_XML_RETURN_CODE_RE (used to
initialize re_return_code) is applied to the entire preview string (cleaned) and
can remove legitimate <return_code> or <exit-code> fragments from command
stdout; instead, first isolate the tool metadata block (the trailing wrapper
content inside the <stdout> or the metadata section) and apply the regex only to
that substring, or move the replace_all call to after you peel off the <stdout>
wrapper; update the logic around the variable cleaned and the use of
re_return_code so you only strip return-code/exit-code tags within the tool
metadata region rather than the full preview.
In `@crates/aish-tools/src/bash.rs`:
- Line 269: The CI failure is due to formatting at the call site of
pty.execute_command(command, command_timeout, Some(&cancel_token), interactive);
— run rustfmt (or cargo fmt) to reformat crates/aish-tools/src/bash.rs so the
call wraps/aligns per rustfmt rules (or manually adjust the call to a
rustfmt-friendly layout, e.g., place args on separate indented lines) and re-run
cargo fmt --check to ensure the formatting issue is resolved.
- Around line 266-272: Replace the global boolean INTERACTIVE_INPUT_ACTIVE
toggles with an atomic reference count: increment (fetch_add(1)) before calling
pty.execute_command(...) and decrement (fetch_sub(1)) after it (ensuring you
never underflow) so overlapping interactive commands keep the flag active until
the last one finishes; update both occurrences where you currently call
INTERACTIVE_INPUT_ACTIVE.store(true/false, Ordering::SeqCst) around
pty.execute_command (the blocks using interactive, INTERACTIVE_INPUT_ACTIVE,
pty.execute_command, command_timeout and cancel_token) to use the atomic counter
instead and use Ordering::SeqCst for increments/decrements to preserve ordering.
---
Nitpick comments:
In `@crates/aish-pty/src/persistent.rs`:
- Around line 3571-3579: The auth prompt detector command_may_prompt_for_auth
currently uses substring checks and should instead inspect only the command's
first token (executable) to avoid false positives; change it to
split_whitespace() (or the same token parsing used by the interactive detection)
and run checks against that first token (e.g., token == "sudo" or
token.starts_with("su") or token == "ssh") rather than using
contains()/contains(" ssh "), so only the actual invoked program triggers
auth-line stripping.
In `@crates/aish-shell/src/app.rs`:
- Around line 1708-1713: The call uses
self.pty.lock().unwrap().execute_command(...) which will panic on a poisoned
mutex; replace this direct lock with the helper lock_pty() used elsewhere to get
the PTY guard (so poison recovery is applied) and then call execute_command on
that guard with the same arguments; specifically, locate the occurrence of
self.pty.lock().unwrap().execute_command and change it to use lock_pty() to
obtain the guard before calling execute_command.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e23a3366-5d34-44b5-a1fe-c96018010d2f
📒 Files selected for processing (5)
crates/aish-llm/src/session.rscrates/aish-pty/src/persistent.rscrates/aish-shell/src/app.rscrates/aish-shell/src/readline.rscrates/aish-tools/src/bash.rs
F16shen
force-pushed
the
fix/sudo-bash-interactive-output
branch
from
eaf7835 to
a7a16be
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Background
AI-triggered interactive bash commands had two regressions on the Rust shell path:
sudopassword prompts could lose stdin ownership while the shell was still polling for AI eventsChanges
ToolExecutionEndso the shell UI can recognize interactive bash completionsValidation
cargo test -p aish-pty test_clean_pty_output --lib -- --nocapturecargo test -p aish-llm tool_execution_end_event_includes_tool_args --lib -- --nocapturecargo test -p aish-shell collapsing_tests:: --lib -- --nocapturecargo build -p aish-cliRisk
The main risk is divergence between interactive and non-interactive bash execution behavior, especially around PTY cleanup and tool preview rendering.
Summary by CodeRabbit
Bug Fixes
New Features