feat: delegate tab completion to PTY bash#146
Merged
Conversation
…pport Replace the custom Python completion system (PATH scanning + PathCompleter) with PTY bash delegation. A new __aish_query_completions function in bash_rc_wrapper.sh uses compgen and bash-completion functions to provide context-aware completions (e.g. git subcommands, systemctl units). Falls back to the original PATH scanning when PTY is unavailable.
📝 WalkthroughWalkthroughAdds an injectable ChangesPTY + Completion Integration
Sequence DiagramsequenceDiagram
participant Editor as ShellPromptController
participant Completer as ShellCompleter
participant PTY as PTYManager
participant Bash as bash_rc_wrapper.sh
participant Fallback as PathCompleter
Editor->>Completer: get_completions(line, cursor_pos)
Completer->>PTY: pty = pty_provider()
alt PTY available
Completer->>PTY: query_completions(line, cursor_pos, timeout)
PTY->>Bash: run __aish_query_completions(line, cursor_pos)
Bash->>Bash: compute cmd, cur, prev
Bash->>Bash: invoke completion function or run compgen -f
Bash->>PTY: return formatted candidates
PTY->>Completer: (candidates, exit_code)
Completer->>Completer: filter by token prefix, set start_position
Completer->>Editor: yield PTY completions
else PTY unavailable/error/no matches
Completer->>Fallback: query PATH and registry for candidates
Fallback->>Completer: candidates
Completer->>Editor: yield fallback completions
end
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)
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 |
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 `@src/aish/shell/ui/completion.py`:
- Around line 116-127: The unconditional return after iterating pty_results
suppresses fallback completions when local filtering leaves zero candidates;
change the logic in the completion generator (the block using pty_results,
current_token and yielding Completion) to only return when at least one
Completion was yielded (e.g., track a boolean yielded flag or detect any
matching candidate), otherwise fall through so fallback completions run instead
of exiting early.
In `@src/aish/terminal/pty/bash_rc_wrapper.sh`:
- Around line 291-323: Replace the manual tokenization loop that builds
words/cword from cmd_line and cursor with a call to bash-completion's helper:
when _get_comp_words_by_ref is available, invoke it to populate COMP_WORDS and
COMP_CWORD (e.g., _get_comp_words_by_ref -n : -- "${cmd_line}" cursor_var), then
set local cmd="${COMP_WORDS[0]:-}", cur="${COMP_WORDS[COMP_CWORD]:-}",
prev="${COMP_WORDS[$((COMP_CWORD-1))]:-}"; if _get_comp_words_by_ref is not
present fall back to the existing parsing logic that uses cmd_line, cursor,
words, cword; ensure the fallback preserves the current behavior but only runs
when the helper is absent.
🪄 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: c9965fc2-a17d-4c43-8e8a-9a82bab4d525
📒 Files selected for processing (4)
src/aish/shell/runtime/app.pysrc/aish/shell/ui/completion.pysrc/aish/shell/ui/editor.pysrc/aish/terminal/pty/bash_rc_wrapper.sh
… out When PTY returns candidates but none match the current prefix after local filtering, continue to the fallback completer instead of returning empty.
Collaborator
Author
|
#147 fixed |
F16shen
reviewed
May 7, 2026
Collaborator
F16shen
left a comment
F16shen
left a comment
There was a problem hiding this comment.
Two blocking issues found during manual verification.
There was a problem hiding this comment.
🧹 Nitpick comments (6)
tests/shell/ui/test_shell_editor.py (2)
15-23: 💤 Low valueMinor: type-annotate
callsfor clarity.
self.calls = []has an inferredlist[Any]type; an explicit annotation matches the rest of the test module's style and helps readers see the recorded tuple shape.♻️ Suggested annotation
-class FakeCompletionPTY: - def __init__(self, output: str = "status\n") -> None: - self.is_running = True - self.output = output - self.calls = [] +class FakeCompletionPTY: + def __init__(self, output: str = "status\n") -> None: + self.is_running = True + self.output = output + self.calls: list[tuple[str, int, float | None]] = []🤖 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 `@tests/shell/ui/test_shell_editor.py` around lines 15 - 23, The test helper FakeCompletionPTY should explicitly type-annotate its calls attribute to match module style and document the recorded tuple shape: change the assignment in FakeCompletionPTY.__init__ so self.calls is declared with a type like list[tuple[str, int, Optional[float]]] (or the appropriate types for line, cursor_pos, timeout) instead of an untyped list; update imports if needed and ensure query_completions appends tuples that conform to that annotation.
282-294: 💤 Low valueTest couples to the hardcoded 500 ms timeout.
The
0.5literal in the call-tracking assertion mirrors the value baked intoShellCompleter._query_pty_completions. If the timeout is ever made configurable (or tuned), this test will need to be updated in lockstep. Consider asserting on the first two positional args only, or pulling the timeout from a shared constant.♻️ Optional: decouple from the literal value
- assert fake_pty.calls == [("git chec kout main", 8, 0.5)] + assert len(fake_pty.calls) == 1 + line, cursor_pos, timeout = fake_pty.calls[0] + assert (line, cursor_pos) == ("git chec kout main", 8) + assert timeout is not None and timeout > 0🤖 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 `@tests/shell/ui/test_shell_editor.py` around lines 282 - 294, The test test_shell_completer_queries_pty_with_full_line_and_cursor is brittle because it asserts the hardcoded 0.5 timeout value; update the assertion to avoid coupling to ShellCompleter._query_pty_completions' internal timeout by only checking the first two positional args on FakeCompletionPTY.calls (the input text and cursor_position) or by reading the timeout from a shared constant if one exists; locate FakeCompletionPTY and ShellCompleter._query_pty_completions in this test and change the final assertion to assert fake_pty.calls == [("git chec kout main", 8)] or compare only the slice of the call tuple containing the first two elements.src/aish/shell/ui/completion.py (2)
137-158: 💤 Low valueOptional: hoist the PTY-completion timeout to a constant or constructor arg.
timeout=0.5is duplicated in spirit with the test assertion and is the kind of value most likely to need tuning per environment (slow remote filesystems, heavy bash-completion loaders like__git_complete). Promoting it to a module-level constant or apty_timeoutconstructor arg makes both tuning and testing easier without behavior change today.♻️ Suggested shape
AI_PREFIXES = (";", "；") +PTY_COMPLETION_TIMEOUT = 0.5 @@ def __init__( self, cwd_provider: Optional[Callable[[], str]] = None, command_provider: Optional[Callable[[], Iterable[str]]] = None, ai_prefixes: tuple[str, ...] = AI_PREFIXES, pty_provider: Optional[Callable[[], Optional["PTYManager"]]] = None, + pty_timeout: float = PTY_COMPLETION_TIMEOUT, ) -> None: @@ - self._pty_provider = pty_provider + self._pty_provider = pty_provider + self._pty_timeout = pty_timeout @@ - output, exit_code = pty_manager.query_completions( - line, - cursor_pos, - timeout=0.5, - ) + output, exit_code = pty_manager.query_completions( + line, + cursor_pos, + timeout=self._pty_timeout, + )🤖 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 `@src/aish/shell/ui/completion.py` around lines 137 - 158, Hoist the hard-coded timeout in _query_pty_completions by adding a configurable pty timeout (either a module-level constant or a constructor arg on the class that owns _query_pty_completions) and use that value instead of timeout=0.5 in the call to pty_manager.query_completions; give the new config a default of 0.5 so behavior is unchanged, and update any tests that asserted on the literal timeout to reference the new constant/attribute.
145-152: 💤 Low valueBroad
except Exception:is acceptable but considerBaseExceptionboundaries.Ruff flags BLE001 here. Catching
Exceptionis defensible for a UX fallback path (we never want a PTY hiccup to break the editor), but it will silently swallow programming errors during development. Logging at debug level — without changing the fallback behavior — would preserve resilience while making real bugs discoverable. Not a blocker.♻️ Suggested change
+import logging + +_logger = logging.getLogger(__name__) @@ - try: - output, exit_code = pty_manager.query_completions( - line, - cursor_pos, - timeout=0.5, - ) - except Exception: - return None + try: + output, exit_code = pty_manager.query_completions( + line, + cursor_pos, + timeout=0.5, + ) + except Exception: # noqa: BLE001 - completion must never break the editor + _logger.debug("PTY completion query failed", exc_info=True) + return None🤖 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 `@src/aish/shell/ui/completion.py` around lines 145 - 152, The try/except around pty_manager.query_completions currently swallows all Exceptions silently; keep the fallback behavior (return None) but log the caught exception at debug level so programming errors are discoverable during development. Modify the except Exception: block that surrounds pty_manager.query_completions(line, cursor_pos, timeout=0.5) to call the module/logger debug method with the exception info (e.g., logger.debug(..., exc_info=True) or similar) before returning None, and do not change the return None fallback or broaden to BaseException.src/aish/terminal/pty/manager.py (2)
521-546: 💤 Low valueLGTM — clean separation of internal vs backend execution.
execute_internal_commandandquery_completionsare tight wrappers that route through the same_exec_via_*helpers withsource=INTERNAL_SOURCE.shlex.quote(line)is the right escaping choice for the bash arg.One follow-up worth considering:
cursor_posis forwarded as-is to the bash function. The__aish_query_completionsbash helper accesses the cursor value directly in substring expansion (${cmd_line:$((cursor-1)):1}) without bounds validation. Negative or out-of-range values would cause unpredictable behavior rather than graceful clamping. A defensive bound on the Python side would make the contract clearer:♻️ Optional defensive clamp
def query_completions( self, line: str, cursor_pos: int, timeout: Optional[float] = 0.5 ) -> tuple[str, int]: """Query backend bash completions without changing shell command state.""" + cursor_pos = max(0, min(cursor_pos, len(line))) escaped_line = shlex.quote(line) command = f"__aish_query_completions {escaped_line} {cursor_pos}" return self.execute_internal_command(command, timeout=timeout)🤖 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 `@src/aish/terminal/pty/manager.py` around lines 521 - 546, The query_completions helper forwards cursor_pos directly to the bash helper __aish_query_completions, but that bash function uses ${cmd_line:$((cursor-1)):1} and will misbehave for negative or out-of-range values; clamp and coerce cursor_pos in query_completions to a safe integer before building the command (e.g. cast to int, ensure cursor = max(1, min(cursor, len(line))) or otherwise clamp into the valid 1..len(line) range), then use that clamped value when composing the __aish_query_completions invocation so execute_internal_command receives a validated cursor argument.
502-538: ⚖️ Poor tradeoffUpdate reference from
execute_internal_commandtoexecute_command; concern remains valid.The timeout+Ctrl+C behavior exists in the current
execute_commandimplementation (sent in_exec_via_thread/_exec_via_pollwhen deadline expires). This method is actively used for completion probes atsrc/aish/shell/ui/completion.py:0.5timeout, so the race condition described—where SIGINT could land on a user's foreground process if one started between completion submit and timeout—is theoretically possible, though unlikely. Worth a quick mental check; not necessarily a code change.🤖 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 `@src/aish/terminal/pty/manager.py` around lines 502 - 538, Race condition: timeout-triggered Ctrl+C from execute_command can interrupt a user's foreground process; inspect _exec_via_thread and _exec_via_poll to harden signal delivery. Locate the deadline-expiry path in _exec_via_thread/_exec_via_poll (paths used by execute_command and execute_internal_command) and ensure the interrupt targets the intended child/PTY process group (e.g., send SIGINT to the child PID or its process group) instead of writing a raw Ctrl-C to the PTY that could be delivered to a newly started foreground job; alternatively run completion probes in a dedicated process group or use an isolated pty/session so timeout signals cannot race into the user's foreground process. Ensure the chosen fix is applied consistently for calls from execute_command and for completion probes.
🤖 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.
Nitpick comments:
In `@src/aish/shell/ui/completion.py`:
- Around line 137-158: Hoist the hard-coded timeout in _query_pty_completions by
adding a configurable pty timeout (either a module-level constant or a
constructor arg on the class that owns _query_pty_completions) and use that
value instead of timeout=0.5 in the call to pty_manager.query_completions; give
the new config a default of 0.5 so behavior is unchanged, and update any tests
that asserted on the literal timeout to reference the new constant/attribute.
- Around line 145-152: The try/except around pty_manager.query_completions
currently swallows all Exceptions silently; keep the fallback behavior (return
None) but log the caught exception at debug level so programming errors are
discoverable during development. Modify the except Exception: block that
surrounds pty_manager.query_completions(line, cursor_pos, timeout=0.5) to call
the module/logger debug method with the exception info (e.g., logger.debug(...,
exc_info=True) or similar) before returning None, and do not change the return
None fallback or broaden to BaseException.
In `@src/aish/terminal/pty/manager.py`:
- Around line 521-546: The query_completions helper forwards cursor_pos directly
to the bash helper __aish_query_completions, but that bash function uses
${cmd_line:$((cursor-1)):1} and will misbehave for negative or out-of-range
values; clamp and coerce cursor_pos in query_completions to a safe integer
before building the command (e.g. cast to int, ensure cursor = max(1,
min(cursor, len(line))) or otherwise clamp into the valid 1..len(line) range),
then use that clamped value when composing the __aish_query_completions
invocation so execute_internal_command receives a validated cursor argument.
- Around line 502-538: Race condition: timeout-triggered Ctrl+C from
execute_command can interrupt a user's foreground process; inspect
_exec_via_thread and _exec_via_poll to harden signal delivery. Locate the
deadline-expiry path in _exec_via_thread/_exec_via_poll (paths used by
execute_command and execute_internal_command) and ensure the interrupt targets
the intended child/PTY process group (e.g., send SIGINT to the child PID or its
process group) instead of writing a raw Ctrl-C to the PTY that could be
delivered to a newly started foreground job; alternatively run completion probes
in a dedicated process group or use an isolated pty/session so timeout signals
cannot race into the user's foreground process. Ensure the chosen fix is applied
consistently for calls from execute_command and for completion probes.
In `@tests/shell/ui/test_shell_editor.py`:
- Around line 15-23: The test helper FakeCompletionPTY should explicitly
type-annotate its calls attribute to match module style and document the
recorded tuple shape: change the assignment in FakeCompletionPTY.__init__ so
self.calls is declared with a type like list[tuple[str, int, Optional[float]]]
(or the appropriate types for line, cursor_pos, timeout) instead of an untyped
list; update imports if needed and ensure query_completions appends tuples that
conform to that annotation.
- Around line 282-294: The test
test_shell_completer_queries_pty_with_full_line_and_cursor is brittle because it
asserts the hardcoded 0.5 timeout value; update the assertion to avoid coupling
to ShellCompleter._query_pty_completions' internal timeout by only checking the
first two positional args on FakeCompletionPTY.calls (the input text and
cursor_position) or by reading the timeout from a shared constant if one exists;
locate FakeCompletionPTY and ShellCompleter._query_pty_completions in this test
and change the final assertion to assert fake_pty.calls == [("git chec kout
main", 8)] or compare only the slice of the call tuple containing the first two
elements.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: afc6337d-a6e2-418f-bceb-1f0b26a3554b
📒 Files selected for processing (5)
src/aish/shell/ui/completion.pysrc/aish/terminal/pty/command_state.pysrc/aish/terminal/pty/manager.pytests/shell/ui/test_shell_editor.pytests/terminal/pty/test_exit_tracker_repeat_error.py
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
2 participants
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
__aish_query_completionsand__aish_mark_dirsfunctions tobash_rc_wrapper.sh, leveragingcompgenand bash-completion functions for context-aware completionsgit add <tab>,systemctl <tab>, etc.Test plan
git <tab>shows git subcommandssystemctl <tab>shows systemctl subcommandsls /tmp/<tab>shows file/directory completions/mod<tab>completes to/model(special aish commands still work);correctly skips completionSummary by CodeRabbit
New Features
Bug Fixes
Tests