feat(ssh): channel-based AI tools, NL detection, and remote bash offload#185
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 (2)
📝 WalkthroughWalkthroughAdds NL detection (pty + shell), changes LLM to return ProcessResult with iteration-limit callback, implements BashExecResult and offload lifecycle (capture, finalize, cancel), handles bracketed paste, updates channel/tools integration, and adds locale strings. ChangesNatural Language Detection and Output Offloading
Sequence Diagram(s)sequenceDiagram
participant User
participant Shell as AishShell
participant NlDet as nl_detect
participant Handler as ai_handler
participant LLM as LlmSession
User->>Shell: enters command
Shell->>NlDet: detect(input)
NlDet->>NlDet: check PATH, score keywords/CJK
NlDet-->>Shell: NlVerdict
alt is_natural_language
Shell->>User: confirm_ask_ai prompt
User->>Shell: approve
Shell->>Handler: handle_question
Handler->>LLM: process_input()
LLM->>LLM: tool loop + iteration checks
alt iteration_limit reached
LLM->>User: iteration_limit prompt
User->>LLM: continue?
end
LLM-->>Handler: ProcessResult{text, new_messages}
Handler->>Shell: response (text)
Shell->>User: render output
else not natural language
Shell->>Shell: execute as external command
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
🚥 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 |
github-actions
Bot
added
ci-cd
docs
packaging
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/aish-shell/src/app.rs (1)
2050-2059:⚠️ Potential issue | 🟠 Major | ⚡ Quick winInclude
remote_offload_pathin the follow-up prompt.When a remote bash command produces long output, it is written to a file on the remote host and
remote_offload_pathcontains that location. The followup closure at line 2050 currently ignores this parameter, so the LLM prompt only receives a preview of the output without any hint about where the full file is stored. This breaks the multi-round SSH flow: the LLM cannot suggest bash commands to inspect the offloaded file because it has no way to know where it is.Suggested fix
- Box::new(move |output: &str, _remote_offload_path: Option<&str>| -> Option<aish_pty::AiResponse> { + Box::new(move |output: &str, remote_offload_path: Option<&str>| -> Option<aish_pty::AiResponse> { + let offload_hint = remote_offload_path + .map(|path| { + format!( + "\nFull output was offloaded on the remote host to `{path}`. \ +Use the bash tool to inspect that file if the preview below is insufficient.\n" + ) + }) + .unwrap_or_default(); let followup_prompt = format!( - "I ran the command on the remote host. Here is the output:\n\ + "I ran the command on the remote host. Here is the output:{offload_hint}\n\ ```\n{}\n```\n\n\ Original question: {}\n\ Please analyze the command output. If further action is needed, \🤖 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 2050 - 2059, The follow-up closure currently ignores the second parameter by naming it _remote_offload_path and never mentioning it in the followup_prompt; update the closure signature to use remote_offload_path (remove the leading underscore) and append a sentence to followup_prompt that, when remote_offload_path is Some(path), informs the LLM that the full command output was offloaded to that remote path (e.g. "The full output is stored at: <path> on the remote host; you can inspect it with a bash command like `cat <path>`"). Ensure the logic gracefully handles None vs Some and still preserves the existing prompt text assigned to followup_prompt.
🧹 Nitpick comments (4)
crates/aish-shell/src/nl_detect.rs (2)
26-29: ⚡ Quick winDRY: CJK detection logic is duplicated.
The CJK Unicode range check (
'\u{4E00}'..='\u{9FFF}'etc.) is duplicated fromaish_pty::nl_detect::is_cjk. Consider exposing theis_cjkhelper or the CJK detection logic fromaish-ptyto avoid maintaining identical code in two places.♻️ Refactor to reuse CJK detection
Option 1: Make
is_cjkpublic inaish-pty/nl_detect.rs:-fn is_cjk(c: char) -> bool { +pub fn is_cjk(c: char) -> bool {Then use it here:
- let is_cjk = input - .chars() - .filter(|c| !c.is_whitespace()) - .any(|c| matches!(c, '\u{4E00}'..='\u{9FFF}' | '\u{3400}'..='\u{4DBF}' | '\u{F900}'..='\u{FAFF}')); + let is_cjk = input + .chars() + .filter(|c| !c.is_whitespace()) + .any(aish_pty::nl_detect::is_cjk);Option 2: Expose a
detect_languagehelper fromaish-ptythat returnsNlLanguage.🤖 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/nl_detect.rs` around lines 26 - 29, The CJK range check is duplicated here; replace the local logic with the shared helper by importing and calling the existing is_cjk from aish_pty::nl_detect (or use a new detect_language that returns NlLanguage) instead of reimplementing the Unicode ranges: make aish_pty::nl_detect::is_cjk public (or add detect_language) if necessary, then change this snippet to call is_cjk(input) (or the appropriate detect_language API) and remove the inline chars()/matches! check so both crates share the same detection logic.
59-62: ⚡ Quick winTest depends on system PATH configuration.
The test for "list all files" only asserts NL detection when
which::which("list")fails. This makes the test environment-dependent — it will behave differently on systems where alistexecutable exists in PATH.Consider using a nonsense command that's guaranteed not to exist, or mock the
whichcall.♻️ Use guaranteed-missing command
#[test] - fn test_detect_english_sentence() { - let v = detect("list all files"); - if which::which("list").is_err() { - assert!(v.is_natural_language); - assert_eq!(v.language, NlLanguage::English); - } + fn test_detect_english_sentence() { + // Use a nonsense command unlikely to exist in PATH + let v = detect("xyzabc all files"); + assert!(v.is_natural_language); + assert_eq!(v.language, NlLanguage::English); }🤖 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/nl_detect.rs` around lines 59 - 62, The test currently relies on which::which("list") to determine environment behavior; replace that fragile check by using a guaranteed-nonexistent command name (e.g., a clearly nonsense string) or mock which::which in the test so it deterministically returns Err, then assert that v.is_natural_language and v.language == NlLanguage::English; update the test that references which::which("list") and the variable v to use the mocked/guaranteed-missing path to avoid PATH-dependent flakiness.crates/aish-pty/src/nl_detect.rs (1)
153-158: ⚖️ Poor tradeoffVerify suffix stripping behavior.
The
simplify_wordfunction strips-ing,-ed, and-lysuffixes, which may not always produce valid word stems. For example:
- "running" → "runn" (not "run")
- "studied" → "studi" (not "study")
This is acceptable for fuzzy NL scoring but could cause false negatives if keywords like "run" are in
NL_KEYWORDSbut "running" doesn't match because "runn" isn't in the set.Consider whether the keyword list should include common inflected forms, or use a proper stemming algorithm.
🤖 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/nl_detect.rs` around lines 153 - 158, The simplify_word helper currently strips literal suffixes in simplify_word which produces invalid stems (e.g., "running" -> "runn") and can cause mismatches against NL_KEYWORDS; update the code so either (a) expand NL_KEYWORDS to include common inflected forms (e.g., run/running, study/studied/studies) or (b) replace simplify_word with a proper stemming/lemmatization step (use a small Rust stemmer crate or implement simple rules to handle double-consonant and -y→-i cases) and ensure any change is applied where simplify_word is used to compute fuzzy NL scoring so keywords and tokens reliably match.packaging/scripts/setup-ci-env.sh (1)
28-28: 💤 Low valueHardcoded architecture limits CI flexibility.
The script now hardcodes
x86_64-unknown-linux-muslinstead of dynamically determining the target. While this aligns with the simplified amd64-only release workflows mentioned in the PR summary, it removes flexibility for testing other architectures (aarch64, etc.) in self-hosted runner or container environments.If multi-arch support will never be needed here, this is fine. Otherwise, consider keeping the dynamic target selection or adding a comment explaining the amd64-only constraint.
🤖 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 `@packaging/scripts/setup-ci-env.sh` at line 28, The script currently hardcodes the Rust target via the command `rustup target add x86_64-unknown-linux-musl`, which removes multi-arch flexibility; either restore dynamic target selection (e.g., derive TARGET from an environment variable like TARGET or detect CPU_ARCH and call `rustup target add "$TARGET"`/map arch to target) or, if amd64-only is intentional, add a clear comment above the `rustup target add x86_64-unknown-linux-musl` line explaining the intentional amd64-only constraint and why other architectures are excluded; update the `rustup target add` invocation to use the chosen variable name (e.g., TARGET) so the script can accept an override in CI.
🤖 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 @.github/workflows/release.yml:
- Around line 127-129: The release workflow's smoke test expects
/usr/local/bin/aish-sandbox but the bundle installer
(packaging/scripts/install-bundle.sh) only installs aish and aish-uninstall;
update the installer and bundle payload so aish-sandbox is installed: modify
packaging/scripts/install-bundle.sh to also copy or install the aish-sandbox
binary (alongside the existing aish and aish-uninstall steps) and ensure the
bundle payload includes the aish-sandbox file so the test in
.github/workflows/release.yml passes.
In `@crates/aish-cli/src/update.rs`:
- Around line 136-147: When include_pre_release is true, don't use
releases.into_iter().next(); instead filter the deserialized releases (the
releases Vec<serde_json::Value> variable) to exclude draft=true and include only
prerelease=true, then parse each remaining release's tag_name (or semver field)
and pick the highest version using the existing compare_versions function;
update the match that currently returns the first element to instead sort/reduce
by compare_versions (or use an explicit max_by using compare_versions) and
return that selected release, returning Ok(None) if no qualifying prereleases
remain.
In `@crates/aish-i18n/locales/de-DE.yaml`:
- Line 191: Replace the inconsistent English acronym in the translation key
confirm_ask_ai so it uses the locale-consistent German term "KI" instead of
"AI"; update the string value for confirm_ask_ai from "Das sieht nach einer
natürlichen Sprachfrage aus. AI fragen? (Y/n)" to use "KI fragen? (Y/n)" so
terminology matches the rest of the de-DE locale.
In `@crates/aish-i18n/locales/es-ES.yaml`:
- Line 191: Update the translation value for the key confirm_ask_ai to use the
Spanish acronym "IA" instead of "AI" for consistency; locate the confirm_ask_ai
entry in the es-ES YAML and change "Esto parece una pregunta en lenguaje
natural. ¿Preguntar a AI? (Y/n)" to use "IA" so it reads "Esto parece una
pregunta en lenguaje natural. ¿Preguntar a IA? (Y/n)".
In `@crates/aish-llm/src/session.rs`:
- Around line 405-409: The return builds ProcessResult::new_messages from
messages[initial_len..] before the assistant reply is appended, so callers lose
the model's response; before creating new_messages in the early-return paths
(the block that returns crate::types::ProcessResult with text:
content.unwrap_or_default()), push an assistant Message containing
content.unwrap_or_default() into messages (matching your Message type/shape) and
then set new_messages = messages[initial_len..].to_vec(); apply the same change
to the other early-return branches (e.g., the similar block around lines
701-705) so the assistant reply is included in ProcessResult::new_messages.
In `@crates/aish-pty/src/persistent.rs`:
- Around line 1915-1924: The offload/base64 payload is still being appended to
output_buf earlier in the read path even though terminal display is suppressed;
update the code that pushes chunks into output_buf (the read/append logic that
feeds send_command_interactive's returned output) to skip appending when
remote_offload_pending.is_some() (and similarly respect
probe_active/was_probe_active and interceptor.is_ai_processing() as used for
display) so remote offload writes do not add the payload to output_buf or the
returned transcript; locate the append site (where output_buf gets the chunk)
and add the same guard used for display (check interceptor.is_ai_processing(),
probe_active/was_probe_active, and remote_offload_pending) to prevent storing
offload payloads.
- Around line 371-379: The write_all_fd helper stops on n <= 0 which drops data
when master_fd is non-blocking; update write_all_fd to treat negative returns
properly by checking errno: on EINTR simply continue, on EAGAIN/EWOULDBLOCK poll
(or select) the fd for writability (POLLOUT) and then retry, and on positive n
advance the buffer as now; keep the existing partial-write loop and only break
on unrecoverable errors. Ensure you reference the write_all_fd function and
account for the fact master_fd is set non-blocking in start().
- Around line 1525-1550: The confirmed-NL branch currently calls
interceptor.call_ai(question) directly which bypasses the TriggerAi path's
host-probe logic; change this so NL-confirmed prompts go through the same
probe-check path as TriggerAi: use the same probe check (call
interceptor.needs_probe(...) with remote_host_for_probe.as_deref() or the helper
used by TriggerAi), if a probe is needed set/retain remote_host_for_probe and
defer (return None) so the profile refresh can happen, otherwise call
interceptor.call_ai(question); update the code at the spot that currently
references interceptor.call_ai(question) and reuse the same probe/TriggerAi flow
(handle_dossier_command, remote_host_for_probe, needs_probe, TriggerAi
semantics).
- Around line 391-432: The remote commands currently create remote_dir and
remote_path without enforcing restrictive permissions; prefix the compound shell
command with a restrictive umask (e.g. "umask 077 &&") or use mkdir with mode
and follow with chmod to ensure private perms so files aren't world-readable.
Modify how cmd is built around remote_dir/remote_path in the block that
constructs the mkdir and printf/base64 fragments (the code using uuid,
remote_dir, remote_path, CHUNK_SIZE, and write_all_fd) so the compound command
begins with "umask 077 && mkdir -p '...'" (and if you prefer, use "mkdir -p -m
700" plus "chmod 600 '...'" after writes) and ensure every branch (single-chunk
and multi-chunk append paths) preserves that umask/mode so the created file is
private.
In `@crates/aish-pty/src/session_interceptor.rs`:
- Line 209: The PossibleNl path returns PossibleNl(line) without clearing the
temporary buffer, so if the user cancels the NL confirmation the old line
remains in self.line_shadow and may be re-detected; modify the branch that
returns PossibleNl to call self.line_shadow.clear() (same as the TriggerAi path
does) before returning, ensuring the shadow buffer is cleared when switching
back to Passthrough; locate the check that yields PossibleNl(line) and add a
self.line_shadow.clear() call there, keeping the TriggerAi behavior consistent.
In `@crates/aish-shell/src/app.rs`:
- Around line 2308-2318: The code unconditionally pushes
ChatMessage::assistant(&pr.text) into the conversation history, causing a
duplicate assistant turn when process_result.pr.new_messages already contains
the assistant message (tool-call case); change the logic in the block that
mutably borrows conversation_history_th so that after
h.push(ChatMessage::user(&followup_prompt)) you extend h with
pr.new_messages.clone() and only push ChatMessage::assistant(&pr.text) when
pr.new_messages.is_empty() (i.e., if let Some(ref pr) = process_result {
h.extend(pr.new_messages.clone()); if pr.new_messages.is_empty() {
h.push(ChatMessage::assistant(&pr.text)); } }); apply the same conditional
change to the similar block around the second occurrence (the one noted at the
other location).
---
Outside diff comments:
In `@crates/aish-shell/src/app.rs`:
- Around line 2050-2059: The follow-up closure currently ignores the second
parameter by naming it _remote_offload_path and never mentioning it in the
followup_prompt; update the closure signature to use remote_offload_path (remove
the leading underscore) and append a sentence to followup_prompt that, when
remote_offload_path is Some(path), informs the LLM that the full command output
was offloaded to that remote path (e.g. "The full output is stored at: <path> on
the remote host; you can inspect it with a bash command like `cat <path>`").
Ensure the logic gracefully handles None vs Some and still preserves the
existing prompt text assigned to followup_prompt.
---
Nitpick comments:
In `@crates/aish-pty/src/nl_detect.rs`:
- Around line 153-158: The simplify_word helper currently strips literal
suffixes in simplify_word which produces invalid stems (e.g., "running" ->
"runn") and can cause mismatches against NL_KEYWORDS; update the code so either
(a) expand NL_KEYWORDS to include common inflected forms (e.g., run/running,
study/studied/studies) or (b) replace simplify_word with a proper
stemming/lemmatization step (use a small Rust stemmer crate or implement simple
rules to handle double-consonant and -y→-i cases) and ensure any change is
applied where simplify_word is used to compute fuzzy NL scoring so keywords and
tokens reliably match.
In `@crates/aish-shell/src/nl_detect.rs`:
- Around line 26-29: The CJK range check is duplicated here; replace the local
logic with the shared helper by importing and calling the existing is_cjk from
aish_pty::nl_detect (or use a new detect_language that returns NlLanguage)
instead of reimplementing the Unicode ranges: make aish_pty::nl_detect::is_cjk
public (or add detect_language) if necessary, then change this snippet to call
is_cjk(input) (or the appropriate detect_language API) and remove the inline
chars()/matches! check so both crates share the same detection logic.
- Around line 59-62: The test currently relies on which::which("list") to
determine environment behavior; replace that fragile check by using a
guaranteed-nonexistent command name (e.g., a clearly nonsense string) or mock
which::which in the test so it deterministically returns Err, then assert that
v.is_natural_language and v.language == NlLanguage::English; update the test
that references which::which("list") and the variable v to use the
mocked/guaranteed-missing path to avoid PATH-dependent flakiness.
In `@packaging/scripts/setup-ci-env.sh`:
- Line 28: The script currently hardcodes the Rust target via the command
`rustup target add x86_64-unknown-linux-musl`, which removes multi-arch
flexibility; either restore dynamic target selection (e.g., derive TARGET from
an environment variable like TARGET or detect CPU_ARCH and call `rustup target
add "$TARGET"`/map arch to target) or, if amd64-only is intentional, add a clear
comment above the `rustup target add x86_64-unknown-linux-musl` line explaining
the intentional amd64-only constraint and why other architectures are excluded;
update the `rustup target add` invocation to use the chosen variable name (e.g.,
TARGET) so the script can accept an override in CI.
🪄 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: 9b9b2afd-6c8c-4711-82f5-82e33cd4af4c
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (30)
.github/workflows/release-preparation.yml.github/workflows/release.ymlCHANGELOG.mdCargo.tomlMakefilecrates/aish-cli/src/uninstall.rscrates/aish-cli/src/update.rscrates/aish-i18n/locales/de-DE.yamlcrates/aish-i18n/locales/en-US.yamlcrates/aish-i18n/locales/es-ES.yamlcrates/aish-i18n/locales/fr-FR.yamlcrates/aish-i18n/locales/ja-JP.yamlcrates/aish-i18n/locales/zh-CN.yamlcrates/aish-llm/src/session.rscrates/aish-llm/src/types.rscrates/aish-pty/Cargo.tomlcrates/aish-pty/src/lib.rscrates/aish-pty/src/nl_detect.rscrates/aish-pty/src/persistent.rscrates/aish-pty/src/session_interceptor.rscrates/aish-shell/src/ai_handler.rscrates/aish-shell/src/app.rscrates/aish-shell/src/lib.rscrates/aish-shell/src/nl_detect.rscrates/aish-tools/src/channel_bash.rsdebian/rulespackaging/scripts/install-bundle.shpackaging/scripts/publish_release.shpackaging/scripts/setup-ci-env.shpackaging/scripts/uninstall-bundle.sh
💤 Files with no reviewable changes (1)
- packaging/scripts/publish_release.sh
- Add channel-based AI tool system for SSH sessions with multi-round tool chaining (bash_exec, ask_user, host_note) - Add local offload for large bash_exec output, writing to /tmp/aish-offload/ instead of buffering in memory - Add NL detection for local aish shell; remove NL detection from SSH sessions to avoid false triggers in vim/less/top - Support bracketed paste mode in SessionInterceptor - Support followup offload with proper temp file cleanup on cancel
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/aish-pty/src/persistent.rs (2)
3097-3112:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRejecting with
nstill looks like ordinary tool output on this path.The plain rejection branch sends
BashExecResult { output: "(cancelled: ...)" }back to the LLM thread and then keeps the ask-user loop alive. That makes the rejection indistinguishable from a normal bash response, so the model can immediately emit anotherAiEvent::BashExec, which undercuts the “reject once and stop further tool calls” behavior this PR is trying to guarantee.🤖 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 3097 - 3112, When the user rejects with 'n', don't send a normal-looking BashExecResult and keep the ask-user loop running; instead send a distinct cancellation signal and terminate the ask-user loop so the LLM thread cannot immediately re-emit BashExec. Concretely, change the output_sender.send call that currently sends BashExecResult { output: format!("(cancelled: {})", command), ... } to emit an unmistakable cancellation variant (e.g., a dedicated enum variant or a BashExecResult field such as canceled: true / offload_path = Some("canceled")) or close/drop the channel, and then break/return out of the ask-user loop so no further AiEvent::BashExec is produced; update any consumers of crate::session_interceptor::BashExecResult to handle the new cancellation signal.
2928-3077:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift
offload_pathhere is still a client-local temp file, not a remote SSH path.This branch builds
PtyOutputOffloadunderstd::env::temp_dir()and forwards the finalized path inBashExecResult.offload_path. Downstream,ChannelBashToolinterprets any populatedoffload_pathas a remotestdout_path, so long SSH outputs point the model at a file that only exists on the aish machine, not under remote/tmp/aish-offload/.... Use the remote PTY offload writer on this path, or leaveoffload_pathunset until you have a genuine remote-readable path.🤖 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 2928 - 3077, The code creates a local PtyOutputOffload (PtyOutputOffload::new using std::env::temp_dir()) and then returns its finalized path in BashExecResult.offload_path, but that path is client-local and not accessible to remote ChannelBashTool; either don't populate offload_path when using the local offloader or instantiate/use the remote PTY offloader API so the path points to a remote-readable location. Concretely: if you cannot create a remote offloader, change the offload_path assignment after offloader.finalize(...) to always produce None (i.e. never send offload_result.stdout.path in BashExecResult.offload_path), otherwise replace the local offloader creation (PtyOutputOffload::new) with the remote offloader factory/method provided by the SSH/channel layer so offloader.finalize() returns a remote path that ChannelBashTool can consume. Ensure you update the code paths around offloader, offloader.finalize, and BashExecResult.offload_path accordingly.
🤖 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-pty/src/persistent.rs`:
- Around line 972-982: The display-only branch writes response.display_text with
"\r\n" to STDOUT, but the later pending_response handler (the other
`pending_response` branch that handles `TriggerAi`-style responses) still only
writes "\r", so explanation-only AI answers are missed on the main path; update
the second `pending_response` handler to mirror the first: when
`response.display_text` is non-empty, clone and append "\r\n" (or reuse the same
formatting logic), and write it to STDOUT using the same unsafe libc::write
sequence (preserving msg.as_ptr() as *const libc::c_void and msg.len()) so both
handlers render display_text consistently for TriggerAi responses.
In `@crates/aish-tools/src/channel_bash.rs`:
- Around line 61-63: The runtime now defaults timeout_secs to 1800 in execute(),
but the tool/schema still advertises a 120s default; update the tool schema and
any advertised/declared default values from 120 to 1800 so they match the
runtime behavior (search for the tool schema or option definition that states
"default: 120" and change it to 1800), and ensure any help text or docs
referencing the timeout default are updated accordingly to keep the contract
consistent with the timeout_secs fallback.
---
Outside diff comments:
In `@crates/aish-pty/src/persistent.rs`:
- Around line 3097-3112: When the user rejects with 'n', don't send a
normal-looking BashExecResult and keep the ask-user loop running; instead send a
distinct cancellation signal and terminate the ask-user loop so the LLM thread
cannot immediately re-emit BashExec. Concretely, change the output_sender.send
call that currently sends BashExecResult { output: format!("(cancelled: {})",
command), ... } to emit an unmistakable cancellation variant (e.g., a dedicated
enum variant or a BashExecResult field such as canceled: true / offload_path =
Some("canceled")) or close/drop the channel, and then break/return out of the
ask-user loop so no further AiEvent::BashExec is produced; update any consumers
of crate::session_interceptor::BashExecResult to handle the new cancellation
signal.
- Around line 2928-3077: The code creates a local PtyOutputOffload
(PtyOutputOffload::new using std::env::temp_dir()) and then returns its
finalized path in BashExecResult.offload_path, but that path is client-local and
not accessible to remote ChannelBashTool; either don't populate offload_path
when using the local offloader or instantiate/use the remote PTY offloader API
so the path points to a remote-readable location. Concretely: if you cannot
create a remote offloader, change the offload_path assignment after
offloader.finalize(...) to always produce None (i.e. never send
offload_result.stdout.path in BashExecResult.offload_path), otherwise replace
the local offloader creation (PtyOutputOffload::new) with the remote offloader
factory/method provided by the SSH/channel layer so offloader.finalize() returns
a remote path that ChannelBashTool can consume. Ensure you update the code paths
around offloader, offloader.finalize, and BashExecResult.offload_path
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: d0625523-cd52-4fb7-bdf1-41e57d6dab3e
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (19)
crates/aish-i18n/locales/de-DE.yamlcrates/aish-i18n/locales/en-US.yamlcrates/aish-i18n/locales/es-ES.yamlcrates/aish-i18n/locales/fr-FR.yamlcrates/aish-i18n/locales/ja-JP.yamlcrates/aish-i18n/locales/zh-CN.yamlcrates/aish-llm/src/session.rscrates/aish-llm/src/types.rscrates/aish-pty/Cargo.tomlcrates/aish-pty/src/lib.rscrates/aish-pty/src/nl_detect.rscrates/aish-pty/src/offload.rscrates/aish-pty/src/persistent.rscrates/aish-pty/src/session_interceptor.rscrates/aish-shell/src/ai_handler.rscrates/aish-shell/src/app.rscrates/aish-shell/src/lib.rscrates/aish-shell/src/nl_detect.rscrates/aish-tools/src/channel_bash.rs
✅ Files skipped from review due to trivial changes (9)
- crates/aish-pty/Cargo.toml
- crates/aish-i18n/locales/ja-JP.yaml
- crates/aish-pty/src/offload.rs
- crates/aish-i18n/locales/de-DE.yaml
- crates/aish-shell/src/lib.rs
- crates/aish-i18n/locales/en-US.yaml
- crates/aish-i18n/locales/fr-FR.yaml
- crates/aish-shell/src/ai_handler.rs
- crates/aish-shell/src/nl_detect.rs
🚧 Files skipped from review as they are similar to previous changes (7)
- crates/aish-llm/src/types.rs
- crates/aish-i18n/locales/es-ES.yaml
- crates/aish-i18n/locales/zh-CN.yaml
- crates/aish-pty/src/lib.rs
- crates/aish-pty/src/nl_detect.rs
- crates/aish-llm/src/session.rs
- crates/aish-shell/src/app.rs
- Fix PtyOutputOffload::finalize() losing internal buffer data (stdout_buf was never included in clean file output) - Ensure clean files are valid UTF-8 via from_utf8_lossy for PTY output - Prefer .clean path over .raw for offload (double defense in both persistent.rs and channel_bash.rs) - Propagate clean_error instead of silently discarding it - Register ReadFileTool in SSH sessions for reading local offload files - Register SkillTool in SSH sessions with skills snapshot and description - Add read_file tool execution display in SSH terminal (ToolExecution events) - Update SSH system prompt with read_file, skill usage, and offload rules - Cancel LLM session token on user rejection to prevent "Channel closed" retry loop in fire-and-forget followup threads
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/aish-shell/src/app.rs (1)
2050-2058:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't drop the offload path in chained prompts.
offload_pathis now available here, but the nextfollowup_promptstill only includesoutput. When a remote command is offloaded, the next LLM turn never learns where the full output lives and has to reason from the truncated preview.Possible fix
- Box::new(move |output: &str, _offload_path: Option<&str>| -> Option<aish_pty::AiResponse> { + Box::new(move |output: &str, offload_path: Option<&str>| -> Option<aish_pty::AiResponse> { + let offload_note = offload_path + .map(|path| { + format!( + "\nFull output was offloaded to `{}`. Read that file before drawing conclusions if the preview looks incomplete.\n", + path + ) + }) + .unwrap_or_default(); let followup_prompt = format!( "I ran the command on the remote host. Here is the output:\n\ ```\n{}\n```\n\n\ + {}\ Original question: {}\n\ Please analyze the command output. If further action is needed, \ suggest the next bash command in a ```bash code block. \ If no further action is needed, just provide a summary.", - output, question_f + output, offload_note, question_f );🤖 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 2050 - 2058, The followup_prompt built inside the closure for Box::new(move |output: &str, _offload_path: Option<&str>| -> Option<aish_pty::AiResponse> is dropping the offload_path, so downstream LLM turns don’t know where full output was offloaded; update the closure to accept and inspect offload_path (rename _offload_path to offload_path), construct an offload_note when Some(path) (e.g., "Full output available at: <path>\n\n"), and include that offload_note in the followup_prompt format arguments (add offload_note into the format string and pass it alongside output and question_f) so the LLM is informed of the offload location.
♻️ Duplicate comments (1)
crates/aish-shell/src/app.rs (1)
1323-1375:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReuse the normal AI post-processing on the NL-routing path.
This branch still returns right after
handle_question, so it skips the plan-exit/approval flow that theInputIntent::Aipath runs. Ifexit_plan_modefires from an NL-routed prompt, the plan can be left without review.🤖 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 1323 - 1375, The NL-routing branch calls self.ai_handler.handle_question(...) and then returns early, skipping the same post-processing/plan-exit/approval flow that the InputIntent::Ai path performs (which can leave plans unreviewed when exit_plan_mode triggers). Change the NL branch to reuse the same AI post-processing path used by InputIntent::Ai: after getting Ok(response) or Err from handle_question, invoke the same post-processing routine (the code that handles plan approval/exit, recording history, and streaming state) instead of short-circuiting—i.e., delegate to or call the same function/module that InputIntent::Ai uses (preserve install_ai_sigint_handler/restore_ai_sigint_handler and the cancellation handling around self.ai_handler.handle_question) so NL-routed prompts run the identical plan/exit/approval logic.
🤖 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-pty/src/offload.rs`:
- Around line 480-494: The code currently sets clean_path to Some(path) even
when fs::write(&p, ...) fails; change the logic so clean_path is Some only when
the write succeeded and clean_error is None — i.e., after calling fs::write(&p,
cleaned_str.as_bytes()) set err = None and set clean_path to p.to_str().map(|s|
s.to_string()) only on Ok(()), and set clean_path to None when Err(e) (store
e.to_string() in clean_error). Update the tuple produced in the block
(clean_path, clean_error) accordingly so callers in persistent.rs won't prefer a
non-existent clean file.
In `@crates/aish-pty/src/persistent.rs`:
- Around line 3014-3025: The prompt detection currently inspects only the latest
read chunk (data) which misses prompts split across reads; modify the check that
sets prompt_seen to examine the accumulated capture buffer concatenated with the
current data (i.e., previous_capture + data) instead of just data so trailing
bytes from the prior read are considered; update the branch that tests
tail.ends_with(b"# ") / b"$ " / b"#\r" / b"$\r" to compute the tail from the
combined buffer (while still using prompt_seen and data as shown) so split SSH
packets correctly trigger prompt_seen.
In `@crates/aish-shell/src/app.rs`:
- Around line 2993-3005: The SSH-toolset (ReadFileTool and SkillTool) is only
registered on the initial session so follow-up LlmSession instances created by
build_followup_closure lose those tools (keeping only bash, ask_user, host_note)
and break offload/skill flows; fix by ensuring every LlmSession created in
build_followup_closure registers the same tools — either extract the
registration logic (session.register_tool with aish_tools::fs::ReadFileTool::new
and aish_tools::SkillTool::new using the same skills_snapshot_th/skill_names_th
closures) into a helper and call it for each new session or copy the same
registration steps into the closure where LlmSession is constructed so all
follow-up sessions have read_file and skill tools available.
- Around line 2993-2994: The SSH LlmSession is registering ReadFileTool directly
(session.register_tool(Box::new(aish_tools::fs::ReadFileTool::new()))) which
exposes local file reads without the normal user confirmation/preflight; update
LlmSession initialization to either omit registering ReadFileTool for SSH
sessions or construct ReadFileTool with the same confirmation/preflight callback
used elsewhere (e.g., pass the session’s preflight/confirmation handler or wrap
ReadFileTool so register_tool receives a tool that enforces prompt approval).
Locate the LlmSession creation and the call to register_tool and ensure the
ReadFileTool is only added when the confirmation callback is wired (or replace
with a safe wrapper that calls the confirmation callback before performing
reads).
---
Outside diff comments:
In `@crates/aish-shell/src/app.rs`:
- Around line 2050-2058: The followup_prompt built inside the closure for
Box::new(move |output: &str, _offload_path: Option<&str>| ->
Option<aish_pty::AiResponse> is dropping the offload_path, so downstream LLM
turns don’t know where full output was offloaded; update the closure to accept
and inspect offload_path (rename _offload_path to offload_path), construct an
offload_note when Some(path) (e.g., "Full output available at: <path>\n\n"), and
include that offload_note in the followup_prompt format arguments (add
offload_note into the format string and pass it alongside output and question_f)
so the LLM is informed of the offload location.
---
Duplicate comments:
In `@crates/aish-shell/src/app.rs`:
- Around line 1323-1375: The NL-routing branch calls
self.ai_handler.handle_question(...) and then returns early, skipping the same
post-processing/plan-exit/approval flow that the InputIntent::Ai path performs
(which can leave plans unreviewed when exit_plan_mode triggers). Change the NL
branch to reuse the same AI post-processing path used by InputIntent::Ai: after
getting Ok(response) or Err from handle_question, invoke the same
post-processing routine (the code that handles plan approval/exit, recording
history, and streaming state) instead of short-circuiting—i.e., delegate to or
call the same function/module that InputIntent::Ai uses (preserve
install_ai_sigint_handler/restore_ai_sigint_handler and the cancellation
handling around self.ai_handler.handle_question) so NL-routed prompts run the
identical plan/exit/approval logic.
🪄 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: 80cda614-d734-4588-999b-c4dd27d273cb
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (19)
crates/aish-i18n/locales/de-DE.yamlcrates/aish-i18n/locales/en-US.yamlcrates/aish-i18n/locales/es-ES.yamlcrates/aish-i18n/locales/fr-FR.yamlcrates/aish-i18n/locales/ja-JP.yamlcrates/aish-i18n/locales/zh-CN.yamlcrates/aish-llm/src/session.rscrates/aish-llm/src/types.rscrates/aish-pty/Cargo.tomlcrates/aish-pty/src/lib.rscrates/aish-pty/src/nl_detect.rscrates/aish-pty/src/offload.rscrates/aish-pty/src/persistent.rscrates/aish-pty/src/session_interceptor.rscrates/aish-shell/src/ai_handler.rscrates/aish-shell/src/app.rscrates/aish-shell/src/lib.rscrates/aish-shell/src/nl_detect.rscrates/aish-tools/src/channel_bash.rs
✅ Files skipped from review due to trivial changes (5)
- crates/aish-pty/Cargo.toml
- crates/aish-i18n/locales/de-DE.yaml
- crates/aish-i18n/locales/en-US.yaml
- crates/aish-i18n/locales/es-ES.yaml
- crates/aish-shell/src/ai_handler.rs
🚧 Files skipped from review as they are similar to previous changes (9)
- crates/aish-shell/src/lib.rs
- crates/aish-pty/src/lib.rs
- crates/aish-i18n/locales/zh-CN.yaml
- crates/aish-llm/src/types.rs
- crates/aish-i18n/locales/fr-FR.yaml
- crates/aish-pty/src/session_interceptor.rs
- crates/aish-shell/src/nl_detect.rs
- crates/aish-llm/src/session.rs
- crates/aish-pty/src/nl_detect.rs
…rite failure - SshReadFileTool: only allow reading files under aish-offload directory to prevent remote LLM from accessing arbitrary local files - offload.rs: return clean_path=None when clean file write fails instead of returning a non-existent path
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/aish-llm/src/session.rs (1)
438-458:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDangling
tool_callsleft innew_messageson short-circuit return.The assistant message containing
tool_callsis pushed before the tool execution loop (lines 415–424 and ~770–780), but on short-circuit early return (lines 438–458 and 795–815):
- The current
tc'stool_resultis never appended tomessages- Any remaining tool calls in the batch are skipped entirely
Downstream callers (
app.rslines 2329, 3036) extendnew_messagesdirectly into conversation history for the next turn. OpenAI and Anthropic enforce strict pairing: every assistanttool_callsmessage must be followed by atoolmessage for each call. An incomplete sequence will cause the next request to be rejected with a 4xx error.Fix: Before the short-circuit return, append a
tool_resultfor the blocked tool and for any remaining unexecuted calls, ensuring the message sequence remains well-formed:Suggested fix (both code paths)
if short_circuit { // ... emit events ... let text = if self.security_notice_callback.is_some() { String::new() } else { output }; + // Keep new_messages well-formed: every tool_call needs a tool_result. + messages.push(ChatMessage::tool_result(&tc.id, output)); + for remaining in tool_calls.iter().skip_while(|c| c.id != tc.id).skip(1) { + messages.push(ChatMessage::tool_result( + &remaining.id, + "Skipped: previous tool was blocked by security policy.".into(), + )); + } let new_messages = messages[initial_len..].to_vec(); return Ok(crate::types::ProcessResult { text, new_messages }); }🤖 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-llm/src/session.rs` around lines 438 - 458, On short-circuit returns in session.rs (inside the block that emits LlmEventType::GenerationEnd and OpEnd and returns a crate::types::ProcessResult), you must append well-formed tool result messages for the current tool call (tc) and any remaining unexecuted entries from tool_calls before building new_messages and returning; otherwise the assistant tool_calls messages are left dangling. Fix by iterating from the current tool call index through the rest of tool_calls, creating and pushing a corresponding tool result message (matching the structure used elsewhere when a tool returns, e.g., a tool_result field or a message with role/type for tool outputs) into messages[initial_len..] (or into new_messages) for tc and each subsequent call, then proceed to construct new_messages and return as before so every assistant tool_calls has a paired tool result. Ensure you use the same message shape and metadata as the normal tool execution path so downstream code (and OpenAI/Anthropic) sees properly paired assistant tool_calls -> tool messages.
🧹 Nitpick comments (6)
crates/aish-pty/src/offload.rs (1)
217-231: ⚡ Quick winConsider adding a unit test for
cancel().The new
cancel()path is otherwise untested. A small test that triggers overflow on both streams, callscancel(), and asserts that the.rawfiles and (when empty) the per-session directory are removed would lock in the intended cleanup contract and protect against future regressions (e.g., leftover artifacts after a Ctrl+C abort inpersistent.rs).🧪 Proposed test sketch
#[test] fn test_cancel_removes_overflow_files_and_dir() { let dir = tmp_dir("cancel_cleanup"); let mut offload = PtyOutputOffload::new( "ls", "test-uuid-cancel", "", 4, // tiny keep_bytes to force overflow on both streams dir.to_str().unwrap(), ); offload.append_overflow(StreamName::Stdout, b"stdout overflow data\n"); offload.append_overflow(StreamName::Stderr, b"stderr overflow data\n"); let session_dir = dir.join("aish-offload").join("test-uuid-cancel"); let stdout_raw = session_dir.join("stdout.raw"); let stderr_raw = session_dir.join("stderr.raw"); assert!(stdout_raw.exists()); assert!(stderr_raw.exists()); offload.cancel(); assert!(!stdout_raw.exists(), "stdout.raw should be removed"); assert!(!stderr_raw.exists(), "stderr.raw should be removed"); assert!(!session_dir.exists(), "empty session dir should be removed"); }🤖 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/offload.rs` around lines 217 - 231, Add a unit test that constructs a PtyOutputOffload (use PtyOutputOffload::new with a small keep_bytes to force overflow), call append_overflow for both StreamName::Stdout and StreamName::Stderr to create the .raw files, assert those files exist, call cancel(), then assert the stdout.raw and stderr.raw files are removed and the per-session directory no longer exists; reference the methods/types PtyOutputOffload::new, append_overflow, cancel, and StreamName so the test locates the same behavior and prevents leftover artifacts after aborts.crates/aish-pty/src/nl_detect.rs (4)
387-392: ⚖️ Poor tradeoffConsider improving word simplification logic.
The suffix stripping (
"running"→"runn") is simplistic and may not match intended keyword stems. For instance,"running"should stem to"run"(which is inNL_KEYWORDS), but the current logic produces"runn", which likely isn't in the keyword set. While the code checks both the original word and simplified version (line 400), the stemming could be more effective with proper algorithmic stemming (e.g., Porter stemmer) or by including common inflected forms directly in the keyword set.Given this is heuristic detection, the current approach is acceptable, but accuracy could improve with better stemming.
🤖 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/nl_detect.rs` around lines 387 - 392, The current simplify_word function naively strips "ing", "ed", "ly" (e.g., "running" -> "runn") which misses canonical stems; replace or enhance simplify_word to produce real stems by either integrating a Rust stemming library (e.g., the Porter stemmer crate) or implementing simple English normalization rules (handle doubled consonants like "running" -> "run", drop terminal "e" before "ing"/"ed", and normalize common irregulars) so that lookups against NL_KEYWORDS and the original word are more likely to match intended keywords; update references to simplify_word accordingly to use the new stemmer.
665-747: ⚡ Quick winConsider adding test coverage for hyphenated English phrases.
The current tests don't verify behavior for hyphenated natural language phrases (e.g.,
"long-term solution","well-known issue"). Since-is included inSHELL_SYNTAX_CHARS, these phrases may receive shell-syntax penalties. Adding a test would document the expected behavior and help verify if the current handling is correct.💡 Suggested test
#[test] fn test_hyphenated_phrases() { // Hyphenated English phrases should still be detected as NL assert!(looks_like_natural_language("how to fix long-term issue")); assert!(looks_like_natural_language("what is a well-known solution")); }🤖 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/nl_detect.rs` around lines 665 - 747, Add a test to cover hyphenated English phrases to ensure the hyphen (which is in SHELL_SYNTAX_CHARS) doesn't incorrectly trigger a shell-syntax penalty: create a new #[test] fn test_hyphenated_phrases() that calls looks_like_natural_language("how to fix long-term issue") and looks_like_natural_language("what is a well-known solution") (and optionally the safe wrapper looks_like_natural_language_safe for the same strings) and assert they return true; reference looks_like_natural_language and looks_like_natural_language_safe to locate where to add the test in the tests module.
361-368: ⚡ Quick winConsider renaming
is_cjktois_chineseor expanding Unicode coverage.The function name suggests it detects CJK (Chinese, Japanese, Korean) characters, but the Unicode ranges only cover Chinese ideographs. Japanese kana (\u{3040}-\u{309F} for Hiragana, \u{30A0}-\u{30FF} for Katakana) and Korean Hangul (\u{AC00}-\u{D7AF}) are not included. Either:
- Rename to
is_chinesefor accuracy, or- Expand the ranges to fully support CJK if that's the intended scope.
Given that
NlLanguagehas aChinesevariant (notCJK), renaming may be more appropriate.♻️ Proposed rename
-fn is_cjk(c: char) -> bool { +fn is_chinese(c: char) -> bool { matches!( c, '\u{4E00}'..='\u{9FFF}'Then update call sites at lines 375, 617, 651.
🤖 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/nl_detect.rs` around lines 361 - 368, The helper is_cjk currently only matches Chinese ideograph ranges but its name implies full CJK coverage; either change its name to is_chinese to match behavior or expand its Unicode ranges to include Hiragana (\u{3040}-\u{309F}), Katakana (\u{30A0}-\u{30FF}) and Hangul (\u{AC00}-\u{D7AF}) if you want true CJK detection; if you choose rename, update all call sites that use is_cjk (references around the places that determine NlLanguage/Chinese detection) to use is_chinese and keep behavior identical, or if you expand ranges, update the is_cjk implementation to include the extra Unicode ranges so callers can remain unchanged.
348-350: 💤 Low valueRedundant space check in
has_shell_syntax.The condition
!word.contains(' ')is redundant because this function is called on tokens fromsplit_whitespace()(line 621, 397), which never contain internal spaces. Consider removing this check for clarity.♻️ Proposed simplification
fn has_shell_syntax(word: &str) -> bool { - !word.contains(' ') && word.contains(SHELL_SYNTAX_CHARS) + word.contains(SHELL_SYNTAX_CHARS) }🤖 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/nl_detect.rs` around lines 348 - 350, The function has_shell_syntax currently checks both !word.contains(' ') and word.contains(SHELL_SYNTAX_CHARS); since callers use split_whitespace() (tokens never have internal spaces), remove the redundant !word.contains(' ') check and simplify has_shell_syntax to return word.contains(SHELL_SYNTAX_CHARS) only; update any references or tests expecting the old behavior if needed (look for has_shell_syntax and callers using split_whitespace()).crates/aish-llm/src/session.rs (1)
426-501: ⚡ Quick winTool-execution loop is duplicated between JSON and streaming paths.
The block that walks
for tc in &tool_calls { execute_tool; short_circuit; consecutive_failures; push tool_result }is essentially copy-pasted between theLlmResponse::JsonandLlmResponse::Streamarms. The short-circuit branch (lines 438–458 vs 795–815), the consecutive-failures branch (lines 462–498 vs 819–855), and the trailingmessages.push(ChatMessage::tool_result(...))are all identical. This is already drifting risk: the related fix for the dangling-tool_callsissue above will need to be applied in both places.Consider extracting a private helper such as
execute_tool_batch(&self, tool_calls: &[ToolCall], messages: &mut Vec<ChatMessage>, initial_len: usize, consecutive_failures: &mut usize) -> ControlFlow<ProcessResult, ()>so both paths share one implementation.Also applies to: 783-858
🤖 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-llm/src/session.rs` around lines 426 - 501, The tool-execution loop is duplicated between the LlmResponse::Json and LlmResponse::Stream branches; extract the shared logic into a private helper (e.g. execute_tool_batch) that accepts &self, a &[ToolCall], &mut Vec<ChatMessage>, initial_len: usize, and &mut consecutive_failures and returns a ControlFlow<crate::types::ProcessResult, ()> so it can short-circuit with a ProcessResult when a tool triggers short-circuit or too many consecutive failures; inside the helper call self.execute_tool, do the langfuse span_tool_call, emit the same LlmEvent events for GenerationEnd/OpEnd/Error, update consecutive_failures, push ChatMessage::tool_result(&tc.id, output) and on early return build and return the same crate::types::ProcessResult used in both branches; then replace the duplicated for tc in &tool_calls { ... } blocks in both LlmResponse::Json and LlmResponse::Stream with a single call to execute_tool_batch and handle the ControlFlow result.
🤖 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-pty/src/nl_detect.rs`:
- Around line 344-346: SHELL_SYNTAX_CHARS currently includes '-' which will
wrongly penalize normal hyphenated English words; remove '-' from the constant
SHELL_SYNTAX_CHARS and implement a targeted hyphen check where the constant is
used: instead of treating any '-' as shell-syntax, add pattern logic that only
counts hyphens when they appear as standalone tokens, as leading dashes in a
token (e.g., token.starts_with('--') or token.starts_with('-') followed by
alphanumeric), or adjacent to flags (e.g., "--flag"), and leave internal hyphens
inside words (e.g., "well-known") unpenalized. Update the code paths that
compute shell-syntax penalties to use the new targeted-hyphen rule so other
shell-special chars still use SHELL_SYNTAX_CHARS unchanged.
In `@crates/aish-tools/src/fs.rs`:
- Around line 199-205: The code currently checks any path component equals
"aish-offload" (via canonical.components().any(...)), which is too permissive;
change the check to verify the canonical path is actually under the authorized
offload root by comparing canonical.starts_with(offload_root) (or equivalent
Path::starts_with) and return ToolResult::error when that check fails; locate
the logic around the canonical variable and the has_offload_component check and
replace that component-based test with a starts_with(offload_root) boundary
check to enforce the exact offload root.
- Around line 187-207: The code currently validates the canonical path into
`canonical` and checks `has_offload_component` but then calls
`self.inner.execute(args)` using the original (unchecked) `args`, creating a
TOCTOU window; fix by replacing the path in the `args` passed to `ReadFileTool`
with the validated canonical path (e.g., create a new args map/struct where the
path value is set to `canonical.to_string_lossy().into_owned()` or
canonical.to_str().unwrap().to_owned()) and pass that modified args to
`self.inner.execute(...)` so the read uses the already-canonicalized path.
---
Outside diff comments:
In `@crates/aish-llm/src/session.rs`:
- Around line 438-458: On short-circuit returns in session.rs (inside the block
that emits LlmEventType::GenerationEnd and OpEnd and returns a
crate::types::ProcessResult), you must append well-formed tool result messages
for the current tool call (tc) and any remaining unexecuted entries from
tool_calls before building new_messages and returning; otherwise the assistant
tool_calls messages are left dangling. Fix by iterating from the current tool
call index through the rest of tool_calls, creating and pushing a corresponding
tool result message (matching the structure used elsewhere when a tool returns,
e.g., a tool_result field or a message with role/type for tool outputs) into
messages[initial_len..] (or into new_messages) for tc and each subsequent call,
then proceed to construct new_messages and return as before so every assistant
tool_calls has a paired tool result. Ensure you use the same message shape and
metadata as the normal tool execution path so downstream code (and
OpenAI/Anthropic) sees properly paired assistant tool_calls -> tool messages.
---
Nitpick comments:
In `@crates/aish-llm/src/session.rs`:
- Around line 426-501: The tool-execution loop is duplicated between the
LlmResponse::Json and LlmResponse::Stream branches; extract the shared logic
into a private helper (e.g. execute_tool_batch) that accepts &self, a
&[ToolCall], &mut Vec<ChatMessage>, initial_len: usize, and &mut
consecutive_failures and returns a ControlFlow<crate::types::ProcessResult, ()>
so it can short-circuit with a ProcessResult when a tool triggers short-circuit
or too many consecutive failures; inside the helper call self.execute_tool, do
the langfuse span_tool_call, emit the same LlmEvent events for
GenerationEnd/OpEnd/Error, update consecutive_failures, push
ChatMessage::tool_result(&tc.id, output) and on early return build and return
the same crate::types::ProcessResult used in both branches; then replace the
duplicated for tc in &tool_calls { ... } blocks in both LlmResponse::Json and
LlmResponse::Stream with a single call to execute_tool_batch and handle the
ControlFlow result.
In `@crates/aish-pty/src/nl_detect.rs`:
- Around line 387-392: The current simplify_word function naively strips "ing",
"ed", "ly" (e.g., "running" -> "runn") which misses canonical stems; replace or
enhance simplify_word to produce real stems by either integrating a Rust
stemming library (e.g., the Porter stemmer crate) or implementing simple English
normalization rules (handle doubled consonants like "running" -> "run", drop
terminal "e" before "ing"/"ed", and normalize common irregulars) so that lookups
against NL_KEYWORDS and the original word are more likely to match intended
keywords; update references to simplify_word accordingly to use the new stemmer.
- Around line 665-747: Add a test to cover hyphenated English phrases to ensure
the hyphen (which is in SHELL_SYNTAX_CHARS) doesn't incorrectly trigger a
shell-syntax penalty: create a new #[test] fn test_hyphenated_phrases() that
calls looks_like_natural_language("how to fix long-term issue") and
looks_like_natural_language("what is a well-known solution") (and optionally the
safe wrapper looks_like_natural_language_safe for the same strings) and assert
they return true; reference looks_like_natural_language and
looks_like_natural_language_safe to locate where to add the test in the tests
module.
- Around line 361-368: The helper is_cjk currently only matches Chinese
ideograph ranges but its name implies full CJK coverage; either change its name
to is_chinese to match behavior or expand its Unicode ranges to include Hiragana
(\u{3040}-\u{309F}), Katakana (\u{30A0}-\u{30FF}) and Hangul (\u{AC00}-\u{D7AF})
if you want true CJK detection; if you choose rename, update all call sites that
use is_cjk (references around the places that determine NlLanguage/Chinese
detection) to use is_chinese and keep behavior identical, or if you expand
ranges, update the is_cjk implementation to include the extra Unicode ranges so
callers can remain unchanged.
- Around line 348-350: The function has_shell_syntax currently checks both
!word.contains(' ') and word.contains(SHELL_SYNTAX_CHARS); since callers use
split_whitespace() (tokens never have internal spaces), remove the redundant
!word.contains(' ') check and simplify has_shell_syntax to return
word.contains(SHELL_SYNTAX_CHARS) only; update any references or tests expecting
the old behavior if needed (look for has_shell_syntax and callers using
split_whitespace()).
In `@crates/aish-pty/src/offload.rs`:
- Around line 217-231: Add a unit test that constructs a PtyOutputOffload (use
PtyOutputOffload::new with a small keep_bytes to force overflow), call
append_overflow for both StreamName::Stdout and StreamName::Stderr to create the
.raw files, assert those files exist, call cancel(), then assert the stdout.raw
and stderr.raw files are removed and the per-session directory no longer exists;
reference the methods/types PtyOutputOffload::new, append_overflow, cancel, and
StreamName so the test locates the same behavior and prevents leftover artifacts
after aborts.
🪄 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: 9d561618-052c-4b22-860f-096bb29bf84e
📒 Files selected for processing (9)
crates/aish-llm/src/session.rscrates/aish-pty/src/nl_detect.rscrates/aish-pty/src/offload.rscrates/aish-pty/src/persistent.rscrates/aish-pty/src/session_interceptor.rscrates/aish-shell/src/app.rscrates/aish-shell/src/nl_detect.rscrates/aish-tools/src/channel_bash.rscrates/aish-tools/src/fs.rs
🚧 Files skipped from review as they are similar to previous changes (5)
- crates/aish-pty/src/session_interceptor.rs
- crates/aish-tools/src/channel_bash.rs
- crates/aish-shell/src/nl_detect.rs
- crates/aish-pty/src/persistent.rs
- crates/aish-shell/src/app.rs
- SshReadFileTool: use starts_with(offload_root) for exact boundary check and pass canonical path to inner tool to avoid TOCTOU - NL detect: remove '-' from SHELL_SYNTAX_CHARS and instead check for leading-dash flag pattern, so hyphenated words like "well-known" are not penalized
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 test -p aish-pty -p aish-shell -p aish-llm -p aish-toolspasses/tmp/aish-offload/nstops further tool callsSummary by CodeRabbit
New Features
Bug Fixes
Documentation