openssh: Add helper scripts to fix service binPath and autostart#7666
Open
SwingURM wants to merge 3 commits intoScoopInstaller:masterfrom
Open
openssh: Add helper scripts to fix service binPath and autostart#7666SwingURM wants to merge 3 commits intoScoopInstaller:masterfrom
SwingURM wants to merge 3 commits intoScoopInstaller:masterfrom
Conversation
Contributor
|
All changes look good. Wait for review from human collaborators. openssh
|
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a post-install step to the OpenSSH manifest that resolves the real installation path (following the Changes
Sequence Diagram(s)sequenceDiagram
participant Installer as Installer (Scoop)
participant PostInstall as post_install script
participant FS as Filesystem (symlink resolution)
participant FixPS as fix.ps1
participant SCM as Service Control Manager (registry)
participant AutostartPS as autostart-sshd.ps1
Installer->>PostInstall: execute post_install commands
PostInstall->>FS: resolve real directory for `current` symlink
PostInstall->>FixPS: write fix.ps1 with absolute binPath targets
FixPS->>SCM: update `sshd` and `ssh-agent` service `ImagePath`/binPath
PostInstall->>AutostartPS: write autostart-sshd.ps1
AutostartPS->>SCM: set `sshd` and `ssh-agent` StartupType = Automatic
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 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 (1)
bucket/openssh.json (1)
47-47: Note the re-run requirement afterscoop updatein the notes.
fix.ps1embeds the resolved absolute versioned path at install time (e.g.,...\openssh\10.0.0.0p2\sshd.exe). Scoop runspost_installon updates as well, expanding$dirto thecurrentjunction path, so each update regeneratesfix.ps1with the new version's path — but the service registry entry is not automatically updated. Users must re-runsudo fix.ps1after everyscoop update opensshfor the service to point to the new binary.Consider expanding the note to reflect this:
📝 Proposed note update
- "Run 'sudo $dir\\fix.ps1' to fix service binPath to absolute path.", + "Run 'sudo $dir\\fix.ps1' to fix service binPath to absolute path. Re-run after every 'scoop update openssh'.",Also applies to: 54-54
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bucket/openssh.json` at line 47, The post_install script writes fix.ps1 embedding the resolved versioned path (via Set-Content writing to "$dir\\fix.ps1"), but Scoop re-runs post_install on updates using the current junction path and does not update existing Windows service registry entries, so users must re-run sudo fix.ps1 after every scoop update openssh to repoint services to the new binary; update the notes referenced (and the duplicate at the second occurrence) to explicitly state that after running scoop update openssh the user must re-run sudo fix.ps1 (or re-create the sshd/ssh-agent service entries) to update the service binPath to the new version.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bucket/openssh.json`:
- Line 45: The post_install line using [IO.Directory]::ResolveLinkTarget($dir,
$true).FullName is a .NET 6+ API and will throw on Windows PowerShell 5.1;
replace that call with the PowerShell 5.0+ compatible approach using (Get-Item
$dir -Force).Target and keep the existing fallback logic (i.e., assign the
target to $real and if it's null/empty set $real = $dir) so junction targets are
resolved when present but the script continues when not a link; update the
expression that sets $real to use the (Get-Item ...).Target lookup and preserve
the subsequent if (!$real) { $real = $dir } behavior.
---
Nitpick comments:
In `@bucket/openssh.json`:
- Line 47: The post_install script writes fix.ps1 embedding the resolved
versioned path (via Set-Content writing to "$dir\\fix.ps1"), but Scoop re-runs
post_install on updates using the current junction path and does not update
existing Windows service registry entries, so users must re-run sudo fix.ps1
after every scoop update openssh to repoint services to the new binary; update
the notes referenced (and the duplicate at the second occurrence) to explicitly
state that after running scoop update openssh the user must re-run sudo fix.ps1
(or re-create the sshd/ssh-agent service entries) to update the service binPath
to the new version.
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
bucket/openssh.json (1)
44-45:(Get-Item $dir -Force).Targetaddresses the previous PS 5.1 compatibility concern.The
.ResolveLinkTargetissue from the prior review is resolved. Two small notes:
.Targetresolves only one level of indirection, unlike the earlierResolveLinkTarget($dir, $true)(recursive). For Scoop's single-layercurrentjunction this is sufficient.Get-Item $dirtreats the path as a glob pattern, so a Scoop root containing[,], or*characters would silently fail to match.-LiteralPathavoids this:🔧 Optional: use
-LiteralPath- "$real = (Get-Item $dir -Force).Target; if (!$real) { $real = $dir }", + "$real = (Get-Item -LiteralPath $dir -Force).Target; if (!$real) { $real = $dir }",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bucket/openssh.json` around lines 44 - 45, The current post_install snippet uses (Get-Item $dir -Force).Target which works but treats $dir as a glob; update the Get-Item call to use -LiteralPath to avoid accidental glob expansion (i.e., replace Get-Item $dir -Force with Get-Item -LiteralPath $dir -Force) while keeping the existing $real fallback logic (post_install, $real, and the !$real check) intact; note that .Target is single-level resolution which is acceptable for Scoop's single-layer junction usage.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bucket/openssh.json`:
- Around line 46-47: The generated fix.ps1 currently uses a $q helper and writes
sc.exe config lines that lose their surrounding double-quotes when PowerShell
evaluates them; remove the "$q = [char]34" helper and change the two sc.exe
strings written by Set-Content (the sc.exe config sshd binPath= ... and sc.exe
config ssh-agent binPath= ... entries) to use a parenthesised single-quoted
expression that includes literal double-quote characters around the real path
(e.g. use ('"'+ <real path> + '"' ) style so the resulting ImagePath in the
registry includes the quote characters and preserves paths with spaces).
---
Nitpick comments:
In `@bucket/openssh.json`:
- Around line 44-45: The current post_install snippet uses (Get-Item $dir
-Force).Target which works but treats $dir as a glob; update the Get-Item call
to use -LiteralPath to avoid accidental glob expansion (i.e., replace Get-Item
$dir -Force with Get-Item -LiteralPath $dir -Force) while keeping the existing
$real fallback logic (post_install, $real, and the !$real check) intact; note
that .Target is single-level resolution which is acceptable for Scoop's
single-layer junction usage.
✅ Actions performedReview triggered.
|
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.
Closes #7665
<manifest-name[@version]|chore>: <general summary of the pull request>Add
post_installscripts that are generated on install:sshdandssh-agentservicebinPathto the real absolute path.sshdandssh-agentservices toAutomaticstartup type.Summary by CodeRabbit
New Features
Chores