fix: close rust parity gaps for update streaming and timeouts#133
Merged
F16shen merged 1 commit intoAI-Shell-Team:rustfrom Apr 24, 2026
Merged
fix: close rust parity gaps for update streaming and timeouts#133F16shen merged 1 commit intoAI-Shell-Team:rustfrom
F16shen merged 1 commit intoAI-Shell-Team:rustfrom
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:
|
📝 WalkthroughWalkthroughSwitches stable update and bundle distribution to a CDN layout (download/latest and download/releases//...), normalizes bundle architecture names to Changes
Sequence Diagram(s)sequenceDiagram
participant GH as GitHub Actions
participant Build as Build Job
participant Artifacts as Artifact Storage
participant Publish as publish_release.sh
participant CDN as CDN (R2/S3)
participant Validator as Validator (curl/sha256sum)
GH->>Build: run build_bundle (ARCH=amd64)
Build->>Artifacts: upload artifacts (release-bundle-linux-*)
GH->>Publish: start publish-release-bundles job
Publish->>Artifacts: download artifacts (release-bundle-linux-*)
Publish->>CDN: upload tar.gz and .sha256 to /releases/${VERSION}/ (immutable cache)
Publish->>CDN: upload VERSION to /latest (no-store)
Publish->>Validator: HEAD/GET checks (retries)
Validator-->>Publish: success/failure
Publish-->>GH: append summary with published URLs (optional)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/release.yml (1)
95-103:⚠️ Potential issue | 🟠 MajorThe CDN release flow only publishes
linux-amd64.
crates/aish-cli/src/update.rsstill advertises self-update bundles fordarwinandarm64, but this workflow now builds/uploads onlyaish-<version>-linux-amd64. After switching stable downloads to the CDN, macOS and Linux arm64 clients will resolve bundle URLs that can never exist here.📦 Suggested direction
- - name: Build release binary - run: cargo build --release --target x86_64-unknown-linux-musl + # Expand this into a matrix that builds/publishes every platform/arch + # combination that `detect_platform()` can return, or narrow + # `detect_platform()` to the bundles you actually ship.Also applies to: 129-172
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/release.yml around lines 95 - 103, The workflow currently builds/uploads only linux-amd64 artifacts (steps named "Build release binary" and "Build release bundle" using PLATFORM="linux" ARCH="amd64" and OUTPUT_DIR="artifacts/amd64"), but crates/aish-cli/src/update.rs still advertises darwin and arm64 bundles; either update the release workflow to build and upload the missing platform targets (add builds for darwin/arm64 with appropriate PLATFORM/ARCH/OUTPUT_DIR values and invoke packaging/build_bundle.sh for each) or change crates/aish-cli/src/update.rs to stop advertising darwin/arm64 bundles so advertised URLs match the actual uploaded artifacts—modify whichever path you choose and ensure the symbols "Build release bundle", PLATFORM, ARCH, OUTPUT_DIR and the update logic in update.rs are kept consistent.
🧹 Nitpick comments (3)
.github/workflows/release-preparation.yml (1)
133-135: Validate checksum content, not only file presence.The CDN layout check currently proves files exist, but not that
*.sha256matches the copied tarball.✅ Suggested integrity check addition
test -f "$RELEASE_LAYOUT/latest" test -f "$RELEASE_LAYOUT/releases/${VERSION}/$BUNDLE" test -f "$RELEASE_LAYOUT/releases/${VERSION}/$SHA_FILE" + ( + cd "$RELEASE_LAYOUT/releases/${VERSION}" + sha256sum -c "$SHA_FILE" + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/release-preparation.yml around lines 133 - 135, The workflow currently only checks file existence for $RELEASE_LAYOUT/releases/${VERSION}/$BUNDLE and $SHA_FILE; update it to validate the checksum content by computing the tarball's SHA-256 and comparing it against the contents of $SHA_FILE (or use sha256sum -c against the checksum file) so the integrity of $BUNDLE is verified; locate the block using the RELEASE_LAYOUT, VERSION, BUNDLE and SHA_FILE variables and replace the simple test -f check with a checksum comparison step that exits non‑zero on mismatch.QUICKSTART.md (1)
15-15: Consider documenting valid<arch>values explicitly.Adding
(amd64|arm64)here would reduce avoidable 404s from users tryingx86_64.✏️ Suggested docs tweak
-Download the matching `aish-<version>-linux-<arch>.tar.gz` bundle from the CDN release directory `https://cdn.aishell.ai/download/releases/<version>/`, then run: +Download the matching `aish-<version>-linux-<arch>.tar.gz` bundle (where `<arch>` is `amd64` or `arm64`) from the CDN release directory `https://cdn.aishell.ai/download/releases/<version>/`, then run:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@QUICKSTART.md` at line 15, The download instruction in QUICKSTART.md uses a generic `<arch>` which leads to 404s; update the text that shows the filename pattern `aish-<version>-linux-<arch>.tar.gz` and the CDN URL `https://cdn.aishell.ai/download/releases/<version>/` to explicitly list supported architectures (e.g., amd64 and arm64), and add a short note that users should use "amd64" rather than "x86_64" (or include both as accepted aliases) so readers know which values to substitute for `<arch>`.README.md (1)
84-84: Use full URLs here for consistency with the rest of install docs.Relative
download/...wording is slightly ambiguous versus explicit CDN URLs used elsewhere.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` at line 84, Update the README sentence to use full CDN URLs instead of relative paths: replace the ambiguous `download/latest` and `download/releases/<version>/...` fragments with the explicit CDN base (matching other docs) and mention the override environment variables AISH_DOWNLOAD_BASE_URL, AISH_LATEST_URL and legacy AISH_REPO_URL so readers clearly see the exact full endpoints to use; ensure the phrasing mirrors other install docs for consistency.
🤖 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-cli/src/update.rs`:
- Around line 674-741: The tests that mutate process-wide environment variables
(test_download_base_url_defaults_to_cdn,
test_download_base_url_prefers_explicit_override,
test_latest_version_url_uses_default_pattern,
test_latest_version_url_prefers_override,
test_release_download_url_uses_versioned_cdn_path) must be serialized to avoid
races; add the serial_test crate as a dev-dependency and annotate each of those
test functions with #[serial] (or alternatively guard the env changes with a
shared Mutex) so environment setup/teardown in download_base_url(),
latest_version_url(), and release_download_url() runs exclusively.
- Around line 398-400: The downloaded release bundle must be integrity-checked
before extraction/execution: after building release_url with
release_download_url(tag_name, &filename) and after
download_with_progress(&release_url, &dest_path, &filename) returns, fetch the
corresponding .sha256 artifact (e.g., release_download_url(tag_name,
&format!("{}.sha256", filename))) or a remote checksum endpoint, compute the
SHA-256 of the downloaded file (read dest_path, compute digest, hex-lowercase
it), trim and compare it to the expected checksum value from the .sha256 file,
and fail (return Err) if they don’t match; only proceed to untar/run install.sh
after this verification. Ensure you reference and update the code paths around
release_download_url, download_with_progress, dest_path and filename to
implement this check.
In `@packaging/scripts/publish_release.sh`:
- Around line 78-80: The CDN validation loop currently runs a single curl HEAD
per URL (the for loop over validated_urls with the curl -fsSI call) which can
fail due to eventual consistency; modify the loop to retry each URL several
times with exponential or fixed backoff before giving up: for each entry in
validated_urls run the curl command, if it fails retry up to N attempts
(configurable constant) with sleep intervals (increasing or fixed), and only
mark the URL as failed after all attempts exhaust; ensure the retry logic
returns the original curl exit code on final failure and keeps the existing
timeout flags (--connect-timeout, --max-time).
---
Outside diff comments:
In @.github/workflows/release.yml:
- Around line 95-103: The workflow currently builds/uploads only linux-amd64
artifacts (steps named "Build release binary" and "Build release bundle" using
PLATFORM="linux" ARCH="amd64" and OUTPUT_DIR="artifacts/amd64"), but
crates/aish-cli/src/update.rs still advertises darwin and arm64 bundles; either
update the release workflow to build and upload the missing platform targets
(add builds for darwin/arm64 with appropriate PLATFORM/ARCH/OUTPUT_DIR values
and invoke packaging/build_bundle.sh for each) or change
crates/aish-cli/src/update.rs to stop advertising darwin/arm64 bundles so
advertised URLs match the actual uploaded artifacts—modify whichever path you
choose and ensure the symbols "Build release bundle", PLATFORM, ARCH, OUTPUT_DIR
and the update logic in update.rs are kept consistent.
---
Nitpick comments:
In @.github/workflows/release-preparation.yml:
- Around line 133-135: The workflow currently only checks file existence for
$RELEASE_LAYOUT/releases/${VERSION}/$BUNDLE and $SHA_FILE; update it to validate
the checksum content by computing the tarball's SHA-256 and comparing it against
the contents of $SHA_FILE (or use sha256sum -c against the checksum file) so the
integrity of $BUNDLE is verified; locate the block using the RELEASE_LAYOUT,
VERSION, BUNDLE and SHA_FILE variables and replace the simple test -f check with
a checksum comparison step that exits non‑zero on mismatch.
In `@QUICKSTART.md`:
- Line 15: The download instruction in QUICKSTART.md uses a generic `<arch>`
which leads to 404s; update the text that shows the filename pattern
`aish-<version>-linux-<arch>.tar.gz` and the CDN URL
`https://cdn.aishell.ai/download/releases/<version>/` to explicitly list
supported architectures (e.g., amd64 and arm64), and add a short note that users
should use "amd64" rather than "x86_64" (or include both as accepted aliases) so
readers know which values to substitute for `<arch>`.
In `@README.md`:
- Line 84: Update the README sentence to use full CDN URLs instead of relative
paths: replace the ambiguous `download/latest` and
`download/releases/<version>/...` fragments with the explicit CDN base (matching
other docs) and mention the override environment variables
AISH_DOWNLOAD_BASE_URL, AISH_LATEST_URL and legacy AISH_REPO_URL so readers
clearly see the exact full endpoints to use; ensure the phrasing mirrors other
install docs for consistency.
🪄 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: 456b8b4c-ee77-4e8e-a994-95a14938603f
📒 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
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
crates/aish-tools/src/system_diagnose.rs (1)
240-259: Consider making allowlist tests exhaustive.These tests cover representative cases, but not every forwarded variant. A table-driven test over all currently allowlisted types would reduce regression risk when
LlmEventTypechanges.Example test structure
#[test] fn test_should_forward_diagnose_event_only_for_ui_relevant_types() { - assert!(should_forward_diagnose_event( - &LlmEventType::ToolExecutionStart - )); - assert!(should_forward_diagnose_event(&LlmEventType::Error)); - assert!(!should_forward_diagnose_event(&LlmEventType::ContentDelta)); - assert!(!should_forward_diagnose_event(&LlmEventType::OpEnd)); + let should_forward = [ + LlmEventType::ToolExecutionStart, + LlmEventType::ToolExecutionEnd, + LlmEventType::Error, + LlmEventType::ToolConfirmationRequired, + LlmEventType::InteractionRequired, + LlmEventType::Cancelled, + ]; + for ty in should_forward { + assert!(should_forward_diagnose_event(&ty)); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/aish-tools/src/system_diagnose.rs` around lines 240 - 259, Update the tests for should_forward_diagnose_event to be exhaustive/table-driven: replace the two ad-hoc tests (test_should_forward_diagnose_event_only_for_ui_relevant_types and test_should_forward_diagnose_event_rejects_generation_markers) with a single parameterized test that iterates over every LlmEventType variant, asserting true for each variant that the allowlist contains and false for all others; locate the allowlist logic in should_forward_diagnose_event and derive the expected boolean per-variant from that same source (or a small canonical list of allowed variants) so future additions to LlmEventType will fail the test if not explicitly allowed.
🤖 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-cli/src/update.rs`:
- Around line 135-143: The current validation allows non-dot numeric strings
(e.g., "release-2026-04-23") that compare_versions() cannot parse; tighten
validation so normalized only accepts dot-separated numeric parts. Replace the
logic around normalized/valid with a check that normalized.split('.').all(|part|
!part.is_empty() && part.chars().all(|ch| ch.is_ascii_digit())), and keep the
existing guard that normalized must contain at least one digit; this ensures
compare_versions() can parse the version strings used elsewhere (e.g., in
compare_versions()).
In `@packaging/scripts/publish_release.sh`:
- Around line 26-36: Current check only ensures at least one matching artifact
exists; instead iterate the discovered bundles and verify each
aish-${VERSION}-linux-*.tar.gz has a corresponding .sha256 file and fail if any
checksum is missing. Update the logic that builds ARTIFACT_FILES (and/or
introduce TAR_FILES and CHECKSUM_FILES) to collect .tar.gz entries, for each tar
verify existence of the same filename with .sha256 (using ARTIFACT_ROOT and
VERSION to construct the expected sibling name), and if any sibling is absent
print an error and exit 1 so publish_release.sh fails fast on incomplete
artifact sets.
- Around line 26-30: The find predicate is broken by the newline after the
escaped "(" which causes the shell to terminate the find command; in
publish_release.sh fix the ARTIFACT_FILES assignment so the parentheses and the
first -name are on the same command line (e.g. change find "$ARTIFACT_ROOT"
-type f \( to find "$ARTIFACT_ROOT" -type f \( -name ... ), resulting in a
single continuous find invocation that includes both -name
"aish-${VERSION}-linux-*.tar.gz" and -name
"aish-${VERSION}-linux-*.tar.gz.sha256" (or alternatively place a
line-continuation backslash at the end of the line before the newline), ensuring
ARTIFACT_FILES, ARTIFACT_ROOT and VERSION are used unchanged.
- Around line 71-85: The validate_url function is currently using $? from the if
compound so it can capture the wrong exit code; modify validate_url to run curl
and immediately store its exit status in exit_code (e.g., run curl -fsSI
--connect-timeout 10 --max-time 30 "$url" >/dev/null; exit_code=$?), then test
exit_code (if (( exit_code == 0 )); then return 0; fi) before any other
commands, and keep the existing retry logic with VALIDATION_RETRY_COUNT and
VALIDATION_RETRY_DELAY_SECS unchanged; ensure exit_code is returned on the final
failed attempt.
---
Nitpick comments:
In `@crates/aish-tools/src/system_diagnose.rs`:
- Around line 240-259: Update the tests for should_forward_diagnose_event to be
exhaustive/table-driven: replace the two ad-hoc tests
(test_should_forward_diagnose_event_only_for_ui_relevant_types and
test_should_forward_diagnose_event_rejects_generation_markers) with a single
parameterized test that iterates over every LlmEventType variant, asserting true
for each variant that the allowlist contains and false for all others; locate
the allowlist logic in should_forward_diagnose_event and derive the expected
boolean per-variant from that same source (or a small canonical list of allowed
variants) so future additions to LlmEventType will fail the test if not
explicitly allowed.
🪄 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: 74675eb4-b238-47b1-b4e6-387dd05bb108
📒 Files selected for processing (10)
.github/workflows/release-preparation.ymlQUICKSTART.mdREADME.mdcrates/aish-cli/src/update.rscrates/aish-i18n/locales/en-US.yamlcrates/aish-i18n/locales/zh-CN.yamlcrates/aish-shell/src/app.rscrates/aish-tools/src/bash.rscrates/aish-tools/src/system_diagnose.rspackaging/scripts/publish_release.sh
✅ Files skipped from review due to trivial changes (1)
- QUICKSTART.md
🚧 Files skipped from review as they are similar to previous changes (4)
- .github/workflows/release-preparation.yml
- crates/aish-i18n/locales/zh-CN.yaml
- crates/aish-tools/src/bash.rs
- README.md
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/aish-cli/src/update.rs (1)
721-793: Consider RAII guards for cleaner environment variable cleanup.The current pattern removes environment variables at the start of tests that need a clean state, which mitigates issues from failed assertions. However, if an assertion fails mid-test after setting variables, the cleanup at the end won't run.
A more robust pattern would use RAII guards that restore variables on drop, but the current approach is acceptable for test code since the mutex prevents concurrent access and subsequent tests clean up at the start.
🧹 Optional: RAII guard pattern
struct EnvGuard { key: &'static str, original: Option<String>, } impl EnvGuard { fn set(key: &'static str, value: &str) -> Self { let original = std::env::var(key).ok(); unsafe { std::env::set_var(key, value); } Self { key, original } } } impl Drop for EnvGuard { fn drop(&mut self) { unsafe { match &self.original { Some(v) => std::env::set_var(self.key, v), None => std::env::remove_var(self.key), } } } } // Usage: let _guard = env_test_lock(); let _url = EnvGuard::set("AISH_DOWNLOAD_BASE_URL", "https://cdn.example.com/download/"); assert_eq!(download_base_url(), "https://cdn.example.com/download"); // Automatic cleanup on drop🤖 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 721 - 793, Tests currently manually set and remove env vars which can leak state on assertion failure; introduce an RAII EnvGuard (with EnvGuard::set(key, val) storing original Option<String> and a Drop impl that restores or removes the var) and replace manual unsafe set/remove pairs in tests like test_download_base_url_prefers_explicit_override, test_latest_version_url_prefers_override, and test_release_download_url_uses_versioned_cdn_path with let _guard = EnvGuard::set("AISH_DOWNLOAD_BASE_URL", "...") (and similar for AISH_REPO_URL/AISH_LATEST_URL) so environment is always restored when the guard is dropped.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@crates/aish-cli/src/update.rs`:
- Around line 721-793: Tests currently manually set and remove env vars which
can leak state on assertion failure; introduce an RAII EnvGuard (with
EnvGuard::set(key, val) storing original Option<String> and a Drop impl that
restores or removes the var) and replace manual unsafe set/remove pairs in tests
like test_download_base_url_prefers_explicit_override,
test_latest_version_url_prefers_override, and
test_release_download_url_uses_versioned_cdn_path with let _guard =
EnvGuard::set("AISH_DOWNLOAD_BASE_URL", "...") (and similar for
AISH_REPO_URL/AISH_LATEST_URL) so environment is always restored when the guard
is dropped.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 32752efe-8b1f-462a-b932-44e2a3ada23a
📒 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 skipped from review due to trivial changes (2)
- CONTRIBUTING.md
- CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (9)
- packaging/build_bundle.sh
- crates/aish-shell/src/app.rs
- .github/workflows/release-preparation.yml
- QUICKSTART.md
- crates/aish-tools/src/system_diagnose.rs
- crates/aish-i18n/locales/en-US.yaml
- crates/aish-i18n/locales/zh-CN.yaml
- README.md
- crates/aish-tools/src/bash.rs
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
Validation
cargo test -p aish-tools bash::tests --lib(a later aggregate rerun stalled on the known slow/flakytest_bash_tool_exit_codePTY case after the new timeout assertions had already passed)cargo test -p aish-llm session::tests --libcargo test -p aish-shell content_delta_is_final --libcargo test -p aish-tools system_diagnose::tests --libcargo test -p aish-cli --bin aish update::testsbash -n packaging/build_bundle.sh packaging/scripts/publish_release.shAI-assisted: yes
Summary by CodeRabbit
New Features
Improvements
Documentation
Localization