fix: route sudo bash_exec through PTY#135
Merged
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 |
github-actions
Bot
added
tools
📝 WalkthroughWalkthroughThis PR enhances PTY-based command execution by replacing file descriptor re-opening with Changes
Sequence DiagramsequenceDiagram
actor User
participant BashTool
participant InteractiveDetector
participant SharedPTY
participant DedicatedPTY
participant Subprocess
User->>BashTool: execute("sudo whoami")
BashTool->>InteractiveDetector: _needs_interactive_bash("sudo whoami")
InteractiveDetector-->>BashTool: true
BashTool->>DedicatedPTY: execute(cmd, use_pty=True)
DedicatedPTY->>Subprocess: Popen with PTY slave
Subprocess->>Subprocess: ioctl(TIOCSCTTY) - set controlling terminal
Subprocess-->>DedicatedPTY: result (with stdin interactive)
DedicatedPTY-->>BashTool: output
BashTool-->>User: result (no stdout collapse)
User->>BashTool: execute("echo test")
BashTool->>InteractiveDetector: _needs_interactive_bash("echo test")
InteractiveDetector-->>BashTool: false
BashTool->>SharedPTY: execute(cmd)
SharedPTY->>Subprocess: Popen (standard)
Subprocess-->>SharedPTY: result
SharedPTY-->>BashTool: output
BashTool-->>User: result (with stdout collapse)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
F16shen
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/aish/tools/bash_executor.py (1)
290-294: Defensive check is unreachable but harmless.
process.wait(timeout=0.1)either returns the exit code or raisessubprocess.TimeoutExpired—it never returnsNone. The None-check on line 292-293 is dead code but doesn't cause harm. Consider removing it for clarity, or wrapping in a try/except forTimeoutExpiredif you want to handle the timeout case explicitly.♻️ Optional: Handle TimeoutExpired explicitly
# Wait for process to end - returncode = process.wait(timeout=0.1) - if returncode is None: - returncode = 0 + try: + returncode = process.wait(timeout=0.1) + except subprocess.TimeoutExpired: + returncode = 0🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/aish/tools/bash_executor.py` around lines 290 - 294, The None-check after calling process.wait(timeout=0.1) is unreachable because subprocess.wait either returns an integer exit code or raises subprocess.TimeoutExpired; update the logic around the process.wait call in bash_executor.py to explicitly handle the timeout instead of checking for None: wrap process.wait(timeout=0.1) in a try/except catching subprocess.TimeoutExpired and set a sensible returncode (e.g., None, -1, or leave running) or remove the dead None-check entirely; refer to the process.wait(timeout=0.1) invocation and the surrounding returncode variable to locate and adjust the code.src/aish/tools/code_exec.py (1)
27-29: Heuristic is minimal but functional; consider documenting edge cases.The heuristic correctly identifies common
sudo/supatterns. Note that it won't catch:
suwithout arguments (user might just typesu)- Alternative privilege escalation tools like
doasorpkexecSince the PR explicitly describes this as a "minimal heuristic," this is acceptable. Consider adding a comment noting these limitations for future maintainers.
📝 Optional: Add docstring noting limitations
def _needs_interactive_bash(command: str) -> bool: + """Minimal heuristic to detect commands requiring interactive PTY. + + Catches common sudo/su patterns. Does not cover all privilege + escalation tools (e.g., doas, pkexec) or bare 'su' without args. + """ lower = command.lower() return "sudo" in lower or " su " in lower or lower.startswith("su ")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/aish/tools/code_exec.py` around lines 27 - 29, The _needs_interactive_bash function uses a minimal heuristic that only checks for "sudo" and common "su" patterns; add a short docstring or inline comment on the _needs_interactive_bash function explaining its limitations (it won't detect bare "su" without args, or other escalation tools like doas or pkexec) and note that this is intentionally minimal so future maintainers know why more exhaustive checks were not implemented and where to extend detection.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/aish/tools/bash_executor.py`:
- Around line 290-294: The None-check after calling process.wait(timeout=0.1) is
unreachable because subprocess.wait either returns an integer exit code or
raises subprocess.TimeoutExpired; update the logic around the process.wait call
in bash_executor.py to explicitly handle the timeout instead of checking for
None: wrap process.wait(timeout=0.1) in a try/except catching
subprocess.TimeoutExpired and set a sensible returncode (e.g., None, -1, or
leave running) or remove the dead None-check entirely; refer to the
process.wait(timeout=0.1) invocation and the surrounding returncode variable to
locate and adjust the code.
In `@src/aish/tools/code_exec.py`:
- Around line 27-29: The _needs_interactive_bash function uses a minimal
heuristic that only checks for "sudo" and common "su" patterns; add a short
docstring or inline comment on the _needs_interactive_bash function explaining
its limitations (it won't detect bare "su" without args, or other escalation
tools like doas or pkexec) and note that this is intentionally minimal so future
maintainers know why more exhaustive checks were not implemented and where to
extend detection.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 62eec3e4-9e7c-4fc8-8a46-dcbaf2ce5730
📒 Files selected for processing (4)
src/aish/tools/bash_executor.pysrc/aish/tools/code_exec.pytests/tools/test_bash_executor.pytests/tools/test_bash_output_offload.py
5 tasks
This was referenced May 15, 2026
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.
概述
bash_exec在共享 PTY 路径下执行sudo/su一类命令时，会停在密码提示处但无法接收用户输入，导致工具调用卡住。sudo/su增加最小启发式分流，命中时绕过共享PTYManager，改走UnifiedBashExecutor.execute(use_pty=True)；同时修复旧 PTY 执行器的 controlling terminal 建立逻辑，并补充针对分流与 PTY 启动的测试。改动类型
涉及范围
用户可见变更
bash_exec遇到sudo/su时不再卡死在密码提示处，用户输入会被转发到命令。bash_exec调用仍可继续使用。兼容性
测试验证
pytest tests/tools/test_bash_executor.py tests/tools/test_bash_output_offload.py -qbash_exec执行sudo -k whoami，确认出现密码提示并可消费错误密码输入，命令结束后提示符恢复。sudo失败流程后，再执行echo shell-alive与bash_exec("echo tool-ok")，确认 shell 和工具链均恢复正常。检查清单
Summary by CodeRabbit
New Features
Bug Fixes
Tests