Repository navigation
Keep the pre-indexed source file cache inside the configured app root - #51
Merged
Merged
Conversation
TempCachePath resolved GetPackagedFileCacheRoot(null), so every manifest and versionData file fetched from a pre-indexed source's CDN landed in the default per-user app root no matter which root the repository was opened with. A host that sets PINGET_APPROOT - a portable UniGetUI, or anything passing RepositoryOptions.AppRoot - had its store relocated but kept writing these files to %LOCALAPPDATA%\Devolutions\Pinget\Microsoft\ Windows Package Manager. Pinget's own tests showed it: they open a repository under a temp app root and delete it afterwards, yet their fixtures accumulate in the developer's real profile. Thread the app root through GetCachedSourceFile and LoadV2VersionData instead. Passing null still resolves the default root, so an ordinary install keeps its existing cache and needs no migration. The Rust twin had drifted the other way: cache_root honoured the app root only for the packaged layout and otherwise fell back to %TEMP%\cache. Both implementations now derive the cache from the app root alone, so they agree on every platform. The app root is no longer an optional trailing parameter on either helper. Omitting it used to compile fine and silently reintroduce the bug. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
TempCachePath interpolated SourceRecord.Identifier as a path segment without sanitizing it, unlike SourceStateDir and SourceInstalledDbPath beside it, and unlike the Rust twin temp_cache_path, which has always called sanitize_path_segment. The identifier is not validated anywhere: AddSource takes it from the caller's source name, and the packaged-layout loader takes it straight from WinGet's metadata YAML. Path.Combine treats a segment whose second character is a colon as rooted, so an identifier like `c:foo` discards the cache root outright and writes outside the app root - which defeats the containment the previous commit establishes. Separators in the identifier escape it a directory at a time. It was also a live divergence: for any identifier containing one of \ / : * ? " < > | the two implementations cached to different directories and re-fetched each other's files. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The source-identifier sanitization needs dedicated regression coverage for unsafe identifier characters.
Review effort: Balanced
Findings: 1
What changed in this PR
Keeps Pinget caches within the configured app root, improving portable-install isolation.
Changes:
- Threads the app root through .NET pre-indexed cache operations and sanitizes source identifiers.
- Aligns Rust cache placement with the configured app root.
- Adds regression coverage and bumps packages to 0.13.0.
| File | Description |
|---|---|
rust/crates/pinget-core/src/lib.rs |
Relocates Rust caches and tests the path. |
rust/crates/pinget-core/Cargo.toml |
Bumps core to 0.13.0. |
rust/crates/pinget-com/Cargo.toml |
Bumps COM bridge to 0.13.0. |
rust/crates/pinget-cli/Cargo.toml |
Bumps CLI and core dependency. |
nuget/Devolutions.Pinget.Cli.Rust/Devolutions.Pinget.Cli.Rust.csproj |
Bumps Rust NuGet package. |
nuget/Devolutions.Pinget.Cli.DotNet/Devolutions.Pinget.Cli.DotNet.csproj |
Bumps .NET NuGet package. |
dotnet/src/Devolutions.Pinget.PowerShell.Engine/PowerShellEngineVersion.cs |
Updates PowerShell engine version. |
dotnet/src/Devolutions.Pinget.PowerShell.Cmdlets/ModuleFiles/Devolutions.Pinget.Client.psd1 |
Updates module version. |
dotnet/src/Devolutions.Pinget.Core/SourceStore.cs |
Makes path sanitization reusable internally. |
dotnet/src/Devolutions.Pinget.Core/Repository.cs |
Passes the configured app root to cache calls. |
dotnet/src/Devolutions.Pinget.Core/PreIndexedSource.cs |
Roots caches correctly and sanitizes identifiers. |
dotnet/src/Devolutions.Pinget.Core.Tests/CoreTests.cs |
Adds cache-containment regression coverage. |
dotnet/src/Devolutions.Pinget.Cli/Program.cs |
Updates CLI version. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
The existing containment test used the source identifier `test`, which sanitizes to itself, so it stayed green with the SanitizePathSegment call removed - the app-root fix and the path-containment fix were not covered independently. Drive the same flow with the identifier `c:foo`. Path.Combine treats a second-character colon as rooted, so without sanitizing, the cached file escapes the app root entirely and the containment assertion fails; with it, the cache lands under `V2_M\c_foo` and `V2_PVD\c_foo` inside the configured app root. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Marc-André Moreau (mamoreau-devolutions)
approved these changes
Oct 6, 2026
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
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.

