speccy@1.34.84: Fix keep required dependencies#18175
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: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
bucket/speccy.json (1)
6-39: 📐 Maintainability & Code Quality | 🔵 TrivialPlease rerun the local Scoop checks before merge.
scoop config debug true scoop config gh_token <your-github-token> .\bin\checkver.ps1 -App speccy -f .\bin\formatjson.ps1 -App speccy scoop install bucket/speccy.json -a 64bit scoop install bucket/speccy.json -a 32bitContribution guide: https://github.com/ScoopInstaller/.github/blob/main/.github/CONTRIBUTING.md
Wiki: https://github.com/ScoopInstaller/Scoop/wiki/App-ManifestsAs per path instructions, "Provide clear instructions for testing the manifest locally before submission."
🤖 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 `@bucket/speccy.json` around lines 6 - 39, The manifest needs local validation before merge, so rerun the Scoop checks against the Speccy manifest using the existing package entry and verify both architectures. Use the speccy manifest workflow by running checkver, formatjson, and installation tests for 64bit and 32bit, then confirm the pre_install steps and shortcuts still resolve correctly in the bucket/speccy.json definition before submitting.Source: Path instructions
🤖 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.
Inline comments:
In `@bucket/speccy.json`:
- Around line 19-24: The DLL/Lang recovery step is too brittle in the speccy
install flow: the cpuidsdk DLL lookup can silently do nothing and the language
folder move can fail outright when the expected path is missing. Update the
recovery logic in the speccy.json extraction script so the cpuidsdk.dll move and
the Lang move both check for the source’s existence, emit a warning when
recovery is partial or missing, and continue without failing the install. Use
the existing recovery block around the Get-ChildItem/Sort-Object/Move-Item
pipeline and the Move-Item for "$dir\\temp\\Lang" to make the
warning-and-continue behavior explicit.
- Around line 14-17: The Speccy extraction step in the JSON script should
validate the EXE path before moving it, because the current
`Get-ChildItem`/`ForEach-Object` flow can silently do nothing when the installer
layout changes. Update the logic around the `Get-ChildItem`, `ForEach-Object`,
and `Move-Item` sequence to resolve the candidate from `$dir\temp`, assert that
exactly one `Speccy.exe` is found, and only then move it to the final
destination. Keep the check close to the existing move operation so the script
fails fast if the file is missing or ambiguous.
---
Nitpick comments:
In `@bucket/speccy.json`:
- Around line 6-39: The manifest needs local validation before merge, so rerun
the Scoop checks against the Speccy manifest using the existing package entry
and verify both architectures. Use the speccy manifest workflow by running
checkver, formatjson, and installation tests for 64bit and 32bit, then confirm
the pre_install steps and shortcuts still resolve correctly in the
bucket/speccy.json definition before submitting.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4a993ce1-043f-449f-893c-71d6e015e259
📒 Files selected for processing (1)
bucket/speccy.json
| "Get-ChildItem -Recurse -File -Path \"$dir\" -Name $FileName -ErrorAction Stop |", | ||
| " ForEach-Object -Process {", | ||
| " Move-Item -Path \"$dir\\$_\" -Destination \"$dir\\Speccy.exe\"", | ||
| " }", |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Validate the extracted EXE before moving it.
Get-ChildItem ... -ErrorAction Stop does not throw on zero matches here, so an installer layout change will silently skip the move and leave bin pointing at a missing Speccy.exe. Resolve the candidate from $dir\temp, assert exactly one match, then move it. Based on PR objectives, the fix is supposed to "check the file location before moving".
Suggested fix
- "Get-ChildItem -Recurse -File -Path \"$dir\" -Name $FileName -ErrorAction Stop |",
- " ForEach-Object -Process {",
- " Move-Item -Path \"$dir\\$_\" -Destination \"$dir\\Speccy.exe\"",
- " }",
+ "$exe = Get-ChildItem -Path \"$dir\\temp\" -Recurse -File -Filter $FileName",
+ "if ($exe.Count -ne 1) { throw \"Unable to locate extracted $FileName\" }",
+ "Move-Item -Path $exe[0].FullName -Destination \"$dir\\Speccy.exe\"",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "Get-ChildItem -Recurse -File -Path \"$dir\" -Name $FileName -ErrorAction Stop |", | |
| " ForEach-Object -Process {", | |
| " Move-Item -Path \"$dir\\$_\" -Destination \"$dir\\Speccy.exe\"", | |
| " }", | |
| "$exe = Get-ChildItem -Path \"$dir\\temp\" -Recurse -File -Filter $FileName", | |
| "if ($exe.Count -ne 1) { throw \"Unable to locate extracted $FileName\" }", | |
| "Move-Item -Path $exe[0].FullName -Destination \"$dir\\Speccy.exe\"", |
🤖 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 `@bucket/speccy.json` around lines 14 - 17, The Speccy extraction step in the
JSON script should validate the EXE path before moving it, because the current
`Get-ChildItem`/`ForEach-Object` flow can silently do nothing when the installer
layout changes. Update the logic around the `Get-ChildItem`, `ForEach-Object`,
and `Move-Item` sequence to resolve the candidate from `$dir\temp`, assert that
exactly one `Speccy.exe` is found, and only then move it to the final
destination. Keep the check close to the existing move operation so the script
fails fast if the file is missing or ambiguous.
| "Get-ChildItem -Path \"$dir\\temp\" -Include 'cpuidsdk*.dll' -Recurse |", | ||
| " Sort-Object -Property 'Length' -Descending:$(if($architecture -eq '64bit'){$true}else{$false}) |", | ||
| " Select-Object -First 1 |", | ||
| " ForEach-Object -Process {Move-Item -Path $_.FullName -Destination \"$dir\\cpuidsdk.dll\"}", | ||
| "# Keep language files", | ||
| "Move-Item -Path \"$dir\\temp\\Lang\" -Destination \"$dir\"", |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Guard the DLL/Lang recovery path and warn on partial extraction failures.
When no cpuidsdk*.dll is found, this becomes a silent no-op, and Move-Item "$dir\\temp\\Lang" will hard-fail if the folder is missing or nested elsewhere. That still misses the #18167 failure path where dependency recovery should warn and continue. Based on PR objectives, the failure case should "trigger warnings ... but installation should still complete."
Suggested fix
- "Get-ChildItem -Path \"$dir\\temp\" -Include 'cpuidsdk*.dll' -Recurse |",
- " Sort-Object -Property 'Length' -Descending:$(if($architecture -eq '64bit'){$true}else{$false}) |",
- " Select-Object -First 1 |",
- " ForEach-Object -Process {Move-Item -Path $_.FullName -Destination \"$dir\\cpuidsdk.dll\"}",
+ "$dll = Get-ChildItem -Path \"$dir\\temp\" -Include 'cpuidsdk*.dll' -Recurse |",
+ " Sort-Object -Property 'Length' -Descending:$(if($architecture -eq '64bit'){$true}else{$false}) |",
+ " Select-Object -First 1",
+ "if ($dll) {",
+ " Move-Item -Path $dll.FullName -Destination \"$dir\\cpuidsdk.dll\"",
+ "} else {",
+ " Write-Warning 'cpuidsdk.dll was not recovered from the installer; Speccy may have limited functionality.'",
+ "}",
"# Keep language files",
- "Move-Item -Path \"$dir\\temp\\Lang\" -Destination \"$dir\"",
+ "$lang = Get-ChildItem -Path \"$dir\\temp\" -Recurse -Directory -Filter 'Lang' | Select-Object -First 1",
+ "if ($lang) { Move-Item -Path $lang.FullName -Destination \"$dir\" }",
+ "else { Write-Warning 'Lang directory was not found in the extracted installer.' }",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "Get-ChildItem -Path \"$dir\\temp\" -Include 'cpuidsdk*.dll' -Recurse |", | |
| " Sort-Object -Property 'Length' -Descending:$(if($architecture -eq '64bit'){$true}else{$false}) |", | |
| " Select-Object -First 1 |", | |
| " ForEach-Object -Process {Move-Item -Path $_.FullName -Destination \"$dir\\cpuidsdk.dll\"}", | |
| "# Keep language files", | |
| "Move-Item -Path \"$dir\\temp\\Lang\" -Destination \"$dir\"", | |
| "$dll = Get-ChildItem -Path \"$dir\\temp\" -Include 'cpuidsdk*.dll' -Recurse |", | |
| " Sort-Object -Property 'Length' -Descending:$(if($architecture -eq '64bit'){$true}else{$false}) |", | |
| " Select-Object -First 1", | |
| "if ($dll) {", | |
| " Move-Item -Path $dll.FullName -Destination \"$dir\\cpuidsdk.dll\"", | |
| "} else {", | |
| " Write-Warning 'cpuidsdk.dll was not recovered from the installer; Speccy may have limited functionality.'", | |
| "}", | |
| "# Keep language files", | |
| "$lang = Get-ChildItem -Path \"$dir\\temp\" -Recurse -Directory -Filter 'Lang' | Select-Object -First 1", | |
| "if ($lang) { Move-Item -Path $lang.FullName -Destination \"$dir\" }", | |
| "else { Write-Warning 'Lang directory was not found in the extracted installer.' }", |
🤖 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 `@bucket/speccy.json` around lines 19 - 24, The DLL/Lang recovery step is too
brittle in the speccy install flow: the cpuidsdk DLL lookup can silently do
nothing and the language folder move can fail outright when the expected path is
missing. Update the recovery logic in the speccy.json extraction script so the
cpuidsdk.dll move and the Lang move both check for the source’s existence, emit
a warning when recovery is partial or missing, and continue without failing the
install. Use the existing recovery block around the
Get-ChildItem/Sort-Object/Move-Item pipeline and the Move-Item for
"$dir\\temp\\Lang" to make the warning-and-continue behavior explicit.
|
/verify |
|
All changes look good. Wait for review from human collaborators. speccy
|
|
/verify |
|
All changes look good. Wait for review from human collaborators. speccy
|
2 participants
Closes #18167.
<manifest-name[@version]|chore>: <general summary of the pull request>