fix(installer): propagate Windows install failures - #5455
fix(installer): propagate Windows install failures#5455NgoQuocViet2001 wants to merge 6 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe installer now uses centralized exit-code assertions and terminating errors for validation and installation failures. Windows installation tests verify quoted MSI paths, MSI and EXE results, invalid channels, release API failures, version handling, and unsupported operating systems. ChangesInstaller error propagation
Estimated code review effort: 3 (Moderate) | ~20 minutes Poem Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ce4e78ccf5
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/install.ps1 (1)
246-249: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDo not report successful MSI reboot codes as failures.
Lines [246-249] emit the failure warning for every nonzero MSI exit code. Exit codes
1641and3010are accepted as successful outcomes byAssert-OpenHumanInstallerProcessSucceeded, so these outcomes still display failure guidance.Emit the warning only for exit codes outside the accepted MSI success set.
Proposed fix
- if ($proc.ExitCode -ne 0) { + if ($proc.ExitCode -notin @(0, 1641, 3010)) { Write-WarnMsg "If this persists, capture a log: msiexec /i `"$tmpFile`" /l*v `"$env:TEMP\OpenHuman-msi.log`"" }🤖 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 `@scripts/install.ps1` around lines 246 - 249, Update the warning condition around Assert-OpenHumanInstallerProcessSucceeded so it excludes MSI exit codes 1641 and 3010, emitting failure guidance only for exit codes outside the accepted success set while preserving the existing assertion call.
🧹 Nitpick comments (1)
scripts/tests/OpenHumanWindowsInstall.Tests.ps1 (1)
130-149: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftAdd coverage for the remaining terminating installer paths.
The existing tests cover the shared installer-process helper, invalid channel, version output, and unsupported OS. Add isolated tests for unsupported architecture, missing release artifact, SHA256 mismatch, unsupported installer type, and MSI/EXE failure responses, each asserting the expected terminating error.
🤖 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 `@scripts/tests/OpenHumanWindowsInstall.Tests.ps1` around lines 130 - 149, Extend the installer test coverage with isolated terminating-error assertions for unsupported architecture, missing release artifact, SHA256 mismatch, unsupported installer type, and MSI/EXE failure responses. Follow the existing Assert-Throws patterns in OpenHumanWindowsInstall.Tests.ps1, invoke the relevant installer paths with controlled inputs, and assert each expected error message without altering the current helper, channel, version, or OS tests.
🤖 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.
Outside diff comments:
In `@scripts/install.ps1`:
- Around line 246-249: Update the warning condition around
Assert-OpenHumanInstallerProcessSucceeded so it excludes MSI exit codes 1641 and
3010, emitting failure guidance only for exit codes outside the accepted success
set while preserving the existing assertion call.
---
Nitpick comments:
In `@scripts/tests/OpenHumanWindowsInstall.Tests.ps1`:
- Around line 130-149: Extend the installer test coverage with isolated
terminating-error assertions for unsupported architecture, missing release
artifact, SHA256 mismatch, unsupported installer type, and MSI/EXE failure
responses. Follow the existing Assert-Throws patterns in
OpenHumanWindowsInstall.Tests.ps1, invoke the relevant installer paths with
controlled inputs, and assert each expected error message without altering the
current helper, channel, version, or OS tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: da5b7ca9-24c8-4635-aa09-36e528d7ef86
📒 Files selected for processing (2)
scripts/install.ps1scripts/tests/OpenHumanWindowsInstall.Tests.ps1
|
Addressed the outside-diff MSI warning finding in 1cccb16: diagnostic guidance is now emitted only for exit codes outside 0, 1641, and 3010. The focused PowerShell suite still passes all 25 checks. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/install.ps1 (1)
178-194: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPreserve release API failures in the terminating error.
When
Invoke-RestMethodfails in Lines 178-191, the catch writes a warning and leaves$assetUrlempty. Line 194 then reports “No Windows x64 installer artifact found”, which is incorrect for network, authentication, or rate-limit failures. Rethrow the API error, or include it in the final exception, and reserve the missing-artifact message for a successful release lookup with no matching asset.Proposed fix
} catch { - Write-WarnMsg "Could not query release API: $($_.Exception.Message)" + throw "Could not query release API: $($_.Exception.Message)" }🤖 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 `@scripts/install.ps1` around lines 178 - 194, Update the release lookup flow around Invoke-RestMethod and the catch block so API failures propagate as terminating errors or are included in the final exception, rather than falling through with an empty $assetUrl. Keep the “No Windows x64 installer artifact found” error only for successful release lookups where Select-OpenHumanWindowsAssetFromRelease returns no matching asset.
🤖 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.
Outside diff comments:
In `@scripts/install.ps1`:
- Around line 178-194: Update the release lookup flow around Invoke-RestMethod
and the catch block so API failures propagate as terminating errors or are
included in the final exception, rather than falling through with an empty
$assetUrl. Keep the “No Windows x64 installer artifact found” error only for
successful release lookups where Select-OpenHumanWindowsAssetFromRelease returns
no matching asset.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a0d39774-2131-48c1-a40d-ae9e09e8ead3
📒 Files selected for processing (1)
scripts/install.ps1
|
Addressed the release-lookup finding in 7ce63ac: API failures now terminate with their original cause instead of falling through to the missing-artifact message. Added a regression assertion; the focused suite passes all 26 checks. |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
scripts/tests/OpenHumanWindowsInstall.Tests.ps1 (1)
130-140: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftAdd coverage for the new installer failure paths.
Lines 131 through 136 test
Assert-OpenHumanInstallerProcessSucceededdirectly. They do not execute the MSI and EXE branches inInstall-OpenHuman. The suite also does not exercise the unsupported-architecture, missing-artifact, or SHA256-mismatch throws. A wiring regression could therefore pass all 26 checks. Add focused tests with scoped command doubles for process, download, release, and hash operations.🤖 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 `@scripts/tests/OpenHumanWindowsInstall.Tests.ps1` around lines 130 - 140, Extend the installer test suite around Install-OpenHuman to cover its MSI and EXE execution branches using scoped doubles for process, download, release, and hash operations. Add focused assertions that unsupported architectures, missing artifacts, and SHA256 mismatches throw the expected errors, while preserving the existing direct Assert-OpenHumanInstallerProcessSucceeded checks and version/argument tests.scripts/install.ps1 (1)
246-249: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse the helper’s MSI success-code policy.
The accepted MSI codes are defined again here as
@(0, 1641, 3010), whileAssert-OpenHumanInstallerProcessSucceededdefines them at Line 65. Use one shared predicate or constant for both the diagnostic warning and the terminating check. Otherwise, future changes can make the warning behavior diverge from the failure behavior.🤖 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 `@scripts/install.ps1` around lines 246 - 249, Update the MSI handling around Assert-OpenHumanInstallerProcessSucceeded to reuse the helper’s existing accepted-exit-code policy for the warning condition instead of duplicating @(0, 1641, 3010). Use a shared predicate or constant so the diagnostic warning and terminating check always remain consistent.
🤖 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.
Nitpick comments:
In `@scripts/install.ps1`:
- Around line 246-249: Update the MSI handling around
Assert-OpenHumanInstallerProcessSucceeded to reuse the helper’s existing
accepted-exit-code policy for the warning condition instead of duplicating @(0,
1641, 3010). Use a shared predicate or constant so the diagnostic warning and
terminating check always remain consistent.
In `@scripts/tests/OpenHumanWindowsInstall.Tests.ps1`:
- Around line 130-140: Extend the installer test suite around Install-OpenHuman
to cover its MSI and EXE execution branches using scoped doubles for process,
download, release, and hash operations. Add focused assertions that unsupported
architectures, missing artifacts, and SHA256 mismatches throw the expected
errors, while preserving the existing direct
Assert-OpenHumanInstallerProcessSucceeded checks and version/argument tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: bdab39cd-66fc-41ae-a409-b7c6e6844061
📒 Files selected for processing (2)
scripts/install.ps1scripts/tests/OpenHumanWindowsInstall.Tests.ps1
|
Addressed the duplicated MSI success-policy nitpick in f9429ec: warning and assertion paths now call the same predicate. The focused suite remains green at 26 checks. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
scripts/install.ps1 (2)
32-35: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winQuote
$MsiPathbefore passing it toStart-Process.
Start-Process -ArgumentListjoins the array on spaces, andmsiexecrequires paths with spaces to be wrapped in double quotes. With a temporary MSI inC:\Temp\Test User\..., this can split the installer path and fail the install.Proposed fix
- return @('/i', $MsiPath, '/qn', '/norestart') + return @('/i', ('"{0}"' -f $MsiPath), '/qn', '/norestart')🤖 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 `@scripts/install.ps1` around lines 32 - 35, Update the ArgumentList construction for $MsiPath before it is passed to Start-Process so the MSI path is wrapped in double quotes, preserving paths containing spaces when msiexec receives the joined arguments. Keep the existing argument order and other tokens unchanged.
202-204: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPreserve the API failure error record.
Line 203 wraps
Invoke-RestMethodfailures in a new exception and drops the original terminating error record. Use barethrowfrom thiscatchblock to keep the original API failure cause.Proposed fix
catch { - throw "Could not query release API: $($_.Exception.Message)" + throw }🤖 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 `@scripts/install.ps1` around lines 202 - 204, Update the catch block around the release API request to use a bare throw instead of creating a new string exception, preserving the original Invoke-RestMethod error record and failure cause.
🤖 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.
Outside diff comments:
In `@scripts/install.ps1`:
- Around line 32-35: Update the ArgumentList construction for $MsiPath before it
is passed to Start-Process so the MSI path is wrapped in double quotes,
preserving paths containing spaces when msiexec receives the joined arguments.
Keep the existing argument order and other tokens unchanged.
- Around line 202-204: Update the catch block around the release API request to
use a bare throw instead of creating a new string exception, preserving the
original Invoke-RestMethod error record and failure cause.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d5ba0002-039a-4881-9120-e83f196264cd
📒 Files selected for processing (1)
scripts/install.ps1
|
Addressed both outside-diff findings in 5d2affb: MSI paths are explicitly quoted before |
5d2affb to
c5cddd2
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
1 participant
Summary
Start-Processjoins the argument list, preserving paths that contain spaces.Problem
The public Windows installer handled unsupported inputs, missing or invalid artifacts, checksum mismatches, and failed installer processes with Write-Host followed by return. In the documented irm | iex flow, those paths could finish without a terminating error even though OpenHuman was not installed, so callers and automation could mistake failure for success.
Solution
Replace terminal error-and-return branches with terminating errors. A focused helper validates MSI and EXE process exit codes, accepts MSI success-with-reboot outcomes (1641 and 3010), and keeps the MSI logging guidance before real errors propagate. The existing PowerShell suite now pins both successful behavior and representative failure paths, including execution through Invoke-Expression.
Submission Checklist
> If a section does not apply to this change, mark the item as N/A with a one-line reason. Do not delete items.
Impact
Windows public installer only. Successful help, version, dry-run, MSI, and EXE flows remain unchanged, while MSI paths containing spaces are now preserved. Failed installs surface a terminating PowerShell error, allowing scripts and CI callers to detect the failure.
Related
AI Authored PR Metadata (required for Codex/Linear PRs)
> Keep this section for AI-authored PRs. For human-only PRs, mark each field N/A.
Linear Issue
Commit & Branch
Validation Run
Validation Blocked
Behavior Changes
Parity Contract
Duplicate / Superseded PR Handling
pnpm dev:app:winwork on default Git for Windows installs #5342 is an unrelated dev launcher change and does not touch install.ps1.Summary by CodeRabbit
Bug Fixes
Tests