fix(install.sh): dry-run exits 0 when platform asset missing#877
Conversation
Fixes two bugs in the dry-run handler in `scripts/install.sh`: - `resolve_from_release_api` call → wrapped in `if ... then rc=0 else rc=$? fi` so `set -euo pipefail` no longer aborts before `resolve_rc=$?` captures rc=2 - `CI=true` / `GITHUB_ACTIONS=true` override → removed (re-exited 1 against the dry-run contract) - dry-run rc ≠ 0 branch → warn + exit 0 with platform-aware diagnostic - real-install rc ≠ 0 branch → still exit 1, now with a `releases/latest` pointer line Verified via shimmed `curl`/`uname`: linux/x86_64 dry-run = exit 0 + warn (was exit 1); real install = exit 1; CI env dry-run = exit 0.
Touches `.github/workflows/installer-smoke.yml`: - `matrix.os` → `[macos-latest, ubuntu-22.04]` - comment → references tinyhumansai#785 instead of "temporarily disabled — no Linux release asset published yet" Safe now that `install.sh --dry-run` warns + exits 0 when no Linux asset is published; the Linux lane covers future regressions on every PR.
New `scripts/validate-release-assets.sh`: - input → `release.json` (GitHub API shape) + `latest.json` (Tauri updater manifest) - supported matrix → `darwin-aarch64`, `darwin-x86_64`, `linux-x86_64`, `windows-x86_64` - asset patterns → `aarch64.app.tar.gz|aarch64.dmg`, `x64.app.tar.gz|x64.dmg`, `.AppImage`, `x64.msi|x64-setup.exe` - exit 0 → every supported platform present in both sources - exit 1 → any platform missing, with `Missing from latest.json` / `Missing release assets` diagnostics on stderr - exit 2 → misuse (bad argv) or unparseable JSON No `release.yml` wiring in this PR — that is a maintainer call on whether missing Linux should block a release or stay informational. Against the current `v0.52.26`: reports `Missing from latest.json: linux-x86_64` + `Missing release assets: linux-x86_64`.
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 28 minutes and 6 seconds. ⌛ 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: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe changes introduce a new asset validation script, modify the installer to gracefully handle missing platform artifacts in dry-run mode (exit 0 with warnings) while failing real installs, and expand the smoke test workflow to include Linux platform testing. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![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
🧹 Nitpick comments (3)
scripts/install.sh (1)
320-322: Minor: "Could not reach release metadata (rc=1)" is slightly misleading.
resolve_from_release_apireturns1for three distinct conditions:curlfailure,python3missing (line 194-197), andpython3parse failure (line 249|| return 1). The "Could not reach" wording only fits the first. Consider generalizing the message, since this only affects the dry-run diagnostic output:✏️ Proposed tweak
*) - log_warn "Could not reach release metadata (rc=${resolve_rc}) for ${OS}/${ARCH}." + log_warn "Could not resolve release metadata (rc=${resolve_rc}) for ${OS}/${ARCH}." ;;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/install.sh` around lines 320 - 322, The log message in the default case for resolve_from_release_api is misleading because resolve_rc can be 1 for curl failure, missing python3, or parse failure; update the warning emitted by log_warn (where resolve_rc is used) to be more general (e.g., "Could not obtain or parse release metadata (rc=${resolve_rc}) for ${OS}/${ARCH}") so it accurately covers all failure modes; locate the switch/default handling that uses resolve_rc and replace the "Could not reach release metadata" text with a generalized message referencing resolve_rc, OS and ARCH.scripts/validate-release-assets.sh (2)
67-72: Consider also validating thatlatest.jsonitself is published as a release asset.The inline release-pipeline check referenced in the PR context (
.github/workflows/release.ymllines ~876-904) treats/^latest\.json$/as a required asset because "without this, installed clients can't discover new releases via plugins.updater.endpoints." This validator acceptslatest.jsonas input but doesn't assert it is present inrelease.assets[], so a release that forgets to upload the manifest would silently pass here (as long as the local file you feed in is valid).Low priority since release.yml already catches this, but including it would make the script a complete standalone regression guard as described in the header.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/validate-release-assets.sh` around lines 67 - 72, The validator currently defines ASSET_PATTERNS but doesn't assert that the release actually includes the literal "latest.json" asset; update the script to explicitly check the release asset list for an entry named "latest.json" (in addition to existing pattern checks). Locate the ASSET_PATTERNS variable and the function or block that iterates over release.assets (the code that validates assets) and add a guard that fails if no asset with the exact name "latest.json" is present, returning a non-zero exit code and a clear error message.
75-80: Nit: usewith open(...)for JSON loads.Minor hygiene — avoids leaked file handles and is the idiomatic pattern, matching how the similar blocks in
install.sh(lines 169, 203, 282) open files.♻️ Proposed tweak
try: - release = json.load(open(release_path)) - latest = json.load(open(latest_path)) + with open(release_path, "r", encoding="utf-8") as f: + release = json.load(f) + with open(latest_path, "r", encoding="utf-8") as f: + latest = json.load(f) except json.JSONDecodeError as e: print(f"validate-release-assets: invalid JSON: {e}", file=sys.stderr) sys.exit(2)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/validate-release-assets.sh` around lines 75 - 80, Replace the direct json.load(open(...)) calls with context managers to avoid leaking file handles: open release_path and latest_path using "with open(release_path) as f:" / "with open(latest_path) as f:" and call json.load on the file objects to assign to release and latest respectively, keeping the existing try/except json.JSONDecodeError handling and the same error message/exit behavior; locate the two json.load(open(...)) usages for variables release and latest to make this change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/validate-release-assets.sh`:
- Around line 82-89: The manifest generator is missing logic to resolve the
linux-x86_64 AppImage so add a branch in publish-updater-manifest.sh alongside
the existing darwin-aarch64, darwin-x86_64, and windows-x86_64 handling: call
find_asset to locate the linux AppImage (matching the documented AppImage naming
convention) and then call add_platform with the key "linux-x86_64" and the found
asset name; mirror the pattern used for the other platforms (the functions
find_asset and add_platform) so linux-x86_64 is included in latest.json.
---
Nitpick comments:
In `@scripts/install.sh`:
- Around line 320-322: The log message in the default case for
resolve_from_release_api is misleading because resolve_rc can be 1 for curl
failure, missing python3, or parse failure; update the warning emitted by
log_warn (where resolve_rc is used) to be more general (e.g., "Could not obtain
or parse release metadata (rc=${resolve_rc}) for ${OS}/${ARCH}") so it
accurately covers all failure modes; locate the switch/default handling that
uses resolve_rc and replace the "Could not reach release metadata" text with a
generalized message referencing resolve_rc, OS and ARCH.
In `@scripts/validate-release-assets.sh`:
- Around line 67-72: The validator currently defines ASSET_PATTERNS but doesn't
assert that the release actually includes the literal "latest.json" asset;
update the script to explicitly check the release asset list for an entry named
"latest.json" (in addition to existing pattern checks). Locate the
ASSET_PATTERNS variable and the function or block that iterates over
release.assets (the code that validates assets) and add a guard that fails if no
asset with the exact name "latest.json" is present, returning a non-zero exit
code and a clear error message.
- Around line 75-80: Replace the direct json.load(open(...)) calls with context
managers to avoid leaking file handles: open release_path and latest_path using
"with open(release_path) as f:" / "with open(latest_path) as f:" and call
json.load on the file objects to assign to release and latest respectively,
keeping the existing try/except json.JSONDecodeError handling and the same error
message/exit behavior; locate the two json.load(open(...)) usages for variables
release and latest to make this change.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4be878e7-3757-41db-ba29-021339b839e1
📒 Files selected for processing (3)
.github/workflows/installer-smoke.ymlscripts/install.shscripts/validate-release-assets.sh
…sent Addresses CodeRabbit primary finding on tinyhumansai#877. Touches `scripts/release/publish-updater-manifest.sh`: - `LIN_X86_64` → `find_asset "^OpenHuman(_| ).*amd64\.AppImage(\.tar\.gz)?$"` matching the AppImage naming convention already documented in the script header (line 94) - `[updater] Resolved updater bundles` → adds `linux-x86_64 = <missing>` log line alongside darwin/windows - `add_platform "linux-x86_64" "$LIN_X86_64"` → slotted between darwin-x86_64 and windows-x86_64 in the platform list Safe-by-default: `add_platform` early-returns on empty `name` (`[ -z "$name" ] && return 0`, line 119). Current releases ship no AppImage → `LIN_X86_64` stays empty → linux-x86_64 key absent from manifest, same as today. Once a Linux AppImage is published, the entry wires automatically without a follow-up change.
…acks Addresses CodeRabbit secondary note on tinyhumansai#877. Touches `scripts/validate-release-assets.sh`: - new `_has_platform(key)` helper → accepts bare `key`, `f"{key}-appimage"`, or `f"{key}-app"` variants - `missing_latest` → uses `_has_platform` instead of the bare `key in latest_platforms` membership test Mirrors the three-step fallback `scripts/install.sh` already uses when looking up `latest.json` platforms (line 173). Without this the validator false-fails a release that correctly ships under `linux-x86_64-appimage` or `linux-x86_64-app`. Verified locally: real `v0.52.26` still fails with `Missing from latest.json: linux-x86_64` (linux genuinely missing); synthetic release with `linux-x86_64-appimage` key now passes (rc=0).
|
@obchain brother, so happy to see you again :D really great to see you contribute to the project. there are some build/typecheck issues. just resolve those and clear to merge. |
|
actually nvm its from some other code issue.. merging this now. |
|
Thanks Brother @senamakel , Never Late to contribute to something great 😄 |
2 participants
Summary
install.sh --dry-runon linux/x86_64 no longer hard-fails when thelatest release ships without a Linux asset. Two stacked bugs fixed in
the dry-run handler (see below); real installs still exit 1.
installer-smoke.ymlmatrix re-enablesubuntu-22.04so the Linuxdry-run path is covered on every PR.
scripts/validate-release-assets.sh— standalone regression guardthat checks a release exposes every supported platform on both
latest.jsonand the release assets list. Runnable locally;release.ymlwiring left as a maintainer call.Problem
bash scripts/install.sh --dry-run --verboseon linux/x86_64 exits 1with
x Could not resolve a compatible asset for linux/x86_64wheneverthe latest release lacks a Linux artifact. The documented install path
is broken for Linux users on those releases, and the
installer-smokematrix has hidden
ubuntu-22.04behind atemporarily disabledcommentfor the same reason.
Two bugs stack:
resolve_from_release_apireturns rc=2 on "no compatible asset",but the bare call under
set -euo pipefailaborts the script onmost bash builds before
resolve_rc=$?can capture it. Thededicated dry-run handler below was unreachable.
CI=true/GITHUB_ACTIONS=truewith the comment "Preserve failure signalfor automation." — directly contradicting the dry-run contract.
Solution
resolve_from_release_apiinif resolve_from_release_api; then resolve_rc=0; else resolve_rc=$?; fiso the rc is always read.
CI/GITHUB_ACTIONSoverride. Any resolver failure under--dry-runnow warns + exits 0 with a platform-aware diagnostic.Real installs still hard-fail with a
releases/latestpointer.ubuntu-22.04back in theinstaller-smokematrix now that theLinux dry-run path is safe.
scripts/validate-release-assets.shas a standalone regressionguard so maintainers can verify locally (or in a follow-up CI step)
that a release covers every supported platform — closing the gap that
let a Linux-less release ship in the first place.
Submission Checklist
PATH-shimmedcurl/unameharness:- linux dry-run + no asset → exit 0 + warn
- linux real install + no asset → exit 1 + error
-
CI=true GITHUB_ACTIONS=truedry-run → exit 0 (was exit 1)- macOS dry-run happy path → exit 0 (unchanged)
ubuntu-22.04re-enabled ininstaller-smoke; fix is now covered on every PR.scripts/validate-release-assets.shagainst
v0.52.26correctly reportsMissing from latest.json: linux-x86_64andMissing release assets: linux-x86_64; full-release syntheticfixture exits 0.
if-guard inline in thescript; header on the validator explains inputs, exit codes, and
the supported-platform source-of-truth.
contract reasoning lives.
Impact
install.sh --dry-runbecomes exit 0 onno-asset. Real installs unchanged. macOS and Windows paths unchanged.
installer-smokenow runs on both macOS and Ubuntu; fail-fastis already disabled so one lane going red doesn't mask the other.
release.yml/release-packages.yml— integration ofvalidate-release-assets.shinto the publish gate is intentionally left as a follow-up (needs a
maintainer call on whether missing Linux should block a release or
stay informational).
Related
Closes #785
Summary by CodeRabbit
Tests
Bug Fixes
Chores