-
Notifications
You must be signed in to change notification settings - Fork 20
fix(workspace): prioritize custom files over installation data and isolate user workspaces #429
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,7 @@ | ||
| using System; | ||
| using System.Collections.Generic; | ||
| using System.Linq; | ||
| using GenHub.Core.Models.Enums; | ||
| using GenHub.Core.Models.Manifest; | ||
| using GenHub.Core.Models.Workspace; | ||
|
|
||
|
|
@@ -11,34 +14,74 @@ public static class WorkspaceConfigurationExtensions | |
| { | ||
| /// <summary> | ||
| /// Gets all unique files from all manifests, deduplicated by relative path. | ||
| /// When multiple manifests contain the same file path, returns the first occurrence. | ||
| /// When multiple manifests contain the same file path, returns the file from the highest priority manifest. | ||
| /// </summary> | ||
| /// <param name="configuration">The workspace configuration to get files from.</param> | ||
| /// <returns>An enumerable of unique manifest files.</returns> | ||
| public static IEnumerable<ManifestFile> GetAllUniqueFiles( | ||
| this WorkspaceConfiguration configuration) | ||
| { | ||
| if (configuration?.Manifests is null || configuration.Manifests.Count == 0) | ||
| { | ||
| return []; | ||
| } | ||
|
|
||
| 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. WARNING: Test-only comment - needs real review text. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. WARNING: Reply with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. WARNING: (Replaces earlier placeholder.) Reply with |
||
| .Select(g => g.OrderByDescending(x => ContentTypePriority.GetPriority(x.Manifest.ContentType)) | ||
| .ThenByDescending(x => x.ManifestIndex) | ||
|
Comment on lines
+30
to
+33
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 1. getalluniquefiles exposes deferred sequence 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
|
||
| .First().File); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Gets all unique files intended for the workspace from all manifests, deduplicated by relative path. | ||
| /// Only includes files where <see cref="ManifestFile.InstallTarget"/> is <see cref="GenHub.Core.Models.Enums.ContentInstallTarget.Workspace"/>. | ||
| /// Only includes files where <see cref="ManifestFile.InstallTarget"/> is <see cref="ContentInstallTarget.Workspace"/>. | ||
| /// Higher-priority manifests (such as mods and patches) take precedence over lower-priority manifests (such as base game installations). | ||
| /// </summary> | ||
| /// <param name="configuration">The workspace configuration to get files from.</param> | ||
| /// <returns>An enumerable of unique workspace-specific manifest files.</returns> | ||
| public static IEnumerable<ManifestFile> GetWorkspaceUniqueFiles( | ||
| this WorkspaceConfiguration configuration) | ||
| { | ||
| if (configuration?.Manifests is null || configuration.Manifests.Count == 0) | ||
| { | ||
| return []; | ||
| } | ||
|
|
||
| return configuration.Manifests | ||
| .SelectMany(m => (m.Files ?? []).Select(f => new { File = f, Manifest = m })) | ||
| .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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. test There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. WARNING: Same case-insensitive grouping concern as above in Reply with |
||
| .Select(g => g.OrderByDescending(x => ContentTypePriority.GetPriority(x.Manifest.ContentType)) | ||
|
Comment on lines
+53
to
56
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 2. getworkspaceuniquefiles remains deferred 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
|
||
| .ThenByDescending(x => x.ManifestIndex) | ||
| .First().File); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Gets all unique workspace files paired with their owning manifest, deduplicated by relative path | ||
| /// and resolved by content type priority (higher-priority content types override lower-priority ones). | ||
| /// </summary> | ||
| /// <param name="configuration">The workspace configuration to get prioritized files from.</param> | ||
| /// <returns>A read-only list of prioritized file and manifest pairs.</returns> | ||
| public static IReadOnlyList<(ManifestFile File, ContentManifest Manifest)> GetPrioritizedWorkspaceFiles( | ||
| this WorkspaceConfiguration configuration) | ||
| { | ||
| if (configuration?.Manifests is null || configuration.Manifests.Count == 0) | ||
| { | ||
| return []; | ||
| } | ||
|
|
||
| return configuration.Manifests | ||
| .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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 6. Unix case-distinct files collapse 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
|
||
| .Select(g => g | ||
| .OrderByDescending(x => ContentTypePriority.GetPriority(x.Manifest.ContentType)) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. test There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. WARNING: When two manifests share the same Reply with |
||
| .ThenByDescending(x => x.ManifestIndex) | ||
| .First()) | ||
|
Comment on lines
+79
to
+83
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 5. Missing winner drops fallback 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
|
||
| .Select(x => (x.File, x.Manifest)) | ||
| .ToList(); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -163,14 +163,14 @@ public async Task PrepareWorkspace_FullCopyStrategy_CopiesGameInstallationAndCli | |
| [Fact] | ||
| public async Task PrepareWorkspace_SymlinkStrategy_LinksGameInstallationAndClientFilesAsync() | ||
| { | ||
| // Skip on systems that don't support symlinks | ||
| // Skip on Windows if not running with administrator privileges | ||
| bool isWindows = OperatingSystem.IsWindows(); | ||
| bool isAdmin = isWindows && | ||
| new System.Security.Principal.WindowsPrincipal( | ||
| System.Security.Principal.WindowsIdentity.GetCurrent()) | ||
| .IsInRole(System.Security.Principal.WindowsBuiltInRole.Administrator); | ||
|
|
||
| if (!isWindows || !isAdmin) | ||
| if (isWindows && !isAdmin) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. SUGGESTION: Reply with |
||
| { | ||
| return; | ||
| } | ||
|
|
@@ -228,7 +228,7 @@ public async Task PrepareWorkspace_MixedContentTypes_HandlesGameInstallationAndG | |
| System.Security.Principal.WindowsIdentity.GetCurrent()) | ||
| .IsInRole(System.Security.Principal.WindowsBuiltInRole.Administrator); | ||
|
|
||
| if (!isWindows || !isAdmin) | ||
| if (isWindows && !isAdmin) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. SUGGESTION: Same runtime skip concern — prefer Reply with |
||
| { | ||
| return; | ||
| } | ||
|
|
@@ -336,7 +336,7 @@ public async Task PrepareWorkspace_AllStrategies_HandleGameInstallationFilesAsyn | |
|
|
||
| // 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. SUGGESTION: Same skip-pattern observation as above for the parametrized strategy test. Reply with |
||
| { | ||
| return; | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -114,7 +114,7 @@ public async Task EndToEndWorkspaceCreation_AllStrategiesAsync(WorkspaceStrategy | |
|
|
||
| if ((strategy == WorkspaceStrategy.SymlinkOnly || | ||
| strategy == WorkspaceStrategy.HybridCopySymlink) && | ||
| (!isWindows || !isAdmin)) | ||
| isWindows && !isAdmin) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. SUGGESTION: Same observation as Reply with |
||
| { | ||
| return; | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
test