fix(pty): clear stale line-discipline input before each command#134
Conversation
|
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 |
|
This pull request description looks incomplete. Please update the missing sections below before review. Missing items:
|
📝 WalkthroughWalkthroughPrefixes PTY command writes with Ctrl‑U and switches to byte-buffer construction; autosuggestion trims leading whitespace and makes UTF‑8 slicing safe; completion aligns cursor to prior UTF‑8 boundary; Python tool truncates stdout to 1000 chars, embeds cwd into the wrapper, and sets Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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 |
a222ee3 to
bb6e2d7
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/aish-tools/src/python.rs (1)
53-137:⚠️ Potential issue | 🟡 MinorCI is blocked by rustfmt.
The pipeline reports
cargo fmt --checkfailure. Please runcargo fmt --allbefore merge.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/aish-tools/src/python.rs` around lines 53 - 137, The file fails rustfmt; run rustfmt (cargo fmt --all) to reformat the code so execute(...) (and related helpers like indent_each_line, wrapper, MAX_OUTPUT_CHARS) comply with formatting rules — simply run cargo fmt --all locally or in CI and commit the resulting changes so the rustfmt check passes.
🧹 Nitpick comments (1)
crates/aish-pty/src/persistent.rs (1)
146-163: Extract the PTY command framing into one helper.
send_commandandsend_command_interactivenow both hand-rollCtrl-U + command + '\n', with only the backend metadata differing. Keeping that protocol in one place will make the next PTY fix much less likely to land in only one path.Proposed refactor
+fn build_command_payload(command: &str, seq: Option<i32>) -> Vec<u8> { + let mut payload = vec![0x15]; + if let Some(s) = seq { + let quoted = shell_quote_escape(command); + payload.extend_from_slice( + format!( + " __AISH_ACTIVE_COMMAND_SEQ={s}; __AISH_ACTIVE_COMMAND_TEXT={quoted}; " + ) + .as_bytes(), + ); + } + payload.extend_from_slice(command.as_bytes()); + payload.push(b'\n'); + payload +} + pub fn send_command(&mut self, command: &str, seq: Option<i32>) -> aish_core::Result<()> { ... - let mut payload = b"\x15".to_vec(); - if let Some(s) = seq { - let quoted = shell_quote_escape(command); - payload.extend_from_slice(...); - } - payload.extend_from_slice(command.as_bytes()); - payload.push(b'\n'); + let payload = build_command_payload(command, seq); self.write_master(&payload) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/aish-pty/src/persistent.rs` around lines 146 - 163, Both send_command and send_command_interactive duplicate the PTY framing (Ctrl-U + optional metadata + command + '\n'); extract that logic into a single helper (e.g., fn frame_pty_command(command: &str, seq: Option<&str>) -> Vec<u8>) and make both send_command and send_command_interactive call it. The helper should: prepend b"\x15", if seq.is_some() add the metadata fragment using shell_quote_escape for the command and the sequence substitution, append command.as_bytes() and a trailing b'\n', and return the Vec<u8> which the callers then pass to self.write_master; update the callers to remove the duplicated construction and reuse the new helper.
🤖 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 228-235: The bug is that pending stdin bytes in write_buf can be
written before the Ctrl-U NAK is sent, so stale lines execute; fix by ensuring
the NAK is injected before any buffered input when we enter the
PromptReady/drain phase (not appended after/only to the outgoing command).
Locate the transition handling PromptReady/entering drain and change the logic
that builds write_buf or calls write_master (symbols: write_buf, PromptReady,
command_state, write_master, payload) to prepend a single 0x15 byte ahead of any
existing buffered stdin and the new command (e.g. build a new buffer = [0x15] +
existing write_buf + command + '\n' or insert 0x15 at the front of write_buf) so
the PTY line discipline clears stale input before anything else is written.
In `@crates/aish-shell/src/readline.rs`:
- Around line 137-142: The code block in readline.rs that clamps `pos` to a
valid UTF-8 character boundary (using `line.is_char_boundary(pos)` and the
reversed range search) is not formatted per rustfmt; run `cargo fmt --all` (or
`cargo fmt`) to reformat the block and commit the changes so `cargo fmt --check`
passes for the `pos` clamping code in the `readline` module.
In `@crates/aish-tools/src/python.rs`:
- Around line 78-79: The injected Python wrapper string in
crates/aish-tools/src/python.rs currently catches exceptions with "except
Exception as e: print(..., file=sys.stderr)" which can make a failure appear as
a successful run; modify that except block to call sys.exit(1) (or re-raise)
after printing the error so the process exit code is non-zero, and then update
the caller logic that treats success as "exit_code == 0 && stdout.is_empty()"
(lines ~103-107) to instead consider stderr and/or the exit_code (e.g., require
exit_code == 0 and stderr.is_empty(), or simply rely on exit_code != 0 to signal
failure) so caught Python exceptions are not reported as ok.
- Around line 95-98: The current truncation slices the UTF-8 string by bytes
(stdout[..MAX_OUTPUT_CHARS]) which can panic on multibyte characters; change the
truncation to find a safe byte boundary using char indices: compute the byte
index of the Nth char via
stdout.char_indices().nth(MAX_OUTPUT_CHARS).map(|(i,_)|
i).unwrap_or(stdout.len()) and use that byte index to slice, then append the
"[Output truncated due to length]" message; reference stdout and
MAX_OUTPUT_CHARS in your change and update the block that builds the truncated s
accordingly.
---
Outside diff comments:
In `@crates/aish-tools/src/python.rs`:
- Around line 53-137: The file fails rustfmt; run rustfmt (cargo fmt --all) to
reformat the code so execute(...) (and related helpers like indent_each_line,
wrapper, MAX_OUTPUT_CHARS) comply with formatting rules — simply run cargo fmt
--all locally or in CI and commit the resulting changes so the rustfmt check
passes.
---
Nitpick comments:
In `@crates/aish-pty/src/persistent.rs`:
- Around line 146-163: Both send_command and send_command_interactive duplicate
the PTY framing (Ctrl-U + optional metadata + command + '\n'); extract that
logic into a single helper (e.g., fn frame_pty_command(command: &str, seq:
Option<&str>) -> Vec<u8>) and make both send_command and
send_command_interactive call it. The helper should: prepend b"\x15", if
seq.is_some() add the metadata fragment using shell_quote_escape for the command
and the sequence substitution, append command.as_bytes() and a trailing b'\n',
and return the Vec<u8> which the callers then pass to self.write_master; update
the callers to remove the duplicated construction and reuse the new helper.
🪄 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: 5c60a8c6-ef2f-4aea-8fc9-8f1dce50d656
📒 Files selected for processing (3)
crates/aish-pty/src/persistent.rscrates/aish-shell/src/readline.rscrates/aish-tools/src/python.rs
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
crates/aish-tools/src/python.rs (2)
78-79:⚠️ Potential issue | 🔴 CriticalCaught Python exceptions can still be treated as successful execution.
Line 78 prints the exception but does not exit non-zero, so failures can be surfaced as success when stdout is empty.
💡 Proposed fix
- let wrapper = format!( - "import sys,os\nos.chdir({cwd_repr})\ntry:\n{indented_code}\nexcept Exception as e:\n print(f'Error: {{e}}',file=sys.stderr)", - ); + let wrapper = format!( + "import sys,os\nos.chdir({cwd_repr})\ntry:\n{indented_code}\nexcept Exception as e:\n print(f'Error: {{e}}', file=sys.stderr)\n sys.exit(1)", + );#!/bin/bash # Verify wrapper exception handler behavior in source. rg -n "except Exception as e|sys\.exit\(1\)|print\(f'Error: \{e\}'.*stderr" crates/aish-tools/src/python.rs sed -n '72,86p' crates/aish-tools/src/python.rs🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/aish-tools/src/python.rs` around lines 78 - 79, The Python wrapper currently catches exceptions and only prints them (the multiline string containing "except Exception as e:\n print(f'Error: {e}',file=sys.stderr)"), which can make failures appear as successful runs; update that except block to exit non-zero (e.g., add "sys.exit(1)" after the print) so the wrapper returns a failure status when an exception occurs.
95-97:⚠️ Potential issue | 🔴 CriticalUTF-8 unsafe truncation may panic at runtime.
Line 96 slices by bytes (
stdout[..MAX_OUTPUT_CHARS]), which can panic when byte 1000 is inside a multibyte character.💡 Proposed fix
- let stdout = if stdout.len() > MAX_OUTPUT_CHARS { - let mut s = stdout[..MAX_OUTPUT_CHARS].to_string(); + let stdout_owned = stdout.into_owned(); + let stdout = if stdout_owned.chars().count() > MAX_OUTPUT_CHARS { + let cut = stdout_owned + .char_indices() + .nth(MAX_OUTPUT_CHARS) + .map(|(idx, _)| idx) + .unwrap_or(stdout_owned.len()); + let mut s = stdout_owned[..cut].to_string(); s.push_str("\n[Output truncated due to length]"); s } else { - stdout.into_owned() + stdout_owned };#!/bin/bash # Verify whether truncation uses direct byte slicing on UTF-8 text. rg -n "stdout\[\.\.MAX_OUTPUT_CHARS\]|char_indices\(\)\.nth\(MAX_OUTPUT_CHARS\)|MAX_OUTPUT_CHARS" crates/aish-tools/src/python.rs sed -n '90,104p' crates/aish-tools/src/python.rs🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/aish-tools/src/python.rs` around lines 95 - 97, The current truncation uses byte-slicing stdout[..MAX_OUTPUT_CHARS] which can panic on UTF-8 boundaries; replace it with a UTF-8-safe truncation that cuts at a char boundary (e.g., compute the byte index of the Nth char via stdout.char_indices().nth(MAX_OUTPUT_CHARS).map(|(i,_)| i).or_else(|| Some(stdout.len())) and slice using that index, or build a truncated string via stdout.chars().take(MAX_OUTPUT_CHARS).collect::<String>()), then append the "[Output truncated due to length]" message as before; update the code around the stdout variable and the MAX_OUTPUT_CHARS truncation site accordingly.
🤖 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-shell/src/readline.rs`:
- Around line 113-122: The hint suffix calculation uses line.trim() which strips
trailing spaces and causes duplicate leading-space hints when the user input
ends with a space; change the input normalization to use trim_start() (i.e.,
compute trimmed = line.trim_start()) before calling guard.suggest(trimmed) so
leading indentation is ignored but trailing spaces in the original line are
preserved for correct suffix alignment in the suggestion logic around
suggest(...), start, and s[start..].
---
Duplicate comments:
In `@crates/aish-tools/src/python.rs`:
- Around line 78-79: The Python wrapper currently catches exceptions and only
prints them (the multiline string containing "except Exception as e:\n
print(f'Error: {e}',file=sys.stderr)"), which can make failures appear as
successful runs; update that except block to exit non-zero (e.g., add
"sys.exit(1)" after the print) so the wrapper returns a failure status when an
exception occurs.
- Around line 95-97: The current truncation uses byte-slicing
stdout[..MAX_OUTPUT_CHARS] which can panic on UTF-8 boundaries; replace it with
a UTF-8-safe truncation that cuts at a char boundary (e.g., compute the byte
index of the Nth char via
stdout.char_indices().nth(MAX_OUTPUT_CHARS).map(|(i,_)| i).or_else(||
Some(stdout.len())) and slice using that index, or build a truncated string via
stdout.chars().take(MAX_OUTPUT_CHARS).collect::<String>()), then append the
"[Output truncated due to length]" message as before; update the code around the
stdout variable and the MAX_OUTPUT_CHARS truncation site 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: e51d56ab-2916-44c2-9472-428a9f00ef7e
📒 Files selected for processing (3)
crates/aish-pty/src/persistent.rscrates/aish-shell/src/readline.rscrates/aish-tools/src/python.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/aish-pty/src/persistent.rs
9f32079 to
f2f94db
Compare
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-tools/src/python.rs`:
- Around line 95-103: The truncation guard uses byte length (stdout.len() >
MAX_OUTPUT_CHARS) which misfires for multibyte UTF-8; change the check to test
for the existence of a character at MAX_OUTPUT_CHARS by using
stdout.char_indices().nth(MAX_OUTPUT_CHARS).is_some(), keeping the existing
truncation logic that computes end via
char_indices().nth(MAX_OUTPUT_CHARS).map(|(i,_)|i).unwrap_or(stdout.len()) and
appends the "[Output truncated due to length]" marker when appropriate.
- Around line 59-61: The code converts the current working directory to a lossy
UTF-8 String (cwd) and embeds it into the generated Python wrapper (used with
os.chdir), which corrupts non-UTF-8 paths; instead keep the current directory as
a PathBuf and pass it to the process spawn via Command::current_dir() where the
Python wrapper is launched. Replace the to_string_lossy()/unwrap_or_else logic
that produces cwd with a PathBuf obtained from std::env::current_dir() and
remove embedding that path into the Python wrapper; in the code that launches
the Python wrapper (the place that currently uses cwd / os.chdir in the
generated script), call Command::current_dir(path_buf) on the Command before
spawn to preserve non-UTF-8 paths.
🪄 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: c08a3ad2-991e-4e04-8326-6938e591f583
📒 Files selected for processing (2)
crates/aish-pty/src/persistent.rscrates/aish-tools/src/python.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/aish-pty/src/persistent.rs
f2f94db to
92ebd6f
Compare
When the interactive forwarding loop receives PromptReady and stdin keystrokes in the same select() iteration, the keystrokes are still forwarded to the PTY master and linger in the line-discipline canonical buffer. On the next command, these stale bytes get prepended to the actual command, corrupting it (e.g. "ip a" becomes "sip a"). Fix: prepend Ctrl-U (NAK, 0x15) before every command written to the PTY and clear write_buf at the PromptReady transition so stale stdin bytes never reach the PTY during the drain phase. Also includes readline CJK boundary fixes and python tool improvements (output truncation with safe UTF-8, sys.exit(1) in exception handler).
92ebd6f to
bd86688
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
crates/aish-shell/src/readline.rs (1)
113-118:⚠️ Potential issue | 🟡 MinorUse
trimmed.len()for the hint slice.
guard.suggest()runs againsttrimmed, so Line 116'sline.len()misaligns or suppresses hints when the input has leading spaces. For example," git"against"git status"slices from the wrong byte offset.Suggested fix
let trimmed = line.trim_start(); let guard = self.autosuggest.lock().unwrap(); guard.suggest(trimmed).and_then(|s| { - let start = line.len(); + let start = trimmed.len(); if start < s.len() && s.is_char_boundary(start) { Some(s[start..].to_string()) } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/aish-shell/src/readline.rs` around lines 113 - 118, The hint slicing uses line.len() but guard.suggest(trimmed) computes suggestions for trimmed, so replace start = line.len() with start = trimmed.len() (still check s.is_char_boundary(start)) inside the closure handling guard.suggest(...) so the byte offset aligns with the trimmed input when computing Some(s[start..].to_string()).crates/aish-pty/src/persistent.rs (1)
420-428:⚠️ Potential issue | 🟠 Major
write_buf.clear()is still too late in one interleaving.Line 320 flushes
write_bufbefore this block runs. If stdin bytes were already buffered from a previous poll andselect()returns bothmaster_fdwritable andPromptReadyreadable, those stale bytes are written to the PTY before Line 424 clears them. Reordering the loop to processcontrol_fdbefore the write step, or skipping writes oncePromptReadyis pending, would close that gap.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/aish-pty/src/persistent.rs` around lines 420 - 428, The bug is that stale stdin bytes in write_buf can be written to the PTY after PromptReady is received because write_buf.clear() runs too late; modify the event loop that handles select()/poll so control_fd (PromptReady) is processed before performing master_fd writes or, alternatively, check a pending PromptReady flag and skip/wipe any write operations when PromptReady is readable—i.e., on detecting PromptReady (control_fd) set draining = true and clear write_buf immediately (before any master_fd write path) so no stale bytes are sent; update the logic around select(), master_fd writable handling, and the write_buf clearing to enforce this ordering (references: write_buf, control_fd, master_fd, PromptReady, draining).
🧹 Nitpick comments (1)
crates/aish-pty/src/persistent.rs (1)
146-159: Consider a shared PTY command encoder.The Ctrl-U prefix/newline framing now exists here and again at Lines 230-233. A small helper would make the stale-input fix harder to drift between the exec and interactive paths.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/aish-pty/src/persistent.rs` around lines 146 - 159, Duplicate PTY framing logic (Ctrl-U prefix, optional "__AISH_ACTIVE_COMMAND_SEQ" and quoted text, command bytes, trailing newline) appears in this file and again in the interactive path; extract it into a single helper like encode_pty_command(command: &str, seq: Option<&str>) -> Vec<u8> (or a suitably named function) and replace both in-place payload construction (current use sites referencing payload, seq, shell_quote_escape) with calls to that helper so the stale-input Ctrl-U and newline framing are consistent across exec and interactive paths.
🤖 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-tools/src/python.rs`:
- Around line 82-104: The current logic treats stdout and stderr separately,
causing successful runs that only print to stderr to be reported as "no output"
and allowing stderr tracebacks to bypass the MAX_OUTPUT_CHARS truncation; modify
the code in crates/aish-tools/src/python.rs (around the stdout/stderr handling
near variables stdout_owned, stderr, exit_code and MAX_OUTPUT_CHARS) to first
construct a single visible string by concatenating stdout and stderr (preserving
a newline between them when both are non-empty), then apply the existing
truncation logic (using char_indices().nth(MAX_OUTPUT_CHARS) and appending
"[Output truncated due to length]") to that combined text, and finally use the
truncated combined text for the success/failure ToolResult::success or failure
branches so both streams share the same visibility/cap semantics.
---
Duplicate comments:
In `@crates/aish-pty/src/persistent.rs`:
- Around line 420-428: The bug is that stale stdin bytes in write_buf can be
written to the PTY after PromptReady is received because write_buf.clear() runs
too late; modify the event loop that handles select()/poll so control_fd
(PromptReady) is processed before performing master_fd writes or, alternatively,
check a pending PromptReady flag and skip/wipe any write operations when
PromptReady is readable—i.e., on detecting PromptReady (control_fd) set draining
= true and clear write_buf immediately (before any master_fd write path) so no
stale bytes are sent; update the logic around select(), master_fd writable
handling, and the write_buf clearing to enforce this ordering (references:
write_buf, control_fd, master_fd, PromptReady, draining).
In `@crates/aish-shell/src/readline.rs`:
- Around line 113-118: The hint slicing uses line.len() but
guard.suggest(trimmed) computes suggestions for trimmed, so replace start =
line.len() with start = trimmed.len() (still check s.is_char_boundary(start))
inside the closure handling guard.suggest(...) so the byte offset aligns with
the trimmed input when computing Some(s[start..].to_string()).
---
Nitpick comments:
In `@crates/aish-pty/src/persistent.rs`:
- Around line 146-159: Duplicate PTY framing logic (Ctrl-U prefix, optional
"__AISH_ACTIVE_COMMAND_SEQ" and quoted text, command bytes, trailing newline)
appears in this file and again in the interactive path; extract it into a single
helper like encode_pty_command(command: &str, seq: Option<&str>) -> Vec<u8> (or
a suitably named function) and replace both in-place payload construction
(current use sites referencing payload, seq, shell_quote_escape) with calls to
that helper so the stale-input Ctrl-U and newline framing are consistent across
exec and interactive paths.
🪄 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: dcd8b032-6335-4cc1-b5a2-e66bc1f897cf
📒 Files selected for processing (3)
crates/aish-pty/src/persistent.rscrates/aish-shell/src/readline.rscrates/aish-tools/src/python.rs
Summary
Root Cause
In
send_command_interactive's forwarding loop, whenselect()returns with both stdin data and PromptReady in the same iteration:write_buf(processed first)draining = truewrite_bufis still written to master_fdThis causes user keystrokes to be forwarded to the PTY's line-discipline buffer even after the command has finished. On the next
send_command_interactivecall, these stale bytes get prepended to the new command, corrupting it (e.g.,ip a→sip a).Test plan
cargo test— 502 passed, 0 failedcargo clippy— 0 warningscargo build --release— successip a— should execute correctlyvim/ssh— interactive commands still workSummary by CodeRabbit
Bug Fixes
Improvements