fix(workspace): prioritize custom files over installation data and isolate user workspaces - #429
fix(workspace): prioritize custom files over installation data and isolate user workspaces#429undead2146 wants to merge 1 commit into
Conversation
…olate user workspaces - Add GetPrioritizedWorkspaceFiles extension to deduplicate workspace files with strict ContentTypePriority ordering regardless of manifest order - Update FullCopyStrategy, HardLinkStrategy, HybridCopySymlinkStrategy, and SymlinkOnlyStrategy to use prioritized workspace files - Fix false-negative test skip conditions in workspace and CAS integration tests on Linux and macOS - Expand WorkspacePrioritizationVerifyTests with comprehensive hierarchy, order-independence, and user workspace isolation tests Closes #42
|
ⓘ Your Qodo trial ends soon. Ask your workspace admin to set up billing to keep reviews running after the trial. Manage billing |
|
Warning Review limit reachedNext included review available in 25 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (9)
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 |
 |
|
Overall Grade |
Security Reliability Complexity Hygiene |
Code Review Summary
| Analyzer | Status | Updated (UTC) | Details |
|---|---|---|---|
| C# |  |
Aug 30, 2026 6:50p.m. | Review ↗ |
| JavaScript | |
Aug 30, 2026 6:50p.m. | Review ↗ |
| Shell | |
Aug 30, 2026 6:50p.m. | Review ↗ |
| Secrets | |
Aug 30, 2026 6:50p.m. | Review ↗ |
Important
AI Review is run only on demand for your team. We're only showing results of static analysis review right now. To trigger AI Review, comment @deepsourcebot review on this thread.
PR Summary by QodoPrioritize custom content across isolated workspace strategies
AI Description
Diagram
High-Level Assessment
Files changed (9)
|
|
Code Review by Qodo
1. Missing winner drops fallback
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| .SelectMany((m, index) => (m.Files ?? []).Select(f => new { File = f, Manifest = m, ManifestIndex = index })) | ||
| .GroupBy(x => x.File.RelativePath, StringComparer.OrdinalIgnoreCase) | ||
| .Select(g => g.OrderByDescending(x => ContentTypePriority.GetPriority(x.Manifest.ContentType)) | ||
| .ThenByDescending(x => x.ManifestIndex) |
There was a problem hiding this comment.
1. getalluniquefiles exposes deferred sequence 📘 Rule violation ⚙ Maintainability
The modified public collection API still returns IEnumerable<ManifestFile> backed by a deferred LINQ query. This permits repeated execution and violates the requirement to expose a read-only collection that is eagerly materialized.
Agent Prompt
## Issue description
`GetAllUniqueFiles` exposes a deferred `IEnumerable<ManifestFile>` rather than an eagerly materialized read-only collection.
## Issue Context
The method returns a finite deduplicated collection, so callers should receive a stable `IReadOnlyList<ManifestFile>` or `IReadOnlyCollection<ManifestFile>` without re-running its LINQ pipeline.
## Fix Focus Areas
- GenHub/GenHub.Core/Extensions/WorkspaceConfigurationExtensions.cs[18-35]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| .SelectMany((m, index) => (m.Files ?? []).Select(f => new { File = f, Manifest = m, ManifestIndex = index })) | ||
| .Where(x => x.File.InstallTarget == ContentInstallTarget.Workspace) | ||
| .GroupBy(x => x.File.RelativePath, StringComparer.OrdinalIgnoreCase) | ||
| .Select(g => g.OrderByDescending(x => ContentTypePriority.GetPriority(x.Manifest.ContentType)) |
There was a problem hiding this comment.
2. getworkspaceuniquefiles remains deferred 📘 Rule violation ⚙ Maintainability
The modified public workspace-file lookup returns a deferred LINQ pipeline through IEnumerable<ManifestFile>. Its finite result should be materialized and exposed as a read-only collection.
Agent Prompt
## Issue description
`GetWorkspaceUniqueFiles` returns a deferred `IEnumerable<ManifestFile>` instead of an eagerly materialized read-only collection.
## Issue Context
The lookup computes a finite, deduplicated set whose query should execute inside the extension method and produce a stable collection for callers.
## Fix Focus Areas
- GenHub/GenHub.Core/Extensions/WorkspaceConfigurationExtensions.cs[37-59]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| if (file.SourceType == ContentSourceType.ContentAddressable && !string.IsNullOrEmpty(file.Hash)) | ||
| { | ||
| // Use CAS content | ||
| await CreateCasLinkAsync(file.Hash, destinationPath, manifest.ContentType, ct); |
There was a problem hiding this comment.
3. fullcopystrategy.prepareasync exceeds complexity 📘 Rule violation ⚙ Maintainability
The modified method has an estimated Sonar-style cognitive complexity of about 18, exceeding the required maximum of 14. Its nested parallel callback, source selection, validation, hash verification, progress handling, and exception paths should be decomposed.
Agent Prompt
## Issue description
`FullCopyStrategy.PrepareAsync` exceeds the cognitive-complexity limit of 14.
## Issue Context
The parallel callback combines source selection, CAS handling, path resolution, validation, copying, hash verification, progress reporting, and exception translation. Extract cohesive operations while preserving cancellation and priority behavior.
## Fix Focus Areas
- GenHub/GenHub/Features/Workspace/Strategies/FullCopyStrategy.cs[93-183]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| if (file.SourceType == ContentSourceType.ContentAddressable && !string.IsNullOrEmpty(file.Hash)) | ||
| { | ||
| if (file.SourceType == ContentSourceType.ContentAddressable && !string.IsNullOrEmpty(file.Hash)) | ||
| if (isEssential) | ||
| { |
There was a problem hiding this comment.
4. hybridcopysymlinkstrategy.prepareasync exceeds complexity 📘 Rule violation ⚙ Maintainability
The modified method has an estimated Sonar-style cognitive complexity of roughly 27–30, well above the maximum of 14. CAS handling, essential-file decisions, hash checks, and symlink/hardlink/copy fallbacks are deeply nested in one method.
Agent Prompt
## Issue description
`HybridCopySymlinkStrategy.PrepareAsync` substantially exceeds the cognitive-complexity limit.
## Issue Context
The method combines CAS and filesystem paths, essential and non-essential behavior, integrity verification, multiple fallback mechanisms, progress accounting, and exception translation. Split these branches into focused private helpers while forwarding the same cancellation token.
## Fix Focus Areas
- GenHub/GenHub/Features/Workspace/Strategies/HybridCopySymlinkStrategy.cs[118-217]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| .GroupBy(x => x.File.RelativePath, StringComparer.OrdinalIgnoreCase) | ||
| .Select(g => g | ||
| .OrderByDescending(x => ContentTypePriority.GetPriority(x.Manifest.ContentType)) | ||
| .ThenByDescending(x => x.ManifestIndex) | ||
| .First()) |
There was a problem hiding this comment.
5. Missing winner drops fallback 🐞 Bug ≡ Correctness
GetPrioritizedWorkspaceFiles discards lower-priority candidates before source availability is checked, so a missing local mod/patch winner prevents FullCopy and Hybrid from materializing an available base file at the same path. Both strategies treat the missing winner as non-fatal and can mark the workspace prepared, while validation uses the reduced successful-file count and therefore does not detect the omitted required destination.
Agent Prompt
## Issue description
Priority deduplication removes lower-priority candidates before the selected source is validated. If a local mod/patch winner is unavailable, FullCopy and Hybrid can silently omit the destination and report successful preparation even when a valid lower-priority base candidate exists.
## Issue Context
Preserve strict priority when the preferred override is available, but resolve candidates with manifest context and select the highest-priority candidate whose source can be materialized, or explicitly fail preparation when no acceptable source is available. `ValidateSourceFile` currently returns `false` rather than failing for missing local files, after which the strategies can mark the workspace prepared; validation must not rely only on the reduced count of successfully processed files or leave a manifest destination unaccounted for.
## Fix Focus Areas
- GenHub/GenHub.Core/Extensions/WorkspaceConfigurationExtensions.cs[75-85]
- GenHub/GenHub/Features/Workspace/Strategies/FullCopyStrategy.cs[139-186]
- GenHub/GenHub/Features/Workspace/Strategies/HybridCopySymlinkStrategy.cs[155-225]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| .SelectMany((manifest, index) => (manifest.Files ?? Enumerable.Empty<ManifestFile>()) | ||
| .Where(f => f.InstallTarget == ContentInstallTarget.Workspace) | ||
| .Select(file => new { File = file, Manifest = manifest, ManifestIndex = index })) | ||
| .GroupBy(x => x.File.RelativePath, StringComparer.OrdinalIgnoreCase) |
There was a problem hiding this comment.
6. Unix case-distinct files collapse 🐞 Bug ≡ Correctness
The new selector always groups paths with OrdinalIgnoreCase, so Hybrid now treats distinct Unix destinations such as Data/foo and Data/Foo as one conflict and materializes only one file. The repository's path policy explicitly uses case-sensitive comparison outside Windows, making this a cross-platform data omission.
Agent Prompt
## Issue description
Workspace conflict grouping unconditionally folds path case, which drops valid case-distinct files on case-sensitive filesystems.
## Issue Context
Use the repository's platform-aware filesystem comparer consistently in all workspace-file grouping helpers, and add a Unix regression test with two case-distinct relative paths.
## Fix Focus Areas
- GenHub/GenHub.Core/Extensions/WorkspaceConfigurationExtensions.cs[31-32]
- GenHub/GenHub.Core/Extensions/WorkspaceConfigurationExtensions.cs[55-56]
- GenHub/GenHub.Core/Extensions/WorkspaceConfigurationExtensions.cs[79-83]
- GenHub/GenHub.Core/Helpers/PathHelper.cs[34-41]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| return configuration.Manifests | ||
| .SelectMany(m => (m.Files ?? []).Select(f => new { File = f, Manifest = m })) | ||
| .SelectMany((m, index) => (m.Files ?? []).Select(f => new { File = f, Manifest = m, ManifestIndex = index })) | ||
| .GroupBy(x => x.File.RelativePath, StringComparer.OrdinalIgnoreCase) |
| .Where(x => x.File.InstallTarget == GenHub.Core.Models.Enums.ContentInstallTarget.Workspace) | ||
| .SelectMany((m, index) => (m.Files ?? []).Select(f => new { File = f, Manifest = m, ManifestIndex = index })) | ||
| .Where(x => x.File.InstallTarget == ContentInstallTarget.Workspace) | ||
| .GroupBy(x => x.File.RelativePath, StringComparer.OrdinalIgnoreCase) |
| .Select(file => new { File = file, Manifest = manifest, ManifestIndex = index })) | ||
| .GroupBy(x => x.File.RelativePath, StringComparer.OrdinalIgnoreCase) | ||
| .Select(g => g | ||
| .OrderByDescending(x => ContentTypePriority.GetPriority(x.Manifest.ContentType)) |
| .IsInRole(System.Security.Principal.WindowsBuiltInRole.Administrator); | ||
|
|
||
| if (!isWindows || !isAdmin) | ||
| if (isWindows && !isAdmin) |
| if ((strategy == WorkspaceStrategy.SymlinkOnly || | ||
| strategy == WorkspaceStrategy.HybridCopySymlink) && | ||
| (!isWindows || !isAdmin)) | ||
| isWindows && !isAdmin) |
There was a problem hiding this comment.
SUGGESTION: Same observation as WorkspaceCasIntegrationTests: the silent return makes the test look like it passed on skipped platforms. Recommend [SkippableFact(Skip = ...)] so test reporters reflect that the scenario was not exercised.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| .IsInRole(System.Security.Principal.WindowsBuiltInRole.Administrator); | ||
|
|
||
| if (!isWindows || !isAdmin) | ||
| if (isWindows && !isAdmin) |
There was a problem hiding this comment.
SUGGESTION: if (isWindows && !isAdmin) return; silently reports as pass when skipped. Use Skip = "Requires admin on Windows" / [SkippableFact] so skipped scenarios are surfaced in CI rather than masked as green.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| .IsInRole(System.Security.Principal.WindowsBuiltInRole.Administrator); | ||
|
|
||
| if (!isWindows || !isAdmin) | ||
| if (isWindows && !isAdmin) |
There was a problem hiding this comment.
SUGGESTION: Same runtime skip concern — prefer [SkippableFact(Skip = "Requires admin on Windows")] to make the skipped run visible in test output.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| // Skip symlink strategies when not admin on Windows | ||
| if ((strategy == WorkspaceStrategy.SymlinkOnly || strategy == WorkspaceStrategy.HybridCopySymlink) && | ||
| (!isWindows || !isAdmin)) | ||
| isWindows && !isAdmin) |
There was a problem hiding this comment.
SUGGESTION: Same skip-pattern observation as above for the parametrized strategy test.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| using System.Linq; | ||
| using System.Threading; | ||
| using System.Threading.Tasks; | ||
| using GenHub.Core.Extensions; |
There was a problem hiding this comment.
SUGGESTION: using Microsoft.Extensions.Logging; is added but the file uses Microsoft.Extensions.Logging.Abstractions (NullLogger) instead — the unqualified Microsoft.Extensions.Logging import is dead and should be removed to keep the alphabetical using list clean.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| using System.Threading.Tasks; | ||
| using GenHub.Core.Extensions; | ||
| using GenHub.Core.Interfaces.Common; | ||
| using GenHub.Core.Interfaces.Storage; |
There was a problem hiding this comment.
SUGGESTION: using GenHub.Core.Interfaces.Storage; is added but appears unused in this file (no ICas* symbols referenced).
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
|
|
||
| long totalSize = 0; | ||
| foreach (var manifest in configuration.Manifests) | ||
| foreach (var file in configuration.GetWorkspaceUniqueFiles()) |
There was a problem hiding this comment.
WARNING: EstimateDiskUsage now uses GetWorkspaceUniqueFiles(), which performs case-insensitive grouping. The returned Count therefore may over- or under-count on case-sensitive file systems, leading to misleading disk-usage estimates on Linux/macOS.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
|
|
||
| long totalUsage = 0; | ||
| foreach (var manifest in configuration.Manifests) | ||
| foreach (var file in configuration.GetWorkspaceUniqueFiles()) |
There was a problem hiding this comment.
WARNING: Same GetWorkspaceUniqueFiles() case-insensitive issue in HybridCopySymlinkStrategy.EstimateDiskUsage — on Linux/macOS the deduplicated file count may differ from the actual file set, producing skewed disk estimates.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
|
|
||
| // Symbolic links use minimal space - approximate 1KB per link for metadata | ||
| return configuration.Manifests.SelectMany(m => m.Files).Count() * LinkOverheadBytes; | ||
| return configuration.GetWorkspaceUniqueFiles().Count() * LinkOverheadBytes; |
There was a problem hiding this comment.
WARNING: Same case-insensitive GetWorkspaceUniqueFiles().Count() issue in SymlinkOnlyStrategy.EstimateDiskUsage — on Linux/macOS the link estimate may be incorrect when manifests contain paths that only differ by letter case.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| /// Verifies that GetPrioritizedWorkspaceFiles returns the winning file and manifest pair for all content types. | ||
| /// </summary> | ||
| [Fact] | ||
| public void GetPrioritizedWorkspaceFiles_FullHierarchy_ResolvesCorrectWinningManifests() |
There was a problem hiding this comment.
SUGGESTION: The hierarchy test only exercises four content types but the priority docstring lists eight (Mod, Patch, GameClient, ModdingTool, Addon, LanguagePack, Map, GameInstallation). Consider adding ModdingTool/Addon/LanguagePack/Map manifests to make this test actually cover the full hierarchy it claims to test.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| return configuration.Manifests | ||
| .SelectMany(m => (m.Files ?? []).Select(f => new { File = f, Manifest = m })) | ||
| .SelectMany((m, index) => (m.Files ?? []).Select(f => new { File = f, Manifest = m, ManifestIndex = index })) | ||
| .GroupBy(x => x.File.RelativePath, StringComparer.OrdinalIgnoreCase) |
There was a problem hiding this comment.
WARNING: Test-only comment - needs real review text.
| return configuration.Manifests | ||
| .SelectMany(m => (m.Files ?? []).Select(f => new { File = f, Manifest = m })) | ||
| .SelectMany((m, index) => (m.Files ?? []).Select(f => new { File = f, Manifest = m, ManifestIndex = index })) | ||
| .GroupBy(x => x.File.RelativePath, StringComparer.OrdinalIgnoreCase) |
There was a problem hiding this comment.
WARNING: GroupBy(..., StringComparer.OrdinalIgnoreCase) deduplicates by case-insensitive path. On Linux/macOS, file systems are case-sensitive, so distinct files such as Config.ini and config.ini would be wrongly merged and one would be dropped from every workspace strategy that consumes this list. The grouping should follow the case sensitivity of the host file system (e.g. StringComparer.Ordinal on Unix, OrdinalIgnoreCase on Windows).
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| .Where(x => x.File.InstallTarget == GenHub.Core.Models.Enums.ContentInstallTarget.Workspace) | ||
| .SelectMany((m, index) => (m.Files ?? []).Select(f => new { File = f, Manifest = m, ManifestIndex = index })) | ||
| .Where(x => x.File.InstallTarget == ContentInstallTarget.Workspace) | ||
| .GroupBy(x => x.File.RelativePath, StringComparer.OrdinalIgnoreCase) |
There was a problem hiding this comment.
WARNING: Same case-insensitive grouping concern as above in GetWorkspaceUniqueFiles. On Linux/macOS this drops legitimately distinct paths that only differ in letter case. The same fix (host-aware comparer) should be applied here.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| .Select(file => new { File = file, Manifest = manifest, ManifestIndex = index })) | ||
| .GroupBy(x => x.File.RelativePath, StringComparer.OrdinalIgnoreCase) | ||
| .Select(g => g | ||
| .OrderByDescending(x => ContentTypePriority.GetPriority(x.Manifest.ContentType)) |
There was a problem hiding this comment.
WARNING: When two manifests share the same ContentType priority, the tie-breaker is ThenByDescending(x => x.ManifestIndex) which silently picks the LAST manifest. This is deterministic only when the manifests collection order is stable, but it is also undocumented: a higher-priority Mod placed after a lower-priority Mod would still be lost. Either document the rule or pick by an explicit source priority (manifest id/version) so callers can reason about the outcome.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| .IsInRole(System.Security.Principal.WindowsBuiltInRole.Administrator); | ||
|
|
||
| if (!isWindows || !isAdmin) | ||
| if (isWindows && !isAdmin) |
There was a problem hiding this comment.
SUGGESTION: Using a runtime if (isWindows && !isAdmin) return; silently swallows the test on Windows non-admin without informing xUnit. Prefer Skip = ... on the [Fact] attribute (or [SkippableFact]) so the skipped status is visible in test reports rather than reported as a passing green test.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| return configuration.Manifests | ||
| .SelectMany(m => (m.Files ?? []).Select(f => new { File = f, Manifest = m })) | ||
| .SelectMany((m, index) => (m.Files ?? []).Select(f => new { File = f, Manifest = m, ManifestIndex = index })) | ||
| .GroupBy(x => x.File.RelativePath, StringComparer.OrdinalIgnoreCase) |
There was a problem hiding this comment.
WARNING: (Replaces earlier placeholder.) GroupBy(..., StringComparer.OrdinalIgnoreCase) deduplicates by case-insensitive path. On Linux/macOS, file systems are case-sensitive, so distinct files such as Config.ini and config.ini would be wrongly merged and one would be dropped from every workspace strategy that consumes this list. The grouping should follow the case sensitivity of the host file system (e.g. StringComparer.Ordinal on Unix, OrdinalIgnoreCase on Windows).
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: 14 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (9 files)
Fix these issues in Kilo Cloud Reviewed by minimax-m3:free · Input: 96.1K · Output: 11.8K · Cached: 2.3M |
1 participant
Summary
Resolves #42 by ensuring custom files (mods, patches, game clients, addons, language packs, maps, modding tools) take strict precedence over base game installation data across all workspace strategies (
FullCopyStrategy,HardLinkStrategy,HybridCopySymlinkStrategy,SymlinkOnlyStrategy), regardless of manifest enumeration order. Workspaces in user directories remain completely isolated from the original game installation so base files are never modified, and launching requires no administrative privileges.Root Cause
Workspace strategies previously processed manifest files in arbitrary or collection-dependent order, and in some strategies (
HybridCopySymlinkStrategy) looped over manifests in sequence without global conflict deduplication. This allowed base game installation files to overwrite mod or patch files when manifests were structured with the mod first, or caused duplicate progress and disk estimation. Additionally, integration tests on Linux and macOS had false-negative admin skip conditions (!isWindows || !isAdmin) which prevented symlink/hybrid workspace tests from executing on Unix platforms.Changes
WorkspaceConfigurationExtensions): AddedGetPrioritizedWorkspaceFilesextension method to deduplicate workspace files with deterministic, priority-ordered selection usingContentTypePriority(Mod > Patch > GameClient > ModdingTool > Addon > LanguagePack > Map > GameInstallation).FullCopyStrategy,HardLinkStrategy,HybridCopySymlinkStrategy,SymlinkOnlyStrategy): Standardized all four strategies to useGetPrioritizedWorkspaceFiles(), processing only winning files once, eliminating redundant disk overwrites, and calculating accurate deduplicated disk usage and progress.WorkspacePrioritizationVerifyTests,WorkspaceIntegrationTests,GameProfileWorkspaceIntegrationTest,WorkspaceCasIntegrationTests):(!isWindows || !isAdmin)to(isWindows && !isAdmin)so Linux and macOS properly run full symlink and hybrid workspace integration tests.Verification
this., and zero magic constants rulesCloses #42
Created with Gemini 3.7 Flash via Antigravity