fix: improve interactive command handling and tool cancellation#139
Merged
Conversation
📝 WalkthroughWalkthroughPropagates a shared, Arc-wrapped cancellation token from LLM session through AiHandler into Bash/SecureBash tools; updates BashTool to observe and bridge AI cancellation, enhances interactive-command detection; refines PTY session startup/draining behavior and shell readline hinting/history seeding. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User as "User"
participant App as "App / AiHandler"
participant LLM as "LlmSession\n(Arc<CancellationToken>)"
participant ToolReg as "SecureBashTool"
participant Bash as "BashTool"
participant PTY as "PTY Process"
rect rgba(200,230,255,0.5)
User->>App: invoke AI action / run command
App->>LLM: hold / clone cancellation token (cancellation_token_arc)
App->>ToolReg: construct SecureBashTool
App->>ToolReg: set_cancellation_token(Arc<CancellationToken>)
ToolReg->>Bash: forward token to inner BashTool
Bash->>PTY: execute command via PTY
end
rect rgba(255,200,200,0.5)
Note right of LLM: External cancel triggered
LLM-->>Bash: Arc token cancelled (observed via bridge thread)
Bash->>PTY: send local cancel / stop polling
PTY-->>Bash: exit/EOF
Bash-->>ToolReg: execution finished
ToolReg-->>App: notify completion
end
Estimated code review effort🎯 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)
Comment |
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:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/aish-pty/src/bash_rc_wrapper.sh`:
- Around line 171-181: The session command detection currently uses local
__aish_cmd_name="${BASH_COMMAND%% *}" which can include an absolute path (e.g.,
/usr/bin/ssh) so the case in the wrapper (matching ssh|telnet|...) will miss
such calls; change the extraction to strip any directory components and use the
basename of BASH_COMMAND (e.g., via parameter expansion to remove everything up
to the last '/') before the case check so names like /usr/bin/ssh match the ssh
pattern (update references to __aish_cmd_name and the case block accordingly to
use the basename).
In `@crates/aish-shell/src/readline.rs`:
- Around line 359-364: The loop that loads history reverses iteration causing
older entries to be prioritized; in the block where you check
self.editor.load_history(path).is_ok() and then iterate history =
self.editor.history(), remove the .rev() so you iterate in forward chronological
order (oldest→newest) while calling guard.add(...) (AutoSuggest::add uses
push_front), ensuring the newest commands end up at the front of the suggestion
queue; update the iteration over Editor::history().iter() accordingly.
🪄 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: f77194de-da0d-4126-9695-71427c379dc0
📒 Files selected for processing (8)
crates/aish-llm/src/session.rscrates/aish-pty/src/bash_rc_wrapper.shcrates/aish-pty/src/persistent.rscrates/aish-shell/src/ai_handler.rscrates/aish-shell/src/app.rscrates/aish-shell/src/readline.rscrates/aish-tools/src/bash.rscrates/aish-tools/src/secure_bash.rs
- Add cancellation token bridging from AI handler to BashTool so Ctrl+C during AI tool execution actually terminates the PTY child process - Expand interactive command detection with explicit lists for TUI programs (vim, htop, less) and session commands (ssh, telnet, mosh) - Re-enable echo and output processing (OPOST/ONLCR) for session commands so remote PTY displays correctly - Extract basename in bash DEBUG trap so absolute paths like /usr/bin/ssh are correctly matched - Improve PTY initialization: wait_for_session_ready now reports whether PromptReady co-arrived, add drain_control_pipe_raw to prevent stale events from shifting exit codes - Enhance readline: gray hint highlighting, pre-populate autosuggest from saved history (oldest-first for correct recency), increase autosuggest capacity to 5000
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/aish-pty/src/persistent.rs`:
- Around line 662-676: In PersistentPty::start the temporary read into mtmp can
drop data because output_callback is None; change the read-handling in the
unsafe libc::read block so that if self.output_callback is None you append the
bytes read to an internal pending buffer (e.g., pending_master_output) or
forward them to the existing drain_master_to_stdout logic, otherwise call the
callback as before; update the struct to hold that pending buffer and ensure
drain_master_to_stdout consumes and clears it when invoked (reference symbols:
PersistentPty::start, output_callback, mtmp, drain_master_to_stdout, master_fd).
In `@crates/aish-tools/src/bash.rs`:
- Around line 37-43: The current check uses the first lexical token (let first =
command.split_whitespace().next()) which misses wrapper/env prefixes; update the
logic in crates/aish-tools/src/bash.rs to iterate over
command.split_whitespace() and skip leading tokens that are environment
assignments (tokens containing '=') or known wrapper tokens (e.g., "env",
"command", "nohup", "TERM" wrappers—introduce a small WRAPPER_TOKENS set), then
take the next non-skipped token as first and derive basename from it before
testing INTERACTIVE_COMMANDS and SESSION_COMMANDS; apply the same skip behavior
to the mirrored places in crates/aish-pty/src/bash_rc_wrapper.sh and
crates/aish-pty/src/persistent.rs so interactive/session routing works
end-to-end.
🪄 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: 0985ea96-c538-44f4-9c55-ebe1559c0357
📒 Files selected for processing (8)
crates/aish-llm/src/session.rscrates/aish-pty/src/bash_rc_wrapper.shcrates/aish-pty/src/persistent.rscrates/aish-shell/src/ai_handler.rscrates/aish-shell/src/app.rscrates/aish-shell/src/readline.rscrates/aish-tools/src/bash.rscrates/aish-tools/src/secure_bash.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/aish-tools/src/secure_bash.rs
- crates/aish-llm/src/session.rs
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.
Summary
Test plan
cargo build— 0 warningscargo clippy— cleancargo test— 524 passed, 0 failed, 3 ignoredssh user@hostfrom aish and verify remote prompt displays correctlySummary by CodeRabbit
New Features
Bug Fixes
Improvements