Add cross-platform installer scripts#5
Merged
Merged
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:
📝 WalkthroughWalkthroughAdded installation documentation and two platform-specific installers ( Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Installer
participant Cargo
participant FS
participant ProcMgr
participant PathMgr
User->>Installer: invoke installer (flags: install-root / no-path-update)
Installer->>Installer: validate cargo and source crate path
Installer->>Cargo: cargo install --path crates/tempyr-cli --root ... --locked --force --bin tempyr
alt install succeeds
Cargo->>FS: write binary to <install_root>/bin/tempyr(.exe)
Cargo-->>Installer: exit 0
Installer->>PathMgr: optionally add bin to user PATH (rc files or registry)
PathMgr-->>Installer: PATH updated / already present
Installer-->>User: success
else install fails with lock/access error
Cargo-->>Installer: non-zero + output indicates lock/access denied
Installer->>ProcMgr: enumerate running processes and canonicalize exec paths
ProcMgr-->>Installer: matching PID(s) for target executable
Installer->>ProcMgr: terminate matching process(es) (SIGTERM / taskkill, escalate to SIGKILL)
ProcMgr-->>Installer: processes stopped
Installer->>Cargo: retry cargo install once
Cargo->>FS: write binary to install root/bin
Cargo-->>Installer: exit 0
Installer->>PathMgr: optionally add bin to user PATH
Installer-->>User: success after retry
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/install.md`:
- Around line 13-19: Add documentation describing the opt-out flags for PATH
mutation: mention the installer flags --no-path-update (for install.sh) and
-NoPathUpdate (for install.ps1) and show a short example for each (e.g., bash
install.sh --no-path-update and PowerShell .\install.ps1 -NoPathUpdate). Update
the sections that explain how the installers idempotently update PATH (the Unix
and Windows install paragraphs) to include a one-line note and the example
commands so users in managed environments can discover the non-mutating install
mode; reference the install scripts by name (install.sh, install.ps1) and the
flags (--no-path-update, -NoPathUpdate).
In `@install.ps1`:
- Around line 85-100: The current shutdown routine stops processes with
Stop-Process and returns $true without confirming termination; re-query the
process set by calling Get-TargetProcessIds -BinaryPath $BinaryPath after the
Stop-Process/Wait-Process loop and only return $true if that re-check returns an
empty array, otherwise return $false so the caller's retry logic won't proceed
while a matching tempyr.exe is still running; update the block containing
$processIds, Stop-Process, the foreach Wait-Process loop, and the final return
to perform this final verification using Get-TargetProcessIds.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 17086604-cfe4-4815-bea2-ca8a2fc7e2bd
📒 Files selected for processing (3)
docs/install.mdinstall.ps1install.sh
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@install.ps1`:
- Line 58: The variable $matches is shadowing PowerShell's automatic $matches;
rename the local variable created with New-Object
System.Collections.Generic.List[int] (e.g., to $matchList or $customMatches) and
update all references where it's used (the subsequent usages around the current
references on lines 72 and 76) to the new name to avoid clobbering the automatic
regex results; ensure the variable name is consistent where it's initialized and
where items are added/iterated.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4e9e8092-80e1-4cf8-97b1-27383ddc8dc9
📒 Files selected for processing (2)
docs/install.mdinstall.ps1
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@install.ps1`:
- Around line 240-243: The thrown error currently discards cargo's captured
output; update the throw sites to include the combined stderr/stdout payload
(the $output variable and/or $installResult.Output) so the error message
contains both the exit code and the captured output; locate the places building
and returning the PSCustomObject (ExitCode and Output) and the subsequent throw
statements (including the similar case around lines 276-277) and concatenate or
interpolate $installResult.Output into the thrown exception message to surface
cargo logs in CI failures.
- Around line 161-195: The current PATH dedupe only checks the user PATH and
treats any malformed segment as fatal; change the logic around
Normalize-PathEntry and the comparison so you build the comparison set from both
persisted scopes ([Environment]::GetEnvironmentVariable("Path","User") and
("Machine")), iterate entries and attempt Normalize-PathEntry inside a safe
check (skip entries that return null/empty or throw), and compare
normalizedEntry to $normalizedTarget using OrdinalIgnoreCase to avoid duplicates
even if the match exists in machine scope; only set the user env and prepend
$env:Path when no matching normalized entry was found, and keep
Broadcast-EnvironmentChange as-is. Ensure references: Normalize-PathEntry,
$userPath, $env:Path, [Environment]::GetEnvironmentVariable("Path","Machine"),
$updatedPath, Broadcast-EnvironmentChange.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@install.ps1`:
- Around line 329-333: The message after Ensure-UserPathContains() uses
$addedToPath and currently says "user PATH" even when the directory was found
only in machine scope; update the Write-Host messaging that references $binDir
and $addedToPath to use a neutral phrase like "already present in PATH" (or
similar) so it accurately covers both user- and machine-scope matches—locate the
block that checks $NoPathUpdate and $addedToPath and change the "Added $binDir
to the user PATH." / "$binDir is already present in the user PATH." strings
accordingly.
- Around line 254-268: Start-Process is building a single command line so
unquoted path values like $CratePath and $InstallRootPath will break when they
contain spaces; fix by ensuring those path arguments include literal escaped
double quotes in the ArgumentList (e.g. wrap the $CratePath and $InstallRootPath
strings with escaped quotes) when constructing the -ArgumentList for
Start-Process (the block that builds the "install", "--path", $CratePath,
"--root", $InstallRootPath, ... arguments); alternatively, replace this
Start-Process invocation with a direct invocation using the call operator (&
$cargoExe ...) which preserves argument boundaries — update the code that
creates $process (Start-Process and its -ArgumentList) to use one of these two
approaches so path arguments are correctly quoted.
- Around line 37-48: The try/catch around [System.IO.File]::Open($Path, ...
[System.IO.FileShare]::None) only catches System.IO.IOException; update the
error handling so UnauthorizedAccessException is caught and treated as a
permission failure (return $false) while IOException still indicates a lock
(return $true). Specifically, add a catch for System.UnauthorizedAccessException
for the Open call that returns $false, keep or add the catch for
System.IO.IOException returning $true, and ensure the catch order/clauses (the
try block using $Path and FileShare.None) reflect this behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@install.ps1`:
- Around line 102-127: Rename the unapproved verbs in the functions: change
Normalize-PathEntry to an approved verb name such as
ConvertTo-CanonicalPathEntry or Resolve-PathEntry and rename
Try-Normalize-PathEntry accordingly (e.g., Try-ConvertTo-CanonicalPathEntry or
Try-Resolve-PathEntry); update all references/call sites to use the new names
and keep the same parameter signature and behavior (Normalize-PathEntry,
Try-Normalize-PathEntry, Resolve-CanonicalPath and Resolve-CanonicalPath -Path
usage in the body remain unchanged), so PSScriptAnalyzer won't flag the verbs
while preserving functionality.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
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
tempyrfromcrates/tempyr-clitempyrprocesses whose executable path exactly matches the installed binary before retrying the installValidation
install.ps1successfully with PowerShell's parserinstall.ps1with a fresh install roottempyr.exe --mcpprocess was holding the installed binary, confirming the installer kills only the exact installed binary path and retries successfullyNotes
Summary by CodeRabbit
Documentation
New Features