fix: revert rust parity gap changes#137
Merged
Merged
Conversation
This reverts commit d8e431f.
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:
|
📝 WalkthroughWalkthroughThis PR migrates the release distribution system from CDN-based delivery to GitHub Releases. The changes remove CDN publishing infrastructure, update workflow definitions to use Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant GitHub as GitHub API
participant Mirror as Mirror/Fallback
participant Disk as Local Storage
Client->>GitHub: check_for_updates()<br/>(releases/latest or /releases)
activate GitHub
GitHub-->>Client: Release metadata + asset info
deactivate GitHub
Client->>GitHub: download_release()<br/>(platform-tagged archive)
activate GitHub
GitHub-->>Client: Bundle downloaded
deactivate GitHub
Client->>Disk: Save bundle
alt GitHub download fails
Client->>Mirror: Retry download from mirror
activate Mirror
Mirror-->>Client: Bundle downloaded
deactivate Mirror
Client->>Disk: Save bundle
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packaging/build_bundle.sh (1)
18-22:⚠️ Potential issue | 🔴 CriticalKeep bundle filenames aligned with the updater.
crates/aish-cli/src/update.rs:59-80still normalizes Linuxx86_64toamd64, anddownload_release()incrates/aish-cli/src/update.rs:312-316still requestsaish-<version>-linux-amd64.tar.gz. After this change, the release workflows publishaish-<version>-linux-x86_64.tar.gzinstead, soaish updateon Linux/x86_64 will stop finding the released asset. Please either preserve the existingamd64bundle naming here or land the corresponding updater rename in the same PR.🔧 Minimal fix if you want to preserve the current client contract
+normalize_bundle_arch() { + case "$1" in + x86_64|amd64) echo "amd64" ;; + aarch64|arm64) echo "arm64" ;; + *) echo "$1" ;; + esac +} + -ARCH="${ARCH:-${2:-x86_64}}" +RAW_ARCH="${ARCH:-${2:-x86_64}}" +ARCH="$(normalize_bundle_arch "$RAW_ARCH")"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packaging/build_bundle.sh` around lines 18 - 22, The bundle naming change broke compatibility with the updater: update the packaging script so BUNDLE_NAME matches the updater's expected asset name (or make the updater change in the same PR). Specifically, in packaging/build_bundle.sh adjust how ARCH is normalized (the ARCH variable and BUNDLE_NAME) so that when PLATFORM is "linux" and ARCH is "x86_64" the output uses "amd64" (preserving the existing aish-<version>-linux-amd64.tar.gz name expected by download_release() and the normalization in crates/aish-cli/src/update.rs), or alternatively ensure the corresponding rename is applied to download_release()/normalization in crates/aish-cli/src/update.rs in the same change set.crates/aish-cli/src/update.rs (2)
202-207:⚠️ Potential issue | 🟡 MinorMisleading error message for non-GitHub download failures.
Using
cli.update.github_api_errorfor download failures (which could be from the mirror) is confusing. Consider using a more generic download error key.📝 Suggested fix
if !resp.status().is_success() { return Err(AishError::Config({ let mut args = std::collections::HashMap::new(); args.insert("status".to_string(), resp.status().to_string()); - t_with_args("cli.update.github_api_error", &args) + t_with_args("cli.update.download_status_error", &args) })); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/aish-cli/src/update.rs` around lines 202 - 207, The error path currently returns AishError::Config with the message key "cli.update.github_api_error" which is misleading for non-GitHub download failures; update the code in the block that checks resp.status() to use a more generic error key (e.g., "cli.update.download_error") when constructing the t_with_args payload, leaving the status insertion into the args and the AishError::Config wrapping unchanged so callers still get the HTTP status detail.
103-150:⚠️ Potential issue | 🟡 MinorPre-release selection may not select an actual pre-release.
When
include_pre_releaseis true, the code fetches all releases and takes the first one (line 138). GitHub's/releasesendpoint returns releases sorted by creation date descending, so this gets the most recent release—but that could be a stable release, not a pre-release. If the intent is to specifically get pre-releases, filter for entries whereprerelease: true.🔧 Suggested fix
let release = if include_pre_release { let releases: Vec<serde_json::Value> = resp.json().map_err(|e| { AishError::Config({ let mut args = std::collections::HashMap::new(); args.insert("error".to_string(), e.to_string()); t_with_args("cli.update.parse_releases_failed", &args) }) })?; - match releases.into_iter().next() { + // When include_pre_release is set, find the first pre-release or fall back to latest + match releases.into_iter().find(|r| { + r.get("prerelease").and_then(|v| v.as_bool()).unwrap_or(false) + }).or_else(|| releases.into_iter().next()) { Some(r) => r, None => return Ok(None), }Note: The above diff has a borrow issue; you'd need to restructure to avoid consuming
releasestwice.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/aish-cli/src/update.rs` around lines 103 - 150, In check_for_updates, when include_pre_release is true you currently take the first item from the releases list which may be a stable release; instead, parse the response into a Vec<serde_json::Value> (as you already do) and then choose the first item where the "prerelease" field is true (e.g. releases.into_iter().filter(|r| r.get("prerelease").and_then(|v| v.as_bool()) == Some(true)).next()), returning Ok(None if none found); ensure you only consume the Vec once (use into_iter and filtering) and keep the same error mapping on resp.json() failures and the Same return behavior if no prerelease exists.
🧹 Nitpick comments (5)
crates/aish-cli/src/update.rs (2)
327-342: GitHub download error is silently swallowed before fallback.When the GitHub download fails, the error is discarded and the code falls back to the mirror without informing the user why the primary source failed. Consider printing the error for diagnostics before attempting the fallback.
🔧 Suggested improvement
// Try GitHub first let github_url = format!("{}/{}/{}", GITHUB_RELEASES_BASE, tag_name, filename); println!( "\x1b[1;36m{}\x1b[0m", t("cli.update.downloading_from_github") ); - if download_with_progress(&github_url, &dest_path, &filename).is_ok() { + match download_with_progress(&github_url, &dest_path, &filename) { + Ok(()) => { let path_str = dest_path.display().to_string(); println!("\x1b[32m{}\x1b[0m", { let mut args = std::collections::HashMap::new(); args.insert("path".to_string(), path_str); t_with_args("cli.update.downloaded", &args) }); return Ok(dest_path); + } + Err(e) => { + eprintln!("\x1b[33m{}: {}\x1b[0m", t("cli.update.github_download_failed"), e); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/aish-cli/src/update.rs` around lines 327 - 342, The GitHub download error is being discarded when calling download_with_progress(&github_url, &dest_path, &filename); modify this block to capture the Result error on failure and log or print it before falling back to the mirror: call download_with_progress, match or if let Err(e) = ... to get the error, print a user-facing diagnostic (e.g., using println! with t("cli.update.downloading_from_github") context and include e.to_string()) so the failure reason is visible, and only proceed to the mirror fallback if the error occurs; keep the successful path that prints t_with_args("cli.update.downloaded", ...) and returns Ok(dest_path) unchanged.
419-444: Security consideration: install.sh is executed with sudo after download.The code downloads
install.shfrom the internet (GitHub or mirror) and executes it withsudo. While SHA256 is displayed for manual verification (line 423-428), there's no automated integrity check against a known-good hash. Consider:
- Verifying the archive checksum against a published
.sha256file- Or requiring GPG signature verification
This is a defense-in-depth concern—GitHub releases are generally trusted, but supply chain attacks are a real threat.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/aish-cli/src/update.rs` around lines 419 - 444, The code currently computes sha256_file(&install_script) and then runs the script with sudo (std::process::Command::new("sudo")...arg(&install_script)), but never verifies that hash against a trusted value; update the flow in the update routine so that after finding the install script (find_install_sh) and computing its hash (sha256_file) you fetch the published checksum or signature from the release (e.g., the .sha256 asset or a detached GPG signature), verify the computed hash matches the published checksum (or verify the GPG signature using a bundled/trusted public key), and abort with a clear AishError::Config (use t_with_args("cli.update.install_script_failed", ...) including both expected and actual values) if verification fails; only proceed to invoke sudo on install_script when verification succeeds.crates/aish-tools/src/bash.rs (3)
210-222: Test validates timeout kills command but doesn't verify output indication.The test confirms
result.okis true after timeout, but doesn't assert that the output indicates the command was terminated due to timeout. Consider adding an assertion that helps distinguish timeout-killed commands from completed ones.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/aish-tools/src/bash.rs` around lines 210 - 222, The test test_bash_tool_timeout currently only checks result.ok after running BashTool::execute with a short timeout; update it to also assert that the result indicates a timeout kill (e.g., check result.output or result.error contains a timeout/kill message or that an exit/status field shows a timeout) so timeout-killed commands are distinguishable from normal completions. Locate test_bash_tool_timeout and the call to BashTool::new().execute(...) and add an assertion that inspects result.output (or result.error/exit/status if present) for a timeout-specific substring like "timed out" or "killed" (or the code/path your execute() uses to report timeouts) to make the test deterministic.
91-97: Timeout thread continues running after command completes.The spawned thread sleeps for the full timeout duration regardless of whether the command finishes early. For short commands with 120s timeout, this creates 120s of unnecessary thread lifetime. While the thread will eventually terminate and resource impact is minimal, consider using a more structured approach.
♻️ Alternative using scoped threads or early exit
One option is to use a separate "done" flag that the main thread sets after
execute_blockingreturns, allowing the timeout thread to check periodically:let done = Arc::new(std::sync::atomic::AtomicBool::new(false)); let done_clone = Arc::clone(&done); let timeout_token = Arc::clone(&cancel_token); let timeout_duration = Duration::from_secs(timeout_secs); std::thread::spawn(move || { let check_interval = Duration::from_millis(100); let mut elapsed = Duration::ZERO; while elapsed < timeout_duration { if done_clone.load(std::sync::atomic::Ordering::Relaxed) { return; } std::thread::sleep(check_interval); elapsed += check_interval; } timeout_token.cancel(); }); // After execute_blocking returns: done.store(true, std::sync::atomic::Ordering::Relaxed);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/aish-tools/src/bash.rs` around lines 91 - 97, The timeout thread currently always sleeps the full timeout and then calls cancel_token.cancel(); change it to stop early by adding a shared done flag (e.g., Arc<AtomicBool>) that the timeout thread polls periodically while accumulating elapsed time (use timeout_duration and a small check_interval), and have the main thread set done.store(true, Ordering::Relaxed) immediately after execute_blocking returns; keep using Arc::clone(&cancel_token) for timeout_token and call timeout_token.cancel() only if the loop reaches the timeout without done being set.
62-66: Consider keepingminimum: 1constraint in schema.Removing the minimum constraint allows
timeout: 0which would cause immediate cancellation viasleep(Duration::from_secs(0)). While technically not breaking, a zero timeout is likely never intentional. Retaining"minimum": 1documents the expected input range to LLM callers.📝 Suggested schema fix
"timeout": { "type": "integer", "description": "Timeout in seconds (default: 120)", - "default": 120 + "default": 120, + "minimum": 1 }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/aish-tools/src/bash.rs` around lines 62 - 66, The JSON schema for the "timeout" property currently omits a minimum constraint; restore a "minimum": 1 to the "timeout" field in the schema so callers cannot pass 0 (which would cause immediate cancellation via sleep(Duration::from_secs(0))); update the "timeout" property definition (the entry named "timeout" in the schema) to include "minimum": 1 alongside the existing "type", "description", and "default".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Line 129: The README sentence claiming the installer resolves releases under
https://www.aishell.ai/repo is inaccurate relative to update.rs; update the
installer description to state that the installer queries GitHub Releases as the
primary source and only falls back to the aishell.ai mirror
(https://www.aishell.ai/repo) if GitHub release retrieval fails — mention
"update.rs" and "GitHub Releases" (primary) and "https://www.aishell.ai/repo"
(fallback) so readers can map the behavior to the code.
---
Outside diff comments:
In `@crates/aish-cli/src/update.rs`:
- Around line 202-207: The error path currently returns AishError::Config with
the message key "cli.update.github_api_error" which is misleading for non-GitHub
download failures; update the code in the block that checks resp.status() to use
a more generic error key (e.g., "cli.update.download_error") when constructing
the t_with_args payload, leaving the status insertion into the args and the
AishError::Config wrapping unchanged so callers still get the HTTP status
detail.
- Around line 103-150: In check_for_updates, when include_pre_release is true
you currently take the first item from the releases list which may be a stable
release; instead, parse the response into a Vec<serde_json::Value> (as you
already do) and then choose the first item where the "prerelease" field is true
(e.g. releases.into_iter().filter(|r| r.get("prerelease").and_then(|v|
v.as_bool()) == Some(true)).next()), returning Ok(None if none found); ensure
you only consume the Vec once (use into_iter and filtering) and keep the same
error mapping on resp.json() failures and the Same return behavior if no
prerelease exists.
In `@packaging/build_bundle.sh`:
- Around line 18-22: The bundle naming change broke compatibility with the
updater: update the packaging script so BUNDLE_NAME matches the updater's
expected asset name (or make the updater change in the same PR). Specifically,
in packaging/build_bundle.sh adjust how ARCH is normalized (the ARCH variable
and BUNDLE_NAME) so that when PLATFORM is "linux" and ARCH is "x86_64" the
output uses "amd64" (preserving the existing aish-<version>-linux-amd64.tar.gz
name expected by download_release() and the normalization in
crates/aish-cli/src/update.rs), or alternatively ensure the corresponding rename
is applied to download_release()/normalization in crates/aish-cli/src/update.rs
in the same change set.
---
Nitpick comments:
In `@crates/aish-cli/src/update.rs`:
- Around line 327-342: The GitHub download error is being discarded when calling
download_with_progress(&github_url, &dest_path, &filename); modify this block to
capture the Result error on failure and log or print it before falling back to
the mirror: call download_with_progress, match or if let Err(e) = ... to get the
error, print a user-facing diagnostic (e.g., using println! with
t("cli.update.downloading_from_github") context and include e.to_string()) so
the failure reason is visible, and only proceed to the mirror fallback if the
error occurs; keep the successful path that prints
t_with_args("cli.update.downloaded", ...) and returns Ok(dest_path) unchanged.
- Around line 419-444: The code currently computes sha256_file(&install_script)
and then runs the script with sudo
(std::process::Command::new("sudo")...arg(&install_script)), but never verifies
that hash against a trusted value; update the flow in the update routine so that
after finding the install script (find_install_sh) and computing its hash
(sha256_file) you fetch the published checksum or signature from the release
(e.g., the .sha256 asset or a detached GPG signature), verify the computed hash
matches the published checksum (or verify the GPG signature using a
bundled/trusted public key), and abort with a clear AishError::Config (use
t_with_args("cli.update.install_script_failed", ...) including both expected and
actual values) if verification fails; only proceed to invoke sudo on
install_script when verification succeeds.
In `@crates/aish-tools/src/bash.rs`:
- Around line 210-222: The test test_bash_tool_timeout currently only checks
result.ok after running BashTool::execute with a short timeout; update it to
also assert that the result indicates a timeout kill (e.g., check result.output
or result.error contains a timeout/kill message or that an exit/status field
shows a timeout) so timeout-killed commands are distinguishable from normal
completions. Locate test_bash_tool_timeout and the call to
BashTool::new().execute(...) and add an assertion that inspects result.output
(or result.error/exit/status if present) for a timeout-specific substring like
"timed out" or "killed" (or the code/path your execute() uses to report
timeouts) to make the test deterministic.
- Around line 91-97: The timeout thread currently always sleeps the full timeout
and then calls cancel_token.cancel(); change it to stop early by adding a shared
done flag (e.g., Arc<AtomicBool>) that the timeout thread polls periodically
while accumulating elapsed time (use timeout_duration and a small
check_interval), and have the main thread set done.store(true,
Ordering::Relaxed) immediately after execute_blocking returns; keep using
Arc::clone(&cancel_token) for timeout_token and call timeout_token.cancel() only
if the loop reaches the timeout without done being set.
- Around line 62-66: The JSON schema for the "timeout" property currently omits
a minimum constraint; restore a "minimum": 1 to the "timeout" field in the
schema so callers cannot pass 0 (which would cause immediate cancellation via
sleep(Duration::from_secs(0))); update the "timeout" property definition (the
entry named "timeout" in the schema) to include "minimum": 1 alongside the
existing "type", "description", and "default".
🪄 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: b3d8f3eb-6228-4a41-9660-6e9db523584d
📒 Files selected for processing (15)
.github/workflows/release-preparation.yml.github/workflows/release.ymlCHANGELOG.mdCONTRIBUTING.mdQUICKSTART.mdREADME.mdcrates/aish-cli/src/update.rscrates/aish-i18n/locales/en-US.yamlcrates/aish-i18n/locales/zh-CN.yamlcrates/aish-llm/src/session.rscrates/aish-shell/src/app.rscrates/aish-tools/src/bash.rscrates/aish-tools/src/system_diagnose.rspackaging/build_bundle.shpackaging/scripts/publish_release.sh
💤 Files with no reviewable changes (1)
- packaging/scripts/publish_release.sh
| ``` | ||
|
|
||
| The installer resolves the latest stable version from `https://cdn.aishell.ai/download/latest`, downloads the matching bundle from `https://cdn.aishell.ai/download/releases/<version>/`, and installs `aish`, `aish-sandbox`, and `aish-uninstall` into `/usr/local/bin`. | ||
| The installer resolves the latest release directory under `https://www.aishell.ai/repo`, downloads the matching bundle for your architecture, and installs `aish`, `aish-sandbox`, and `aish-uninstall` into `/usr/local/bin`. |
There was a problem hiding this comment.
Documentation describes mirror URL as primary source, but code uses GitHub as primary.
The installer description states it resolves releases under https://www.aishell.ai/repo, but update.rs uses GitHub Releases as the primary download source with aishell.ai/repo as the fallback mirror. Consider updating documentation to reflect the actual primary-then-fallback behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` at line 129, The README sentence claiming the installer resolves
releases under https://www.aishell.ai/repo is inaccurate relative to update.rs;
update the installer description to state that the installer queries GitHub
Releases as the primary source and only falls back to the aishell.ai mirror
(https://www.aishell.ai/repo) if GitHub release retrieval fails — mention
"update.rs" and "GitHub Releases" (primary) and "https://www.aishell.ai/repo"
(fallback) so readers can map the behavior to the code.
This was referenced May 9, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR reverts commit d8e431f.
Reason:
Validation:
Summary by CodeRabbit
Changed
timeoutparameter now defaults to 120 secondsDocumentation