Setting
PINGET_APPROOT(orRepositoryOptions.AppRoot) relocated the store but not the pre-indexed source file cache, so manifests kept landing in the default per-user app root. Reported downstream as Devolutions/UniGetUI#5337: a portable UniGetUI still wrote Pinget files into%LOCALAPPDATA%, even for users on the WinGet backend who never selected Pinget.Root cause
PreIndexedSource.TempCachePathresolvedGetPackagedFileCacheRoot(null)— it ignored the app root the repository was opened with — so every manifest andversionData.mszymlfetched from a pre-indexed source's CDN went to:Our own test suite shows it:
Show_Preindexed*open a repository under a temp app root and delete it infinally, yet their fixtures accumulate in the developer's real profile (V2_M\test\manifests\Test.*.yaml).The Rust twin had drifted the other way —
cache_roothonoured the app root only for the packaged layout and otherwise fell back to%TEMP%\cache, so for a custom root the two implementations cached to different places entirely.Changes
Three commits, reviewable independently; each builds and is green on its own.
c357a55— Cache pre-indexed source files under the configured app root. Threads the app root throughGetCachedSourceFileandLoadV2VersionData;cache_rootin Rust now always derives from the app root. Passingnullstill resolves the default root, so an ordinary install keeps its existing cache and needs no migration. The app root is no longer an optional trailing parameter on either helper — omitting it used to compile fine and silently reintroduce the bug.26f13dd— Sanitize the source identifier in the cache path. Pre-existing hole, split out so it can be dropped on its own.TempCachePathinterpolatedSourceRecord.Identifierraw, unlikeSourceStateDir/SourceInstalledDbPathbeside it and unlike Rust'stemp_cache_path. The identifier is never validated (AddSourcetakes the caller's name; the packaged loader takes it from WinGet's metadata YAML), andPath.Combinetreats a second-character colon as rooted:so an identifier alone could put the cache outside the app root and defeat the containment the first commit establishes.
aeb7b97— Bump version to 0.13.0. Viascripts/Set-PingetVersion.ps1. Minor bump per repo convention (no patch releases in history), and the right slot anyway given the cache relocations and the two now-required parameters.Verification
End-to-end in UniGetUI, not just in isolation: two portable copies of the same build, one with the shipped
Devolutions.Pinget.Core.dlland one with this branch's, each serving onepackage detailsrequest for a package not yet cached.%LOCALAPPDATA%\Devolutions\PingetInkscape.InkscapeV2_M\...\manifests\i\Inkscape\...,V2_PVD\...\versionData.mszymlqBittorrent.qBittorrent<portable>\Settings\Pinget\Microsoft\Windows Package Manager\...Installer metadata is unaffected — a cold-miss re-fetch of
Inkscape.Inkscapeunder the fix returns the same version, installer URL, SHA256 and type as the stock build.All
AGENTS.mdgates pass at 0.13.0:cargo fmt --checkcargo clippy --workspace --tests -- -D warningscargo test -p pinget-corecargo test -p pinget-clidotnet build slnx -c Releasedotnet test Core.Testspwsh RunTests.ps1dotnet format --verify-no-changesIDE0060, none in touched filesThe new
Show_Preindexed_CachesSourceFilesInsideTheConfiguredAppRootis a genuine regression test — it fails without the fix and asserts both theV2_MandV2_PVDbuckets land under the app root, so dropping_appRootfrom any single call site is caught.Behaviour changes worth a reviewer's sign-off
...\Microsoft.DesktopAppInstaller_8wekyb3d8bbwe\LocalState, .NET previously cached to%LOCALAPPDATA%\Devolutions\Pinget\...and now caches under thatLocalState. Rust always did this andCoreTests.cs:143documents it as the intent, but otherDevolutions.Pinget.Coreconsumers in packaged layout will see their manifest cache move. One-time re-download; no correctness impact.%TEMP%\cacheinto the app root. Nothing evicts these buckets —source removedeletes only the source state dir — so a long-lived install now grows permanently where OS temp cleanup could previously reclaim it. Worth deciding separately whethersource removeshould purgeV2_M/V2_PVD/REST_M.\ / : * ? " < > |— those entries are orphaned and re-fetched once.$TMPDIR/pinget/...into the app root. Covered by tests, but only exercised on Windows here.Nothing migrates or deletes an existing
%LOCALAPPDATA%\Devolutions\Pinget, so users keep whatever is already there and need to clear it once to see the difference.Downstream
After a 0.13.0 release, UniGetUI needs
Devolutions.Pinget.CoreandDevolutions.Pinget.Cli.Rustbumped off 0.12.0 inUniGetUI.PackageEngine.Managers.WinGet.csprojandUniGetUI.Avalonia.csproj.🤖 Generated with Claude Code