feat(installations): detect retail installations by archive presence - #352
feat(installations): detect retail installations by archive presence#352bobtista wants to merge 3 commits into
Conversation
… not executable name
…bling archives in combined-directory validation
📝 WalkthroughSummary by CodeRabbit
WalkthroughRetail installation detection now uses canonical archive presence instead of executable names. Combined Generals and Zero Hour directories receive typed manifests and validation. Windows and macOS detection support flat roots and native deployment paths. Client discovery also recognizes extensionless native binaries. ChangesRetail detection and validation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant MacOSInstallationDetector
participant RetailArchiveClassifier
participant GameInstallation
MacOSInstallationDetector->>RetailArchiveClassifier: Classify candidate root archives
RetailArchiveClassifier-->>MacOSInstallationDetector: Return archive flags
MacOSInstallationDetector->>GameInstallation: Create installation from detected paths
GameInstallation->>RetailArchiveClassifier: Classify each game path
RetailArchiveClassifier-->>GameInstallation: Return Generals and Zero Hour flags
Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
📋 Issue PlannerBuilt with CodeRabbit's Coding Plans for faster development and fewer bugs. View plan used: ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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
added
Enhancement
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if (!hasZeroHour && | ||
| archiveName.EndsWith(RetailArchiveConstants.ZeroHourArchiveSuffix, StringComparison.OrdinalIgnoreCase)) | ||
| { | ||
| hasZeroHour = true; |
There was a problem hiding this comment.
When a broad macOS search root such as Downloads contains a community archive like 340_ControlBarProZH.big, this suffix check marks the entire directory as a Zero Hour retail installation, causing unrelated files to be cached and processed as game content.
Knowledge Base Used: Game Detection: Installations and Client Identification
Prompt To Fix With AI
This is a comment left during a code review.
Path: GenHub/GenHub.Core/Helpers/RetailArchiveClassifier.cs
Line: 57-60
Comment:
**Non-retail archives trigger installation detection**
When a broad macOS search root such as Downloads contains a community archive like `340_ControlBarProZH.big`, this suffix check marks the entire directory as a Zero Hour retail installation, causing unrelated files to be cached and processed as game content.
**Knowledge Base Used:** [Game Detection: Installations and Client Identification](https://app.greptile.com/genhub/-/custom-context/knowledge-base/community-outpost/genhub/-/docs/game-detection.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| if (isCombinedDirectory) | ||
| { | ||
| var generalsVersion = new GameClient | ||
| { | ||
| Name = $"Generals {version}", | ||
| Id = string.Empty, // Set later by manifest | ||
| Version = version, | ||
| ExecutablePath = actualExePath, | ||
| GameType = GameType.Generals, | ||
| InstallationId = inst.Id, | ||
| WorkingDirectory = inst.GeneralsPath, | ||
| }; | ||
| await GenerateClientManifestAndSetIdAsync(generalsVersion, inst.GeneralsPath, inst, GameType.Generals); | ||
| gameClients.Add(generalsVersion); | ||
| logger.LogDebug( | ||
| "Skipping standard Generals client scan for {InstallationId}: {GeneralsPath} is a combined directory scanned once as Zero Hour", | ||
| inst.Id, | ||
| inst.GeneralsPath); | ||
| } |
There was a problem hiding this comment.
When both games share one directory, this branch skips creation of the standard Generals client while still creating the Zero Hour client, causing automatic Generals profile creation and Generals manifest or dependency resolution to fail despite Generals being detected.
Knowledge Base Used: Game Detection: Installations and Client Identification
Prompt To Fix With AI
This is a comment left during a code review.
Path: GenHub/GenHub/Features/GameClients/GameClientDetector.cs
Line: 72-78
Comment:
**Combined installs lose Generals client**
When both games share one directory, this branch skips creation of the standard Generals client while still creating the Zero Hour client, causing automatic Generals profile creation and Generals manifest or dependency resolution to fail despite Generals being detected.
**Knowledge Base Used:** [Game Detection: Installations and Client Identification](https://app.greptile.com/genhub/-/custom-context/knowledge-base/community-outpost/genhub/-/docs/game-detection.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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 `@GenHub/GenHub.Core/Models/GameInstallations/GameInstallation.cs`:
- Around line 248-269: The archive classification safety logic is duplicated
across two callers. In
GenHub/GenHub.Core/Models/GameInstallations/GameInstallation.cs#L248-L269,
replace ClassifyArchivesSafely with a shared
RetailArchiveClassifier.TryClassifyArchives(string path, out
RetailArchiveClassification classification) call while preserving SetPaths’s
existing warning log; in
GenHub/GenHub.Windows/GameInstallations/WindowsInstallationDetector.cs#L314-L331,
remove the duplicate helper and call the shared method from
DetectRetailInstallations, allowing each caller to retain its own warning
behavior.
In `@GenHub/GenHub.Windows/GameInstallations/WindowsInstallationDetector.cs`:
- Around line 222-243: Update the IsCombinedDirectory branch to skip the
installation only when combinedPath exists in both seenGeneralsPaths and
seenZeroHourPaths; retain it when only one game has already been claimed,
matching the non-combined deduplication behavior.
In `@GenHub/GenHub/Features/GameInstallations/InstallationPathResolver.cs`:
- Around line 97-111: Update IsValidGameInstallationAsync, which filters
candidates for SearchForInstallationAsync, to validate Generals and Zero Hour
directories using RetailArchiveClassifier.ClassifyArchives and the corresponding
archive flags, matching the archive-based validation in
ResolveInstallationPathAsync. Remove the requirement that generals.exe must
exist so moved native installations can be recovered.
In `@GenHub/GenHub/Features/Validation/GameInstallationValidator.cs`:
- Around line 82-88: The combined-directory predicate is duplicated across two
files; extract it into one shared helper on GameInstallation or as an extension,
preserving both-game flags, non-empty paths, normalized full-path comparison,
and case-insensitive matching. Update GameInstallationValidator.cs lines 82-88
and GameClientDetector.cs lines 64-68 to call the shared helper instead of
computing the predicate inline.
- Line 119: Update the nested content validation call in the outer validation
loop to prevent ContentValidator.ValidateAllAsync from reporting its own Total=3
values through the outer progress subscriber. Pass null for progress, or use an
adapter that rescales nested ValidationProgress reports to the outer totalSteps
scale while preserving the outer progress contract.
- Around line 108-124: Remove the separate
contentValidator.ValidateManifestAsync call and its
manifestValidationResult.Issues addition from the validation flow. Keep
ValidateAllAsync as the single manifest and content validation entry point,
preserving the existing progress reporting and content-issue filtering.
- Around line 140-143: Update the required-directory validation in the manifest
validation flow so ValidateDirectoriesAsync runs whenever requiredDirs contains
entries, including the fallback case where gameType is null. Preserve the
existing requiredDirs guard and ensure fallback manifests still report missing
Data or Maps directories.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 9b2c81ba-210f-469b-aaa6-e6f188e7a753
📒 Files selected for processing (20)
GenHub/GenHub.Core/Constants/GameClientConstants.csGenHub/GenHub.Core/Constants/RetailArchiveConstants.csGenHub/GenHub.Core/Helpers/RetailArchiveClassifier.csGenHub/GenHub.Core/Interfaces/Manifest/IManifestProvider.csGenHub/GenHub.Core/Models/GameInstallations/GameInstallation.csGenHub/GenHub.Core/Models/GameInstallations/RetailArchiveClassification.csGenHub/GenHub.MacOS/GameInstallations/MacOSInstallationDetector.csGenHub/GenHub.Tests/GenHub.Tests.Core/Features/GameClients/GameClientDetectorTests.csGenHub/GenHub.Tests/GenHub.Tests.Core/Features/Manifest/ManifestProviderTests.csGenHub/GenHub.Tests/GenHub.Tests.Core/Features/Validation/GameInstallationValidatorTests.csGenHub/GenHub.Tests/GenHub.Tests.Core/Helpers/RetailArchiveClassifierTests.csGenHub/GenHub.Tests/GenHub.Tests.Core/Models/GameInstallations/GameInstallationTests.csGenHub/GenHub.Tests/GenHub.Tests.MacOS/GameInstallations/MacOSInstallationDetectorTests.csGenHub/GenHub.Windows/GameInstallations/WindowsInstallationDetector.csGenHub/GenHub/Features/Content/Services/ContentValidator.csGenHub/GenHub/Features/Content/Services/Publishers/SuperHackersManifestFactory.csGenHub/GenHub/Features/GameClients/GameClientDetector.csGenHub/GenHub/Features/GameInstallations/InstallationPathResolver.csGenHub/GenHub/Features/Manifest/ManifestProvider.csGenHub/GenHub/Features/Validation/GameInstallationValidator.cs
| /// <summary> | ||
| /// Classifies a directory's archives without letting a filesystem error escape. | ||
| /// </summary> | ||
| /// <param name="path">The directory to classify.</param> | ||
| /// <returns>The classification, or neither game when the directory cannot be read.</returns> | ||
| /// <remarks> | ||
| /// <see cref="SetPaths"/> is called from every platform detector, so it must not | ||
| /// throw. An unreadable directory is logged rather than silently reading as "no | ||
| /// archives" — the flag still ends up false, but the log names the real cause. | ||
| /// </remarks> | ||
| private RetailArchiveClassification ClassifyArchivesSafely(string path) | ||
| { | ||
| var possibleExes = new[] { GameClientConstants.SteamGameDatExecutable, GameClientConstants.GeneralsExecutable, GameClientConstants.ZeroHourExecutable }; | ||
| return possibleExes.Any(exe => Path.Combine(path, exe).FileExistsCaseInsensitive()); | ||
| try | ||
| { | ||
| return RetailArchiveClassifier.ClassifyArchives(path); | ||
| } | ||
| catch (Exception ex) when (ex is UnauthorizedAccessException or IOException) | ||
| { | ||
| _logger?.LogWarning(ex, "Could not read {Path} while classifying retail archives; treating it as holding none", path); | ||
| return default; | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Duplicated ClassifyArchivesSafely in two files. Both files define the same private helper with the same UnauthorizedAccessException or IOException filter, because RetailArchiveClassifier exposes no safe entry point. Add one, for example RetailArchiveClassifier.TryClassifyArchives(string path, out RetailArchiveClassification classification), and let each caller log its own warning.
GenHub/GenHub.Core/Models/GameInstallations/GameInstallation.cs#L248-L269: replace the private helper with the shared classifier method and keep the existing warning log inSetPaths.GenHub/GenHub.Windows/GameInstallations/WindowsInstallationDetector.cs#L314-L331: delete the private helper and call the shared classifier method fromDetectRetailInstallations.
📍 Affects 2 files
GenHub/GenHub.Core/Models/GameInstallations/GameInstallation.cs#L248-L269(this comment)GenHub/GenHub.Windows/GameInstallations/WindowsInstallationDetector.cs#L314-L331
🤖 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 `@GenHub/GenHub.Core/Models/GameInstallations/GameInstallation.cs` around lines
248 - 269, The archive classification safety logic is duplicated across two
callers. In
GenHub/GenHub.Core/Models/GameInstallations/GameInstallation.cs#L248-L269,
replace ClassifyArchivesSafely with a shared
RetailArchiveClassifier.TryClassifyArchives(string path, out
RetailArchiveClassification classification) call while preserving SetPaths’s
existing warning log; in
GenHub/GenHub.Windows/GameInstallations/WindowsInstallationDetector.cs#L314-L331,
remove the duplicate helper and call the shared method from
DetectRetailInstallations, allowing each caller to retain its own warning
behavior.
| // A combined directory — both games flagged at the same path — is one unit. | ||
| // Splitting it across sources by clearing whichever game another source | ||
| // already claimed would leave the same directory owned by two installations | ||
| // and scanned twice for clients, so it is kept whole or dropped whole. | ||
| if (IsCombinedDirectory(installation)) | ||
| { | ||
| var combinedPath = Path.GetFullPath(installation.GeneralsPath); | ||
| if (seenGeneralsPaths.Contains(combinedPath) || seenZeroHourPaths.Contains(combinedPath)) | ||
| { | ||
| logger.LogWarning( | ||
| "Skipping combined {InstallationType} installation at {CombinedPath} (directory already detected from another source)", | ||
| installation.InstallationType, | ||
| combinedPath); | ||
| continue; | ||
| } | ||
|
|
||
| seenGeneralsPaths.Add(combinedPath); | ||
| seenZeroHourPaths.Add(combinedPath); | ||
| deduplicated.Add(installation); | ||
| continue; | ||
| } | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
A partially claimed combined directory loses one game.
The branch drops the combined installation when either seen-path set already contains the directory. If a higher-priority source claimed only Generals at that directory, Zero Hour is then reported by no installation. The non-combined branch below keeps the unique game in the same situation, so the two branches disagree.
Skip only when both games are already claimed.
🐛 Proposed fix
if (IsCombinedDirectory(installation))
{
var combinedPath = Path.GetFullPath(installation.GeneralsPath);
- if (seenGeneralsPaths.Contains(combinedPath) || seenZeroHourPaths.Contains(combinedPath))
+ if (seenGeneralsPaths.Contains(combinedPath) && seenZeroHourPaths.Contains(combinedPath))
{
logger.LogWarning(
"Skipping combined {InstallationType} installation at {CombinedPath} (directory already detected from another source)",
installation.InstallationType,
combinedPath);
continue;
}📝 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.
| // A combined directory — both games flagged at the same path — is one unit. | |
| // Splitting it across sources by clearing whichever game another source | |
| // already claimed would leave the same directory owned by two installations | |
| // and scanned twice for clients, so it is kept whole or dropped whole. | |
| if (IsCombinedDirectory(installation)) | |
| { | |
| var combinedPath = Path.GetFullPath(installation.GeneralsPath); | |
| if (seenGeneralsPaths.Contains(combinedPath) || seenZeroHourPaths.Contains(combinedPath)) | |
| { | |
| logger.LogWarning( | |
| "Skipping combined {InstallationType} installation at {CombinedPath} (directory already detected from another source)", | |
| installation.InstallationType, | |
| combinedPath); | |
| continue; | |
| } | |
| seenGeneralsPaths.Add(combinedPath); | |
| seenZeroHourPaths.Add(combinedPath); | |
| deduplicated.Add(installation); | |
| continue; | |
| } | |
| // A combined directory — both games flagged at the same path — is one unit. | |
| // Splitting it across sources by clearing whichever game another source | |
| // already claimed would leave the same directory owned by two installations | |
| // and scanned twice for clients, so it is kept whole or dropped whole. | |
| if (IsCombinedDirectory(installation)) | |
| { | |
| var combinedPath = Path.GetFullPath(installation.GeneralsPath); | |
| if (seenGeneralsPaths.Contains(combinedPath) && seenZeroHourPaths.Contains(combinedPath)) | |
| { | |
| logger.LogWarning( | |
| "Skipping combined {InstallationType} installation at {CombinedPath} (directory already detected from another source)", | |
| installation.InstallationType, | |
| combinedPath); | |
| continue; | |
| } | |
| seenGeneralsPaths.Add(combinedPath); | |
| seenZeroHourPaths.Add(combinedPath); | |
| deduplicated.Add(installation); | |
| continue; | |
| } |
🤖 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 `@GenHub/GenHub.Windows/GameInstallations/WindowsInstallationDetector.cs`
around lines 222 - 243, Update the IsCombinedDirectory branch to skip the
installation only when combinedPath exists in both seenGeneralsPaths and
seenZeroHourPaths; retain it when only one game has already been claimed,
matching the non-combined deduplication behavior.
| // Check if it still holds the games' retail archives — the same signal that | ||
| // flagged the games at detection time, so a native install whose binary is | ||
| // extensionless does not read as vanished. | ||
| var hasValidFiles = false; | ||
|
|
||
| if (installation.HasGenerals && !string.IsNullOrEmpty(installation.GeneralsPath)) | ||
| if (installation.HasGenerals && !string.IsNullOrEmpty(installation.GeneralsPath) | ||
| && RetailArchiveClassifier.ClassifyArchives(installation.GeneralsPath).HasGeneralsArchives) | ||
| { | ||
| var generalsExe = Path.Combine(installation.GeneralsPath, "generals.exe"); | ||
| if (File.Exists(generalsExe)) | ||
| { | ||
| hasValidFiles = true; | ||
| } | ||
| hasValidFiles = true; | ||
| } | ||
|
|
||
| if (installation.HasZeroHour && !string.IsNullOrEmpty(installation.ZeroHourPath)) | ||
| if (installation.HasZeroHour && !string.IsNullOrEmpty(installation.ZeroHourPath) | ||
| && RetailArchiveClassifier.ClassifyArchives(installation.ZeroHourPath).HasZeroHourArchives) | ||
| { | ||
| var zhExe = Path.Combine(installation.ZeroHourPath, "generals.exe"); | ||
| if (File.Exists(zhExe)) | ||
| { | ||
| hasValidFiles = true; | ||
| } | ||
| hasValidFiles = true; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Path recovery still requires generals.exe, so native installs cannot be re-resolved.
Validation now accepts a directory based on retail archives. When validation fails, ResolveInstallationPathAsync calls SearchForInstallationAsync, and its candidate filter IsValidGameInstallationAsync (Lines 295-300) returns false unless generals.exe exists. A moved native installation therefore validates as invalid and is never recovered, which is the case this PR targets.
Apply the same archive classification in the candidate filter.
🤖 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 `@GenHub/GenHub/Features/GameInstallations/InstallationPathResolver.cs` around
lines 97 - 111, Update IsValidGameInstallationAsync, which filters candidates
for SearchForInstallationAsync, to validate Generals and Zero Hour directories
using RetailArchiveClassifier.ClassifyArchives and the corresponding archive
flags, matching the archive-based validation in ResolveInstallationPathAsync.
Remove the requirement that generals.exe must exist so moved native
installations can be recovered.
| var isCombinedDirectory = installation.HasGenerals | ||
| && installation.HasZeroHour | ||
| && !string.IsNullOrEmpty(installation.GeneralsPath) | ||
| && !string.IsNullOrEmpty(installation.ZeroHourPath) | ||
| && Path.GetFullPath(installation.GeneralsPath).Equals( | ||
| Path.GetFullPath(installation.ZeroHourPath), | ||
| StringComparison.OrdinalIgnoreCase); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Combined-directory detection is duplicated identically across two files. Both sites compute the same "both games flagged at the same normalized path" predicate independently. This is one predicate, not two; if it changes in one file without the other, validation and client detection can disagree about which directories are combined.
GenHub/GenHub/Features/Validation/GameInstallationValidator.cs#L82-L88: extract this computation into a shared helper (for example anIsCombinedRetailDirectoryextension or method onGameInstallation) and call it here.GenHub/GenHub/Features/GameClients/GameClientDetector.cs#L64-L68: call the same shared helper here instead of recomputing the predicate inline.
📍 Affects 2 files
GenHub/GenHub/Features/Validation/GameInstallationValidator.cs#L82-L88(this comment)GenHub/GenHub/Features/GameClients/GameClientDetector.cs#L64-L68
🤖 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 `@GenHub/GenHub/Features/Validation/GameInstallationValidator.cs` around lines
82 - 88, The combined-directory predicate is duplicated across two files;
extract it into one shared helper on GameInstallation or as an extension,
preserving both-game flags, non-empty paths, normalized full-path comparison,
and case-insensitive matching. Update GameInstallationValidator.cs lines 82-88
and GameClientDetector.cs lines 64-68 to call the shared helper instead of
computing the predicate inline.
|
|
||
| // Installation-specific validations (directories, etc.) | ||
| var requiredDirs = manifest.RequiredDirectories ?? Enumerable.Empty<string>(); | ||
| if (requiredDirs.Any()) | ||
| { | ||
| if (installation.HasGenerals) | ||
| progress?.Report(new ValidationProgress(++currentStep, totalSteps, "Core manifest validation")); | ||
|
|
||
| var manifestValidationResult = await contentValidator.ValidateManifestAsync(manifest, cancellationToken); | ||
| issues.AddRange(manifestValidationResult.Issues); | ||
|
|
||
| progress?.Report(new ValidationProgress(++currentStep, totalSteps, "Validating content files")); | ||
|
|
||
| // Use ContentValidator for full content validation (integrity + extraneous files) | ||
| try | ||
| { | ||
| var fullValidation = await contentValidator.ValidateAllAsync(sourcePath, manifest, progress, cancellationToken); | ||
| var contentIssues = isCombinedDirectory && gameType is not null | ||
| ? fullValidation.Issues.Where(issue => !IsSiblingGameRootArchive(issue, gameType.Value)) | ||
| : fullValidation.Issues; | ||
| issues.AddRange(contentIssues); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Manifest structure is validated twice per pass.
ContentValidator.ValidateManifestAsync calls ValidateManifestStructure(manifest) and returns those issues (Line 111-112). ContentValidator.ValidateAllAsync also calls ValidateManifestStructure(manifest) as its own first step, and its result is added via contentIssues at Line 123. The isCombinedDirectory filter at Line 120-121 only removes UnexpectedFile issues; it does not deduplicate manifest-structure issues. Every manifest-structure problem (missing Id, Name, Version, empty Files, and so on) is added to issues twice per pass, and up to four times total for a combined installation with two passes.
Remove the separate ValidateManifestAsync call, since ValidateAllAsync already performs the same structural check.
🐛 Proposed fix
- progress?.Report(new ValidationProgress(++currentStep, totalSteps, "Core manifest validation"));
-
- var manifestValidationResult = await contentValidator.ValidateManifestAsync(manifest, cancellationToken);
- issues.AddRange(manifestValidationResult.Issues);
-
+ progress?.Report(new ValidationProgress(++currentStep, totalSteps, "Core manifest validation"));
+
progress?.Report(new ValidationProgress(++currentStep, totalSteps, "Validating content files"));Do you want me to also add a regression test that uses the real ContentValidator with a manifest missing Name, to confirm the issue count is 1, not 2?
📝 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.
| // Installation-specific validations (directories, etc.) | |
| var requiredDirs = manifest.RequiredDirectories ?? Enumerable.Empty<string>(); | |
| if (requiredDirs.Any()) | |
| { | |
| if (installation.HasGenerals) | |
| progress?.Report(new ValidationProgress(++currentStep, totalSteps, "Core manifest validation")); | |
| var manifestValidationResult = await contentValidator.ValidateManifestAsync(manifest, cancellationToken); | |
| issues.AddRange(manifestValidationResult.Issues); | |
| progress?.Report(new ValidationProgress(++currentStep, totalSteps, "Validating content files")); | |
| // Use ContentValidator for full content validation (integrity + extraneous files) | |
| try | |
| { | |
| var fullValidation = await contentValidator.ValidateAllAsync(sourcePath, manifest, progress, cancellationToken); | |
| var contentIssues = isCombinedDirectory && gameType is not null | |
| ? fullValidation.Issues.Where(issue => !IsSiblingGameRootArchive(issue, gameType.Value)) | |
| : fullValidation.Issues; | |
| issues.AddRange(contentIssues); | |
| } | |
| progress?.Report(new ValidationProgress(++currentStep, totalSteps, "Core manifest validation")); | |
| progress?.Report(new ValidationProgress(++currentStep, totalSteps, "Validating content files")); | |
| // Use ContentValidator for full content validation (integrity + extraneous files) | |
| try | |
| { | |
| var fullValidation = await contentValidator.ValidateAllAsync(sourcePath, manifest, progress, cancellationToken); | |
| var contentIssues = isCombinedDirectory && gameType is not null | |
| ? fullValidation.Issues.Where(issue => !IsSiblingGameRootArchive(issue, gameType.Value)) | |
| : fullValidation.Issues; | |
| issues.AddRange(contentIssues); | |
| } |
🤖 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 `@GenHub/GenHub/Features/Validation/GameInstallationValidator.cs` around lines
108 - 124, Remove the separate contentValidator.ValidateManifestAsync call and
its manifestValidationResult.Issues addition from the validation flow. Keep
ValidateAllAsync as the single manifest and content validation entry point,
preserving the existing progress reporting and content-issue filtering.
| // Use ContentValidator for full content validation (integrity + extraneous files) | ||
| try | ||
| { | ||
| var fullValidation = await contentValidator.ValidateAllAsync(sourcePath, manifest, progress, cancellationToken); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Nested ValidateAllAsync progress reports use a different Total than the outer loop.
This loop reports progress against totalSteps = passes.Count * 4 (Line 91). Line 119 forwards the same progress instance into contentValidator.ValidateAllAsync. ContentValidator.ValidateAllAsync reports its own progress on a Total = 3 scale (GenHub/GenHub/Features/Content/Services/ContentValidator.cs, lines 69-84). A single IProgress<ValidationProgress> subscriber receives interleaved reports with inconsistent Total values within the same validation run. The existing test ValidateAsync_WithProgressCallback_ReportsProgress asserts "All progress reports should have the same total. Found totals: [{string.Join(", ", allTotals)}]", so this contract is expected to hold; it currently only passes because that test mocks IContentValidator and never triggers the nested reports.
Pass null for progress in the ValidateAllAsync call, or wrap progress in an adapter that rescales nested reports into the outer step range.
🐛 Proposed minimal fix
- var fullValidation = await contentValidator.ValidateAllAsync(sourcePath, manifest, progress, cancellationToken);
+ var fullValidation = await contentValidator.ValidateAllAsync(sourcePath, manifest, null, cancellationToken);📝 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.
| var fullValidation = await contentValidator.ValidateAllAsync(sourcePath, manifest, progress, cancellationToken); | |
| var fullValidation = await contentValidator.ValidateAllAsync(sourcePath, manifest, null, cancellationToken); |
🤖 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 `@GenHub/GenHub/Features/Validation/GameInstallationValidator.cs` at line 119,
Update the nested content validation call in the outer validation loop to
prevent ContentValidator.ValidateAllAsync from reporting its own Total=3 values
through the outer progress subscriber. Pass null for progress, or use an adapter
that rescales nested ValidationProgress reports to the outer totalSteps scale
while preserving the outer progress contract.
| var requiredDirs = manifest.RequiredDirectories ?? Enumerable.Empty<string>(); | ||
| if (requiredDirs.Any() && gameType is not null) | ||
| { | ||
| progress?.Report(new ValidationProgress(++currentStep, totalSteps, "Validating Zero Hour directories")); | ||
| issues.AddRange(await ValidateDirectoriesAsync(installation.ZeroHourPath, requiredDirs, cancellationToken)); | ||
| issues.AddRange(await ValidateDirectoriesAsync(sourcePath, requiredDirs, cancellationToken)); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## Locate file"
fd -a 'GameInstallationValidator\.cs$' . || true
echo "## Relevant file outline/size"
file="$(fd 'GameInstallationValidator\.cs$' GenHub 2>/dev/null | head -n1 || true)"
if [ -n "${file:-}" ]; then
wc -l "$file"
ast-grep outline "$file" || true
echo "## Relevant sections"
sed -n '1,220p' "$file" | cat -n
fiRepository: community-outpost/GenHub
Length of output: 10885
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## Manifest and validator interfaces"
sed -n '1,220p' GenHub.Core/Interfaces/Manifest/IManifestProvider.cs | cat -n || true
sed -n '1,220p' GenHub.Core/Interfaces/Manifest/IManifest.cs | cat -n || true
sed -n '1,180p' GenHub.Core/Interfaces/Validation/IGameInstallationValidator.cs | cat -n || true
echo "## RequiredDirectories occurrences"
rg -n "RequiredDirectories|ValidateDirectoriesAsync|DirectoryMissing|MissingDirectory|InstallDirectory" GenHub GenHub.Core -S || true
echo "## ManifestProvider implementations"
fd -a '.*Manifest.*\.cs$' . | while read -r f; do echo "--- $f"; wc -l "$f"; rg -n "GetManifestAsync|RequiredDirectories|DirectoryMissing" "$f" || true; doneRepository: community-outpost/GenHub
Length of output: 26060
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="GenHub/GenHub/Features/Validation/GameInstallationValidator.cs"
test_file="GenHub/GenHub.Tests/GenHub.Tests.Core/Features/Validation/GameInstallationValidatorTests.cs"
manifest_provider="GenHub/GenHub/Features/Manifest/ManifestProvider.cs"
content_manifest="GenHub/GenHub.Core/Models/Manifest/ContentManifest.cs"
content_validator="GenHub/GenHub/Features/Content/Services/ContentValidator.cs"
fs_validator="GenHub/GenHub/Features/Validation/FileSystemValidator.cs"
echo "## GameInstallationValidatorTests around missing directory test"
sed -n '200,270p' "$test_file" | cat -n
echo "## GameInstallationValidator fallback and directory validation"
sed -n '70,145p' "$file" | cat -n
echo "## ContentManifest RequiredDirectories declaration"
sed -n '90,115p' "$content_manifest" | cat -n
echo "## ContentValidator required directory handling"
sed -n '190,225p' "$content_validator" | cat -n
echo "## FileSystemValidator directory helper"
sed -n '25,55p' "$fs_validator" | cat -n
echo "## ManifestProvider manifest builds around required directories"
sed -n '120,170p' "$manifest_provider" | cat -n
sed -n '250,280p' "$manifest_provider" | cat -n
echo "## Static control-flow probe for required directory condition"
python3 - <<'PY'
from pathlib import Path
import re
content = Path("GenHub/GenHub/Features/Validation/GameInstallationValidator.cs").read_text()
fallback = bool(re.search(r'passes\.Count == 0\s*\{[^}]*passes\.Add\(\(null, installation\.InstallationPath\)\)', content, re.S))
directive = bool(re.search(r'if \(requiredDirs\.Any\(\) && gameType is not null\)', content))
method_calls = []
m = re.finditer(r'ValidateDirectoriesAsync\(([^)]*)\)', content)
for mm in m:
method_calls.append(mm.group(1))
print({
"fallback_adds_null_game_type": fallback,
"directories_condition": directive,
"require_game_type_non_null": "gameType is not null" in content,
"validate_directories_calls": list(method_calls),
"affected_passes_count_expr": "passes.Count * 4" in content
})
PYRepository: community-outpost/GenHub
Length of output: 17919
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## GameInstallation.cs relevant constructor/slots"
file="$(fd 'GameInstallation\.cs$' GenHub GenHub.Core 2>/dev/null | head -n1 || true)"
if [ -n "${file:-}" ]; then
wc -l "$file"
sed -n '1,180p' "$file" | cat -n
fi
echo "## ManifestProvider fallback null gameType branch"
sed -n '235,350p' GenHub/GenHub/Features/Manifest/ManifestProvider.cs | cat -n
echo "## Content validator required dir validation effect"
rg -n "missing directories|MissingDirectory|DirectoryMissing|expectedDirectories|ContentValidator" GenHub/GenHub/Features/Content/Services/ContentValidator.cs GenHub/GenHub.Core/Models/Validation/ValidationIssue.cs GenHub/GenHub/Features/Validation/FileSystemValidator.cs -SRepository: community-outpost/GenHub
Length of output: 9954
Avoid skipping required-directory validation in the fallback pass.
When passes falls back to (null, installation.InstallationPath), the generated manifest still adds "Data" and "Maps" to RequiredDirectories, but ValidateDirectoriesAsync is not called because the condition requires gameType is not null. As a result, an installation with missing Data or Maps in the fallback path can miss a DirectoryMissing issue even though the other validation steps run. Either validate RequiredDirectories for gameType is null, or remove the fallback manifest fallback so this invalidation never happens.
🤖 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 `@GenHub/GenHub/Features/Validation/GameInstallationValidator.cs` around lines
140 - 143, Update the required-directory validation in the manifest validation
flow so ValidateDirectoriesAsync runs whenever requiredDirs contains entries,
including the fallback case where gameType is null. Preserve the existing
requiredDirs guard and ensure fallback manifests still report missing Data or
Maps directories.
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
|
|
||
| var allFiles = Directory.GetFiles(directory, "*.exe", SearchOption.AllDirectories); | ||
| var allFiles = Directory.EnumerateFiles(directory, "*", SearchOption.AllDirectories) | ||
| .Where(path => ExecutableFileClassifier.IsLegacyLaunchCandidate(path, path)); |
There was a problem hiding this comment.
SUGGESTION: The XML doc remark above (lines 214–221) is now stale.
It states "a content-based (magic-byte) classification slots in at the classifier call without this site changing," implying this call still uses the name-only overload. Commit 3 of this PR changed it to the content-aware IsLegacyLaunchCandidate(path, path) overload — the site did change — and the analogous remark on GameClientDetector.GetLaunchCandidateFiles was updated to match, but this one was missed. A future reader could be misled into thinking content-based classification has not been applied here.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| && inst.HasZeroHour | ||
| && !string.IsNullOrEmpty(inst.GeneralsPath) | ||
| && !string.IsNullOrEmpty(inst.ZeroHourPath) | ||
| && Path.GetFullPath(inst.GeneralsPath).Equals(Path.GetFullPath(inst.ZeroHourPath), StringComparison.OrdinalIgnoreCase); |
There was a problem hiding this comment.
SUGGESTION: The "combined directory" predicate is duplicated across three sites.
The same boolean (both games flagged and identical resolved paths) appears inline here, again in GameInstallationValidator.ValidateAsync, and as WindowsInstallationDetector.IsCombinedDirectory. If the definition of "combined" changes, all three must be kept in sync or detection, dedup, and validation will disagree — the validator's IsSiblingGameRootArchive suppression relies on matching the same definition the detectors used. Consider promoting this to a shared member (e.g. a GameInstallation.IsCombinedDirectory computed property) for a single source of truth.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: 2 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (20 files)
|
1 participant
Summary
Detects retail installations by the presence of retail archives rather than by
executable name, and scans launch candidates through the executable classifier.
Closes #330.
Why
Executable-name detection misses a retail install whose engine binary is
extensionless — the shape a native Mach-O or ELF build has — and misses combined
directories where retail archives sit beside publisher binaries. Archive presence
is the property that actually identifies a retail install.
Rebase note
This branch was stacked on
feat/native-launch, which squash-merged as #332. Itsown commits were replayed onto
developmentwithgit rebase --onto origin/development d1adeeb.The replay applied cleanly but did not compile, which is worth recording: #339
split
ExecutableFileClassifier.IsLegacyLaunchCandidateinto a name-only overloadand a content-aware
(path, absolutePath)overload, and this branch's two callsites passed the old single-argument form as a method group.
Both sites enumerate real files from
Directory.EnumerateFiles, so per theclassifier's own guidance — "whenever the file is on disk, call
IsLegacyLaunchCandidate(path, absolutePath)so extensionless files are judged bytheir magic bytes" — they now use the content-aware overload.
That surfaced a second failure.
GameClientDetectorTestswrote"dummy content"asits extensionless native-binary fixture, which magic-byte classification correctly
rejects. The fixture now writes real ELF magic
(
0x7F 0x45 0x4C 0x46 0x02 0x01 0x01 0x00), matching the fixtures #339 introduced.The
GetLaunchCandidateFilesremark claimed content-based classification would slotin "without this site changing". It did change, so the remark was corrected.
These adaptations are isolated in their own commit rather than folded into the
feature commits, so the API drift is reviewable on its own.
Testing
dotnet test GenHub/GenHub.Tests/GenHub.Tests.Core/GenHub.Tests.Core.csproj -c ReleaseGreptile Summary
This PR replaces executable-name-based retail installation detection with archive classification, adds combined-installation manifest and validation handling, and expands executable scanning to native binaries.
Confidence Score: 4/5
The PR should not merge until false retail-install detection and the missing standard Generals client for combined installations are fixed.
A single non-retail
*ZH.bigfile in a broad macOS search root can create a bogus installation, and legitimate combined installations no longer produce the standard Generals client required by profile and manifest consumers.Files Needing Attention: GenHub/GenHub.Core/Helpers/RetailArchiveClassifier.cs, GenHub/GenHub.MacOS/GameInstallations/MacOSInstallationDetector.cs, GenHub/GenHub/Features/GameClients/GameClientDetector.cs
Important Files Changed
zh.bigsuffix accepts known non-retail community archives as installation evidence.Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD Root[Candidate directory] --> Archive[Classify retail archives] Archive --> Install[Set game flags and paths] Install --> Clients[Detect standard and publisher clients] Install --> Manifests[Resolve per-game manifests] Manifests --> Validation[Validate each detected game] Clients --> Profiles[Create game profiles]Prompt To Fix All With AI
Reviews (1): Last reviewed commit: "fix(detection): classify launch candidat..." | Re-trigger Greptile
Context used (3)