feat(install): Add support for historical manifests by git history #6370
Conversation
- Implemented try-catch block in `install_app` function to handle installation failures and provide informative error messages. - Added `Get-HistoricalManifest` function to search for specific app versions in git history, enhancing version management during installation. - Updated `generate_user_manifest` to utilize historical manifests as a fallback when autoupdate fails. - Enhanced documentation in `scoop-config.ps1` to clarify the use of git history for version installation.
There was a problem hiding this comment.
Pull Request Overview
This PR adds a historical manifest search feature to the app installation process, enabling Scoop to search a bucket’s git history for specific app versions before falling back to automatic manifest generation.
- Updated installation command comments to document the new historical search fallback.
- Introduced configuration options and functions (e.g., Get-HistoricalManifest) to support historical manifest retrieval.
- Added fallback logic in autoupdate and installation routines to use historical manifests when available.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| libexec/scoop-install.ps1 | Updated comments to reflect search order: git history first, then auto-generation. |
| libexec/scoop-config.ps1 | Added configuration option for git history usage. |
| lib/manifest.ps1 | Implemented Get-HistoricalManifest and added detailed git history search logic. |
| lib/install.ps1 | Wrapped installer calls in a try/catch to improve error handling and fallback behavior. |
|
|
One of the current implementation's issue is that when user provided a approximate version it will search all the git log entry which is a major performance bottleneck |
|
The best way to search historical version is using sqlite feature enabled, and you should search the database first, if not found, use git history. |
Will implement the SQLite version when I am available! |
|
It is there already, so you should combine these two methods. |
- Add Get-RelativePathCompat function that falls back to Uri-based implementation for PowerShell 5.x - Replace direct GetRelativePath call with compatible wrapper - Ensures historical manifest feature works across all PowerShell versions
- Add Get-HistoricalManifestFromDB function to search SQLite cache first - Modify Get-HistoricalManifest to use SQLite before falling back to git history - Implement best version matching for SQLite database results - Add conditional database import to ensure functions are available - Improve user messages to indicate whether cache or git history is used This addresses @niheaven's suggestion to use SQLite for faster historical version searches, significantly reducing performance bottlenecks when searching for approximate versions.
- Fix code style issues to pass tests - Maintain existing functionality without changes
- Change '3.7.x' to '3.12.x' to match the command '[email protected]' - Addresses GitHub Copilot's documentation consistency feedback
- Use Out-UTF8File instead of Set-Content with UTF8Encoding for PowerShell 5 - Add version checks to use appropriate encoding methods - Fixes 'Cannot bind parameter Encoding' error in Windows PowerShell 5 - Ensures historical manifest search works on all PowerShell versions
- Fix git show command returning arrays instead of strings - Join array elements with newlines to create proper JSON content - Remove misleading PowerShell 5 specific comments - Fixes historical manifest search in both PowerShell 5 and 7
- Remove git history fallback logic from install_app function - Move git history concerns to manifest layer where they belong - Improve separation of concerns and reduce coupling - Update tests to reflect the cleaner design - Merge git history tests into Scoop-Install.Tests.ps1 for consistency This follows the 'Pull complexity downward' principle by keeping installation logic focused on installation, while manifest resolution handles all version fallback strategies internally.
The try-catch block addition was a general error handling improvement that belongs in a separate PR, not in this git history feature PR.
… test expectations - Add missing psmodules.ps1 import to Install tests - Fix architecture parameter validation (x64 -> 64bit) - Add null/empty bucket parameter handling in Get-HistoricalManifestFromDB - Fix file format issues (add missing newlines, remove trailing whitespace) - Update JSON parsing tests to match actual implementation behavior - Fix path separator normalization in Get-RelativePathCompat test - Remove invalid error handling tests for non-existent try-catch blocks - Reduce test failures from 11 to 4, with remaining issues being mock-related All fixes respect existing codebase behavior and design decisions.
- Keep only exact match logic (lines 177-189) - Return null immediately when no exact match found (line 193) - Remove trailing whitespace from manifest.ps1 - Function now only supports exact version matches, no compatibility matching
Lutra-Fs
force-pushed
the
feature/install_via_git_history
branch
from
c51e52e to
4585f50
Compare
…idance and fallback mechanisms
…calManifestFromDB for exact version matching
…r improved data handling and output
Hi @niheaven |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds historical manifest lookup (SQLite cache and git history), integrates it into manifest generation before autoupdate, refactors DB command lifecycle and parameter binding, documents a use_git_history config, updates tests, and adds a changelog entry. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Installer as "scoop install"
participant Gen as "generate_user_manifest"
participant Cache as "SQLite Cache"
participant Git as "Git History"
participant AU as "Autoupdate"
User->>Installer: install app@version
Installer->>Gen: resolve manifest for app@version
rect rgba(220,240,255,0.35)
note right of Gen: Historical lookup (preferred)
Gen->>Cache: Find-HistoricalManifestInCache(app,bucket,version)
alt Cache hit
Cache-->>Gen: manifest text + version
Gen->>Installer: Write-ManifestToUserCache -> manifest path
else Cache miss
Gen->>Git: Find-HistoricalManifestInGit(app,bucket,version)
alt Git hit
Git-->>Gen: manifest text + version
Gen->>Installer: Write-ManifestToUserCache -> manifest path
else Git miss
Gen-->>Installer: no historical manifest found
end
end
end
opt Fallback to autoupdate
Gen->>AU: Invoke-AutoUpdate(app)
alt autoupdate success
AU-->>Gen: generated manifest path
Gen-->>Installer: manifest path
else autoupdate fails
Gen-->>Installer: abort with installation failure message
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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: 4
🧹 Nitpick comments (19)
CHANGELOG.md (1)
1-6: Changelog entry reads well; consider cross-linking the motivating issue.Add a reference to issue #3894 alongside the PR/issue link to help readers connect the feature to the original problem statement.
- - **install:** Add support for historical manifests ([#6370](https://github.com/ScoopInstaller/Scoop/issues/6370)) + - **install:** Add support for historical manifests ([#6370](https://github.com/ScoopInstaller/Scoop/issues/6370), closes [#3894](https://github.com/ScoopInstaller/Scoop/issues/3894))libexec/scoop-install.ps1 (1)
7-7: Clarify config keys in the help text.Name the exact settings to reduce ambiguity for users scanning help.
-# (will search sqlite cache if enabled, then git history (if use_git_history is $true), then auto-generate manifest as fallback): +# (searches SQLite cache first if use_sqlite_cache = $true, then git history if use_git_history = $true, then autoupdate-based manifest generation as fallback):libexec/scoop-config.ps1 (1)
33-37: Document partial-version “best match” behavior.The feature also supports partial versions (e.g., 2.30 → 2.30.x). Reflect that to set correct expectations.
-# Enable searching for specific versions in git history when installing apps with version specifiers. -# When enabled, Scoop will first search the bucket's git history for the exact version before falling back to autoupdate. +# Enable searching for specific versions in git history when installing apps with version specifiers. +# When enabled, Scoop will first search the bucket's git history for the requested version +# (exact match, or best match when a partial like '2.30' is provided) before falling back to autoupdate.lib/database.ps1 (1)
312-315: Reusing a prepared command is correct; clear params per iteration in Process for safety.You already clear in Process for Get-ScoopDBItem. Consider mirroring this pattern in Select-ScoopDBItem to prevent parameter accumulation across multiple Process calls.
function Select-ScoopDBItem { @@ - process { - $dbCommand.Parameters.AddWithValue('@Pattern', $(if ($Pattern -eq '') { '%' } else { '%' + $Pattern + '%' })) | Out-Null + process { + $dbCommand.Parameters.Clear() + $dbCommand.Parameters.AddWithValue('@Pattern', $(if ($Pattern -eq '') { '%' } else { '%' + $Pattern + '%' })) | Out-Null [void]$dbAdapter.Fill($result) }test/Scoop-Manifest.Tests.ps1 (3)
98-109: Paths are Windows-specific; optionally gate on platform to avoid cross-OS flakiness.Use $IsWindows to skip on non-Windows agents, or normalize with Join-Path.
Describe 'Get-RelativePathCompat' -Tag 'Scoop' { - It 'returns relative path for child path' { + It 'returns relative path for child path' -Skip:(-not $IsWindows) { @@ - It 'returns original when different drive/scheme' { + It 'returns original when different drive/scheme' -Skip:(-not $IsWindows) {
112-131: DB-path test shape could better reflect DataTable.Mock Get-ScoopDBItem to return a real DataTable with Rows to align with the new function behavior and catch schema regressions.
- Mock Get-ScoopDBItem { [pscustomobject]@{ Rows = @([pscustomobject]@{ manifest = '{"version":"1.2.3"}' }) } } + Mock Get-ScoopDBItem { + $t = New-Object System.Data.DataTable + [void]$t.Columns.Add('manifest',[string]) + $row = $t.NewRow(); $row.manifest = '{"version":"1.2.3"}'; $t.Rows.Add($row) | Out-Null + $t + }
163-237: Add a test for partial-version “best match”.Given the feature supports requests like 2.30 → best 2.30.x, add a case to assert best-match selection and source labeling (e.g., git_best_match).
I can draft this Pester example if helpful.
lib/manifest.ps1 (12)
175-204: Normalize inputs and harden Uri fallback in Get-RelativePathCompatGuard against relative inputs and Uri construction failures to avoid edge-case crashes on PS 5.1 and non-rooted paths.
function Get-RelativePathCompat($from, $to) { @@ - if ($PSVersionTable.PSVersion.Major -ge 6) { + if ($PSVersionTable.PSVersion.Major -ge 6) { # PowerShell Core/7+ - use built-in method try { return [System.IO.Path]::GetRelativePath($from, $to) } catch { # Fallback if method fails } } - # Windows PowerShell compatible implementation - $fromUri = New-Object System.Uri($from.TrimEnd('\') + '\') - $toUri = New-Object System.Uri($to) + # Windows PowerShell compatible implementation + try { + $fromFull = [System.IO.Path]::GetFullPath($from) + $toFull = [System.IO.Path]::GetFullPath($to) + } catch { + # If normalization fails, return original 'to' as-is + return $to + } + try { + $fromUri = New-Object System.Uri($fromFull.TrimEnd('\') + '\') + $toUri = New-Object System.Uri($toFull) + } catch { + return $to + } @@ - return $relativePath -replace '/', '\' + return $relativePath -replace '/', '\' }
228-233: Wrap ConvertFrom-Json with error handlingA malformed DB entry would currently throw and skip cleanup. Add try/catch (as shown in the previous diff) to keep execution robust.
382-401: Replace Write-Host with repo-standard info/warn logging for consistency and toolingUniform logging helps scripting scenarios and respects verbosity settings.
- Write-Host "" - Write-Host "No exact match found for version '$requestedVersion' for app '$app'." - Write-Host "Available versions in git history (newest to oldest):" - Write-Host "" + info '' + info "No exact match found for version '$requestedVersion' for app '$app'." + info "Available versions in git history (newest to oldest):" + info '' @@ - Write-Host " $($allAvailableVersions[$i])" + info " $($allAvailableVersions[$i])" @@ - Write-Host " ... and $($allAvailableVersions.Count - $displayCount) more versions" + info " ... and $($allAvailableVersions.Count - $displayCount) more versions" @@ - Write-Host "" - Write-Host "To install a specific version, use: scoop install $app@<version>" - Write-Host "" + info '' + info "To install a specific version, use: scoop install $app@<version>" + info ''
377-381: Deduplicate version list before displayAvoid repeated versions if multiple commits share the same version string.
- $allAvailableVersions = ($foundVersions | Sort-Object { - try { [version]($_.version -replace '[^\d\.].*$', '') } catch { $_.version } - } -Descending).version + $allAvailableVersions = ( + $foundVersions | Sort-Object { + try { [version]($_.version -replace '[^\d\.].*$', '') } catch { $_.version } + } -Descending + ).version | Select-Object -Unique
417-417: Avoid noisy warning when manifest is nullOnly warn about a mismatch if a current manifest exists; otherwise clarify state.
- warn "Given version ($version) does not match manifest ($($manifest.version))" + if ($manifest) { + warn "Given version ($version) does not match manifest ($($manifest.version))" + } else { + warn "No current manifest found for '$bucket/$app'; falling back to history/autoupdate." + }
501-509: Remove redundant git-history fallback after autoupdate catchGit history has already been tried above when $historicalResult is null; re-running it here doubles the cost without benefit.
- # If autoupdate fails and we haven't tried git history yet, try it as final fallback - if (!$historicalResult -and !(get_config USE_SQLITE_CACHE)) { - warn 'Autoupdate failed. Trying git history as final fallback...' - $fallbackResult = Get-HistoricalManifestFromGitHistory $app $bucket $version - if ($fallbackResult) { - warn "Using historical manifest as fallback for '$app' version '$($fallbackResult.version)'" - return $fallbackResult.path - } - }
486-500: Use standard warn/info logging instead of Write-Host with colorsAligns with the rest of the codebase and improves scriptability.
- Write-Host -ForegroundColor DarkRed "Could not install $app@$version" - Write-Host -ForegroundColor Yellow "Autoupdate failed for version $version" + warn "Could not install $app@$version" + warn "Autoupdate failed for version $version" @@ - Write-Host "Possible reasons:" - Write-Host " - Version $version may not exist or be available for download" - Write-Host " - Download URLs may have changed or be inaccessible" - Write-Host " - The version format may be incompatible with autoupdate patterns" - Write-Host "" - Write-Host "Suggestions:" - Write-Host " 1. Install current version: scoop install $app" - Write-Host " 2. Try a different version that was shown in the available list" - Write-Host " 3. Check the app's official releases or download page" - Write-Host "" + info "Possible reasons:" + info " - Version $version may not exist or be available for download" + info " - Download URLs may have changed or be inaccessible" + info " - The version format may be incompatible with autoupdate patterns" + info "" + info "Suggestions:" + info " 1. Install current version: scoop install $app" + info " 2. Try a different version that was shown in the available list" + info " 3. Check the app's official releases or download page" + info "" @@ - Write-Host -ForegroundColor Red "All attempts to find or generate manifest for '$app@$version' failed." + warn "All attempts to find or generate manifest for '$app@$version' failed."Also applies to: 511-514
421-438: Optional: Short-circuit when DB/githistory finds a manifest to avoid continued messagingYou already return early, but consider scoping subsequent guidance inside an else branch to avoid confusion when refactoring later.
239-407: Performance: Consider DB prefix search before scanning up to 200 commitsFor approximate versions, a DB LIKE-based query (e.g., '2.30.%') can be far faster than iterating git history. Keep git as the fallback when DB lacks entries.
Do we have or can we add a DB helper that supports prefix queries (VersionPrefix or -Like)? If yes, I can draft the integration.
287-294: Heuristic early-stops may skip valid candidatesThe 200-commit cap and previous-minor limit can stop before encountering a valid matching version in long histories. Consider stopping only after a best-match has been selected or when crossing below the requested major/minor boundary.
Also applies to: 353-361
421-429: Ensure database.ps1 is loaded before inline DB calls (if you keep them)If you decide not to remove the duplicate DB check, make sure to dot-source database.ps1 here too (as done in Get-HistoricalManifestFromDB) to avoid "command not found" errors when the earlier function path wasn’t taken.
239-407: Add tests for edge paths and config matricesPlease add tests for:
- USE_SQLITE_CACHE on/off; USE_GIT_HISTORY on/off; autoupdate present/absent.
- Approximate version resolving to best match (e.g., X.Y → highest X.Y.Z).
- DB result shape variations (DataTable vs single object) and malformed JSON.
I can generate Pester tests covering these matrices if helpful.
Also applies to: 410-517
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
CHANGELOG.md(1 hunks)lib/database.ps1(2 hunks)lib/manifest.ps1(2 hunks)libexec/scoop-config.ps1(2 hunks)libexec/scoop-install.ps1(1 hunks)test/Scoop-Manifest.Tests.ps1(2 hunks)
🔇 Additional comments (13)
CHANGELOG.md (1)
11-11: Bullet formatting change looks consistent.No functional impact; matches the surrounding style.
libexec/scoop-config.ps1 (1)
174-174: Minor style tweak LGTM.Space after if aligns with prevailing style.
lib/database.ps1 (3)
267-272: Good fix: avoid enumerating DataTable and dispose resources.Copying and emitting with -NoEnumerate prevents row-wise enumeration and post-disposal access issues.
298-307: Pipeline binding improvements are solid.ValueFromPipelineByPropertyName on Name/Bucket/Version improves composability without breaking callers.
320-345: Parameterized query and per-call parameter reset are correct.Prevents SQL injection and stale parameter values; returning a DataTable copy is appropriate for downstream consumers that expect Rows.
test/Scoop-Manifest.Tests.ps1 (8)
3-8: Expanded module loading is appropriate for new test coverage.Brings needed functions into scope for history-aware tests.
14-23: Stronger assertions for invalid JSON behavior.Covers both return value and warning side-effect; good.
135-160: Git-history test is representative and isolates external calls.Mocks for Find-BucketDirectory/Test-Path/Invoke-Git are sufficient; nice.
164-169: Manifest passthrough case covered.Ensures fast path when requested equals current.
171-182: DB-first preference test looks good.Asserts correct call order and result selection.
184-195: Fallback-to-git test is correct.Validates miss in DB leads to Git lookup.
197-208: Autoupdate fallback test is clear.Covers generation path and output location.
227-236: Abort path test is appropriate.Ensures a hard failure when neither history nor autoupdate is available.
…Write-ManifestToUserCache helper function
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
lib/manifest.ps1 (2)
205-233: Normalize DB result shape; current code assumes DataTable.Rows.Get-ScoopDBItem can return a DataTable, DataRow, or PSObject. Accessing .Rows[0] will throw in the latter cases.
Apply this diff:
- $dbResult = Get-ScoopDBItem -Name $app -Bucket $bucket -Version $requestedVersion - if ($dbResult.Rows.Count -gt 0) { - $row = $dbResult.Rows[0] - $manifestText = $row.manifest - $manifestObj = $null - try { $manifestObj = $manifestText | ConvertFrom-Json -ErrorAction Stop } catch {} - $manifestVersion = if ($manifestObj -and $manifestObj.version) { $manifestObj.version } else { $requestedVersion } - - return @{ ManifestText = $manifestText; version = $manifestVersion; source = "sqlite_exact_match" } - } + $dbResult = Get-ScoopDBItem -Name $app -Bucket $bucket -Version $requestedVersion + if ($null -eq $dbResult) { return $null } + $first = $null + if ($dbResult.PSObject.Properties['Rows']) { + if ($dbResult.Rows.Count -eq 0) { return $null } + $first = $dbResult.Rows[0] + } else { + $first = $dbResult + } + $manifestText = $null + if ($first -is [System.Data.DataRow]) { + $manifestText = $first['manifest'] + } elseif ($first.PSObject.Properties['manifest']) { + $manifestText = $first.manifest + } + if ([string]::IsNullOrWhiteSpace($manifestText)) { return $null } + $manifestObj = $null + try { $manifestObj = $manifestText | ConvertFrom-Json -ErrorAction Stop } catch {} + $manifestVersion = if ($manifestObj -and $manifestObj.version) { $manifestObj.version } else { $requestedVersion } + return @{ ManifestText = $manifestText; version = $manifestVersion; source = "sqlite_exact_match" }
234-290: Fix commit selection: use the commit(s) containing the requested version, not HEAD or parent-of-first.Using HEAD when only one match exists and commit^ when multiple is off-by-one and can fetch the wrong version. Iterate commits and return the first whose blob matches the requested version.
Apply this diff:
- if ($commits.Count -eq 0) { return $null } - - $targetSpec = if ($commits.Count -ge 2) { "$($commits[0])^" } else { 'HEAD' } - - $manifestContent = Invoke-Git -Path $bucketDir -ArgumentList @('show', "$targetSpec`:$relativeManifestPath") - if (-not $manifestContent -or ($LASTEXITCODE -ne 0)) { return $null } - if ($manifestContent -is [Array]) { $manifestContent = $manifestContent -join "`n" } - - $manifestObj = $null - try { $manifestObj = $manifestContent | ConvertFrom-Json -ErrorAction Stop } catch { return $null } - if (-not $manifestObj -or $manifestObj.version -ne $requestedVersion) { return $null } - - return @{ ManifestText = $manifestContent; version = $requestedVersion; source = "git_manifest:$targetSpec" } + if ($commits.Count -eq 0) { return $null } + + foreach ($c in $commits) { + $content = Invoke-Git -Path $bucketDir -ArgumentList @('show', "$c`:$relativeManifestPath") + if (-not $content -or ($LASTEXITCODE -ne 0)) { continue } + if ($content -is [Array]) { $content = $content -join "`n" } + try { + $obj = $content | ConvertFrom-Json -ErrorAction Stop + } catch { + continue + } + if ($obj -and $obj.version -eq $requestedVersion) { + return @{ ManifestText = $content; version = $requestedVersion; source = "git_manifest:$c" } + } + } + return $nullNote: ensure Invoke-Git reliably propagates $LASTEXITCODE under proxy mode; see prior comment in core.ps1 about setting $global:LASTEXITCODE from the job path.
🧹 Nitpick comments (3)
test/Scoop-Manifest.Tests.ps1 (3)
98-109: Make path assertions cross-platform.Tests hardcode backslashes; on non-Windows/PS7, Get-RelativePathCompat returns OS-native separators. Assert with Join-Path or a regex.
Apply this diff:
It 'returns relative path for child path' { - $from = 'C:\root\bucket' - $to = 'C:\root\bucket\foo\bar.json' - Get-RelativePathCompat $from $to | Should -Be 'foo\bar.json' + $from = 'C:\root\bucket' + $to = 'C:\root\bucket\foo\bar.json' + $expected = Join-Path 'foo' 'bar.json' + Get-RelativePathCompat $from $to | Should -Be $expected }
111-128: Add a DB-return-shape test to prevent regressions.Current test covers a Rows[...] shape only. Add a case where Get-ScoopDBItem returns a single PSObject with .manifest to match production variability.
Apply this diff to add coverage:
It 'returns manifest text and version when cache has exact match' { @@ $result.ManifestText | Should -Match '"version":"1.2.3"' } + +It 'handles single-object DB return shape (no .Rows)' { + Mock get_config -ParameterFilter { $name -in @('use_sqlite_cache','use_git_history') } { $true } + Mock Get-ScoopDBItem { [pscustomobject]@{ manifest = '{"version":"9.9.9"}' } } + $result = Find-HistoricalManifestInCache 'foo' 'main' '9.9.9' + $result | Should -Not -BeNullOrEmpty + $result.version | Should -Be '9.9.9' + $result.ManifestText | Should -Match '"version":"9.9.9"' +}
202-215: Assert warnings on autoupdate failure.Strengthen the failure-path test by verifying warn was invoked once.
Apply this diff:
Mock warn {} @@ { generate_user_manifest 'foo' 'main' '1.0.0' } | Should -Throw +Should -Invoke -CommandName warn -Times 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
lib/manifest.ps1(2 hunks)test/Scoop-Manifest.Tests.ps1(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: Lutra-Fs
PR: ScoopInstaller/Scoop#6370
File: lib/manifest.ps1:239-407
Timestamp: 2025-08-27T09:24:36.044Z
Learning: User Lutra-Fs prefers to keep PR scope focused and avoid adding unnecessary complexity, even when suggested functionality aligns with stated objectives. They prioritize clean, focused implementations over feature completeness in a single PR.
📚 Learning: 2025-08-27T09:24:36.044Z
Learnt from: Lutra-Fs
PR: ScoopInstaller/Scoop#6370
File: lib/manifest.ps1:239-407
Timestamp: 2025-08-27T09:24:36.044Z
Learning: User Lutra-Fs prefers to keep PR scope focused and avoid adding unnecessary complexity, even when suggested functionality aligns with stated objectives. They prioritize clean, focused implementations over feature completeness in a single PR.
Applied to files:
lib/manifest.ps1
🔇 Additional comments (7)
test/Scoop-Manifest.Tests.ps1 (3)
3-7: LGTM: necessary module imports loaded upfront.Brings core, buckets, database, and autoupdate into scope for the new tests. Looks good.
14-22: LGTM: stronger assertions for parse_json.Non-throw + null and warning checks are appropriate.
217-226: LGTM: clear abort when no history and no autoupdate.lib/manifest.ps1 (4)
175-203: LGTM: compatible relative-path helper.Graceful PS7 vs Windows PowerShell handling and URI fallback look good.
292-319: LGTM: clear, layered orchestration (DB → Git).Writes to user cache only when ManifestText is present; sensible.
329-347: LGTM: history-first, then autoupdate with concise failure messaging.Flow and messages align with PR goals.
357-367: LGTM: small, reusable writer helper.
…tching version via h'^/h with validated fallback
…improved manifest retrieval
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
lib/manifest.ps1 (2)
175-203: Get-RelativePathCompat: normalize paths and handle cross-drive earlyEnsure absolute paths and bail out on different roots to avoid odd “absolute-as-relative” results on Windows PowerShell.
function Get-RelativePathCompat($from, $to) { @@ - # Windows PowerShell compatible implementation - $fromUri = New-Object System.Uri($from.TrimEnd('\') + '\') - $toUri = New-Object System.Uri($to) + # Windows PowerShell compatible implementation + $fromAbs = [System.IO.Path]::GetFullPath($from) + $toAbs = [System.IO.Path]::GetFullPath($to) + # If roots (drive/UNC) differ, return absolute target path + $fromRoot = ([System.IO.Path]::GetPathRoot($fromAbs)).TrimEnd('\') + $toRoot = ([System.IO.Path]::GetPathRoot($toAbs)).TrimEnd('\') + if ($fromRoot -ne $toRoot) { return $toAbs } + $fromUri = [System.Uri]::new($fromAbs.TrimEnd('\') + '\') + $toUri = [System.Uri]::new($toAbs) @@ - return $relativePath -replace '/', '\' + return $relativePath -replace '/', '\' }
378-388: Sanitize app name when writing to user cache pathPrevent odd filenames or accidental subpaths by using the existing
sanitary_pathhelper.- $tempManifestPath = "$(usermanifestsdir)\$App.json" + $safe = sanitary_path $App + $tempManifestPath = "$(usermanifestsdir)\$safe.json"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
CHANGELOG.md(2 hunks)lib/manifest.ps1(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: Lutra-Fs
PR: ScoopInstaller/Scoop#6370
File: lib/manifest.ps1:239-407
Timestamp: 2025-08-27T09:24:36.066Z
Learning: User Lutra-Fs prefers to keep PR scope focused and avoid adding unnecessary complexity, even when suggested functionality aligns with stated objectives. They prioritize clean, focused implementations over feature completeness in a single PR.
📚 Learning: 2025-08-27T09:24:36.066Z
Learnt from: Lutra-Fs
PR: ScoopInstaller/Scoop#6370
File: lib/manifest.ps1:239-407
Timestamp: 2025-08-27T09:24:36.066Z
Learning: User Lutra-Fs prefers to keep PR scope focused and avoid adding unnecessary complexity, even when suggested functionality aligns with stated objectives. They prioritize clean, focused implementations over feature completeness in a single PR.
Applied to files:
lib/manifest.ps1
🔇 Additional comments (5)
lib/manifest.ps1 (3)
238-312: Git history: $LASTEXITCODE reliance and search bounds
$LASTEXITCODEfromInvoke-Gitcan be unreliable under the Start-Job proxy path; you already tracked this separately. Given that, your existing-not $contentguard is the safer primary check. No action needed here within this PR.- Optional: cap the breadth of the fallback scan (
$outAll) or make it configurable to avoid walking very deep histories on large buckets.
313-341: Orchestrator flow looks goodClear DB→Git ordering and single place to persist the manifest. No issues spotted.
350-367: generate_user_manifest: error flow and messaging are appropriateHistory-first, then autoupdate with clear aborts on failure. LGTM.
CHANGELOG.md (2)
5-5: Changelog entry reads wellAccurately reflects the added feature and links the issue.
22-22: Minor formatting tweak is fineNo semantic changes; formatting looks consistent.
| function Find-HistoricalManifestInCache($app, $bucket, $requestedVersion) { | ||
| if (!(get_config USE_SQLITE_CACHE)) { | ||
| return $null | ||
| } | ||
|
|
||
| # Return null if bucket is null or empty | ||
| if (!$bucket) { | ||
| return $null | ||
| } | ||
|
|
||
| # Import database functions if not already loaded | ||
| if (!(Get-Command 'Get-ScoopDBItem' -ErrorAction SilentlyContinue)) { | ||
| . "$PSScriptRoot\database.ps1" | ||
| } | ||
|
|
||
| $dbResult = Get-ScoopDBItem -Name $app -Bucket $bucket -Version $requestedVersion | ||
|
|
||
| # Strictly follow DB contract: must be DataTable with at least one row | ||
| if (-not ($dbResult -is [System.Data.DataTable])) { return $null } | ||
| if ($dbResult.Rows.Count -eq 0) { return $null } | ||
|
|
||
| $manifestText = $dbResult.Rows[0]['manifest'] | ||
| if ([string]::IsNullOrWhiteSpace($manifestText)) { return $null } | ||
|
|
||
| $manifestObj = $null | ||
| try { $manifestObj = $manifestText | ConvertFrom-Json -ErrorAction Stop } catch {} | ||
| $manifestVersion = if ($manifestObj -and $manifestObj.version) { $manifestObj.version } else { $requestedVersion } | ||
|
|
||
| return @{ ManifestText = $manifestText; version = $manifestVersion; source = "sqlite_exact_match" } | ||
|
|
||
| return $null | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
SQLite path: unreachable return and strict result-shape assumptions
- Line 235: The
return $nullis unreachable. Remove it. - Result handling assumes a DataTable; if
Get-ScoopDBItemever returns a DataRow/PSObject, this will silently skip a valid hit. If that contract is guaranteed, fine; otherwise add a small normalization block.
return @{ ManifestText = $manifestText; version = $manifestVersion; source = "sqlite_exact_match" }
-
- return $nullIf you want to be defensive without much code churn, consider:
if (-not ($dbResult -is [System.Data.DataTable])) { return $null }documented as an intentional contract check (i.e., add a short comment). Otherwise, normalize the first item regardless of shape.
🏁 Script executed:
#!/bin/bash
# Locate and inspect the Get-ScoopDBItem function to confirm its return contract
rg -nP -C5 "function\s+Get-ScoopDBItem" -g "*.ps1"Length of output: 518
🏁 Script executed:
#!/bin/bash
sed -n '290,360p' lib/database.ps1Length of output: 2133
Remove unreachable return; DataTable check is safe
- Drop the second
return $nullafter the manifest return (line 235). Get-ScoopDBItemis documented with.OUTPUTS System.Data.DataTableand usesWrite-Outputon a DataTable copy, so the strict DataTable check is safe—no normalization needed.
🤖 Prompt for AI Agents
In lib/manifest.ps1 around lines 205 to 237, remove the unreachable "return
$null" that appears after the successful "return @{ ManifestText =
$manifestText; version = $manifestVersion; source = "sqlite_exact_match" }"
(line ~235); simply delete that trailing return so control flow is correct, and
keep the existing strict DataTable checks as-is since Get-ScoopDBItem reliably
outputs a System.Data.DataTable.
niheaven
changed the title
|
Just noticed that I cannot push to the branch anymore ＞﹏＜ since more strict permission setup has been introduced. |
| # Strictly follow DB contract: must be DataTable with at least one row | ||
| if (-not ($dbResult -is [System.Data.DataTable])) { return $null } |
There was a problem hiding this comment.
| # Strictly follow DB contract: must be DataTable with at least one row | |
| if (-not ($dbResult -is [System.Data.DataTable])) { return $null } |
|
|
||
| return $null |
There was a problem hiding this comment.
| return $null |
|
Still working on it, and I'll merge the reviewed code after resolved conflicts. |
Description
Motivation and Context
Closes #3894
Relates to #XXXX
How Has This Been Tested?
Checklist:
developbranch.Summary by CodeRabbit
New Features
Improvements
Documentation
Tests