chore: release alpha 4 - #378
Conversation
…trols - Added `NullableDecimalToIntConverter` to handle data conversion between View and ViewModel. - Defined a new "phantom" style for `NumericUpDown` to match the existing minimalist UI. - Replaced `TextBlock` value displays with `NumericUpDown` controls for Gamma, Audio volumes, Font sizes, and Camera settings. - Adjusted Grid column widths to accommodate interactive input fields.
Added manual folder picker for Retail GameInstallations without registry
…227) Fixed GameSettings not persisting whenever the profile was saved.
… resolution (#225) * fix(deps): ensure disabled game installation is auto-resolved when required by client * feat(local): add GameType selection and validation for local content * feat(game-profiles): hide GameInstallation from UI
…ate detection (#228) Upon launching a GameProfile that has a GeneralsOnline GameClient, a check for an update will be made. When an update is available it will download the new release, remove the old GameClient and MapPack, reconcile the profiles with the new release, then continue with launching the game.
Users can now subscribe to development branches to receive notifications for every new push or update
…xternal JSON (#205) This commit lays the foundation for the Provider Defition framework whereby publishers can become ContentProviders in GenHub.
… data` button under Danger Zone (#230)
… management (#235) - Added a GameType filter for Generals and Zero Hour. - Updated the selected ContentType button to highlight when active. - Centered all ContentItems. - Organized ContentItems into a brick-style layout. - Replaced the add/remove buttons with a click action on the ContentItem itself. - Set the "Delete" button to be visible only on hover.
Added a section under settings for the following data directories; workspace, profiles, manifests and CAS-pool,
…234) Removed old local content dialog properties and methods from GameProfileSettingsViewModel. Enhanced UI with drag-and-drop support for file and folder imports.
…age and multi-instance debugging support (#239) * Added persistent storage for manual game installations and multi-instance debugging support. * Improved update reliability with HTTP retries, GitHub API caching, and enhanced error handling. * Refactored constants and implemented automated cleanup for orphaned workspace directories.
…port and GitHub rate limit tracking (#237) This update introduces a persistent notification feed with history tracking and a redesigned bell UI featuring dynamic unread badges. It adds support for multi-action notifications with styled buttons and integrates a GitHub rate limit tracker to provide real-time API usage warnings. The core notification architecture has been refactored into record types for better immutability, alongside service enhancements for managing read states and history. Additionally, the UI features a new custom title bar and redesigned toasts, backed by comprehensive unit tests to ensure thread safety and reliability.
…y launching (#241) * feat: Implement SteamLauncher for game directory preparation and proxy launching - Added SteamLauncher service to manage game directory preparation for Steam-tracked profiles. - Introduced ProxyConfig for configuring the proxy launcher with target executable and working directory. - Implemented methods for deploying the proxy launcher and creating necessary configuration files. - Enhanced manifest generation to support backup executable handling from the Steam Proxy Launcher. - Updated SteamManifestPatcher to improve executable validation logic. - Refactored HardLinkStrategy to prioritize file deduplication based on content type. - Improved error handling and logging throughout the game installation and manifest processes. - Updated documentation to reflect changes in detection and manifest creation workflows. * fix: resolve proxy launcher not replacing generals.exe * chore: resolve greptile comments * fix: resolve GameProfileSettingsWindow drag to resize from fullscreen
) * feat: add GitHub Pages deployment workflow and landing page assets * feat: Add Replay Manager tool with drag-and-drop functionality and file management - Implemented ReplayManagerView for managing replay files. - Added drag-and-drop support for importing replay files (.rep, .zip). - Enhanced ToolsViewModel to include Replay Manager services. - Updated UI styles for better visual consistency. - Introduced EnumToBoolConverter and EqualsToConverter for improved data binding. - Added documentation for Replay Manager features and usage. - Registered Replay Manager services in the dependency injection module. * feat(tools): enhance map/replay managers and refine tools sidebar ui * feat(tools): enhance map/replay managers and refine tools sidebar ui * feat(tools): enhance map/replay managers and refine tools sidebar ui * fix: resolve build version mismatch and the add local content DragDrop not working admin
…odding utilities (#247) Users can now add individual executables, or tools to be ran in a gameprofile. Tools or executables like WorldBuilder.exe, WNDeditor, ParticleEditor, Discord.exe, Notepad.exe, or any other program or tool they desire.
…for creating profiles (#253)
… process monitoring
…to High (0) in Options.ini (#256)
… manifest reconstruction, and eliminate copy fallback (#419)
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| } | ||
|
|
||
| if (!string.Equals(branch, AppUpdateConstants.DevelopmentBranch, StringComparison.OrdinalIgnoreCase) && | ||
| !string.Equals(branch, AppUpdateConstants.MainBranch, StringComparison.OrdinalIgnoreCase)) |
There was a problem hiding this comment.
WARNING: main-subscribed users with a PAT never get release-update notifications
For a PAT holder subscribed to main, CheckSubscribedBranchUpdateAsync checks artifacts, and when none exist the gate at 438-442 excludes main from the stale-branch fallback and the method simply ends — CheckStandardReleaseUpdateAsync is never called. The no-token path (lines 385-390) and UpdateNotificationViewModel (lines 818-832) both fall through to the release check for main, so only this background path silently drops it.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| } | ||
|
|
||
| logger?.LogDebug("Periodic update check timer triggered"); | ||
| _ = CheckForUpdatesInBackgroundAsync(_cts.Token); |
There was a problem hiding this comment.
WARNING: Race between OnPeriodicUpdateTimerCallback and Dispose can crash the process
The callback checks _disposed/_cts.IsCancellationRequested at line 868, but Dispose can then run _cts.Cancel() and _cts.Dispose() (lines 150-159) before line 874 executes; reading _cts.Token on a disposed CTS throws ObjectDisposedException on the timer thread-pool thread, which is unhandled and terminates the process (Timer.Dispose does not wait for in-flight callbacks). RestartPeriodicUpdateTimer (line 843) has the same window. Capture the token before the guard or wrap the invocation in try/catch.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| finally | ||
| { | ||
| var currentSettings = userSettingsService.Get(); | ||
| velopackUpdateManager.SubscribedPrNumber = currentSettings.SubscribedPrNumber; |
There was a problem hiding this comment.
WARNING: Coordinator and ViewModel race on shared mutable IVelopackUpdateManager state
The finally block (106-107) and the per-channel helpers (182-183, 397-398, 469) write SubscribedPrNumber/SubscribedBranch on the shared singleton manager, while UpdateNotificationViewModel writes the same properties from the UI thread (e.g. SubscribeToPr, lines 1510-1511). If the user interacts with the update window while a background check runs, the two flows interleave and each clobbers the other's subscription, yielding wrong-channel artifact results. _checkLock only serializes coordinator-vs-coordinator, not coordinator-vs-VM.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| { | ||
| throw; | ||
| } | ||
| catch (Exception ex) |
There was a problem hiding this comment.
WARNING: Catch-all swallows failures and CheckForUpdatesAsync returns bare Task
The inner catch (Exception) (99-102) hides every check failure from callers, and the public IBackgroundUpdateCoordinator.CheckForUpdatesAsync returns Task rather than an OperationResult per docs/dev/result-pattern.md. It also duplicates the legitimate worker-boundary catch-all in CheckForUpdatesInBackgroundAsync (835-838), making the outer one effectively dead for direct callers; CS-R1008 allows the generic catch only at the outermost boundary.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| IGitHubTokenStorage? gitHubTokenStorage = null) : IBackgroundUpdateCoordinator, IRecipient<UpdateSettingsChangedMessage> | ||
| { | ||
| private readonly CancellationTokenSource _cts = new(); | ||
| private readonly SemaphoreSlim _checkLock = new(1, 1); |
There was a problem hiding this comment.
SUGGESTION: _checkLock is never disposed
Dispose(bool) (144-160) disposes the timer and _cts but not the SemaphoreSlim, so the catch (ObjectDisposedException) around _checkLock.Release() (113-116) is unreachable dead code. Either dispose the semaphore in Dispose or remove the handler.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| { | ||
| try | ||
| { | ||
| await Task.Delay(3000, cancellationToken); |
There was a problem hiding this comment.
SUGGESTION: Magic delay and fully-qualified types
Task.Delay(3000, ...) duplicates the 3-second interval already modeled by constants elsewhere (e.g. TimeIntervals), and System.Threading.CancellationToken (here and around line 429) should come from a using directive like the rest of the file.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| <CheckBox Grid.Column="1" Grid.Row="0" Content="Remember User" IsChecked="{Binding GoRememberUsername}" Margin="0,0,0,4" /> | ||
| <Grid ColumnDefinitions="140,*,Auto"> | ||
| <TextBlock Text="Transition Speed" VerticalAlignment="Center" /> | ||
| <Slider Grid.Column="1" Value="{Binding TshGameWindowTransitionSpeedMultiplier}" Minimum="1.0" Maximum="4.0" TickFrequency="0.05" IsSnapToTickEnabled="True" SmallChange="0.05" LargeChange="0.5" VerticalAlignment="Center" Margin="0,0,12,0" /> |
There was a problem hiding this comment.
SUGGESTION: Hardcoded slider bounds duplicate GameSettingsTheSuperHackersConstants
Minimum="1.0" Maximum="4.0" (324-325) restate the min/max transition-speed-multiplier constants; if the constants change, the XAML silently drifts. Expose the bounds from the ViewModel or bind to x:Static constants.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| { | ||
| if (tsh.TryGetValue("UseDoubleClickAttackMove", out var doubleClickTsh)) | ||
| profile.VideoUseDoubleClickAttackMove = ParseBool(doubleClickTsh); | ||
| if (tsh.TryGetValue("ScrollFactor", out var scrollTsh) && int.TryParse(scrollTsh, out var scrollTshVal)) |
There was a problem hiding this comment.
SUGGESTION: Culture-sensitive int.TryParse for machine-written INI values
Lines 647 and 653 parse TSH values with a culture-sensitive int.TryParse while the sibling new ParseTransitionSpeedMultiplier correctly uses CultureInfo.InvariantCulture. Parsing program-generated settings should be invariant for consistency and to avoid locale surprises if the parsed type ever widens.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| For game selection tabs, replay category toggles, or filter pills: | ||
|
|
||
| - **Style:** Inset rounded pill (`CornerRadius="8"`, `Padding="16,8"`). | ||
| - **Pointer-over:** Soft hover highlight `{DynamicResource SurfaceHoverBrush}` or `#10FFFFFF`. |
There was a problem hiding this comment.
SUGGESTION: Guidance offers a hardcoded hex as an acceptable alternative
{DynamicResource SurfaceHoverBrush} or #10FFFFFF (and foreground White on line 208) contradicts the file's own anti-pattern section, which forbids hardcoded hexes and mandates dynamic theme resources. Reference a semantic token (or define one) instead of blessing the literal.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| using GenHub.Common.Services; | ||
| using GenHub.Core.Constants; | ||
| using GenHub.Core.Interfaces.Common; | ||
| using GenHub.Core.Models.Common; |
There was a problem hiding this comment.
SUGGESTION: Unused using
using GenHub.Core.Models.Common; is not referenced — only ColorTheme from GenHub.Core.Models.Theming is used.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| { | ||
| profileKeys = []; | ||
| index.ProfileInstallations[manifest.ProfileId] = profileKeys; | ||
| // Manifest file exists on disk but could not be read; retain conflict conservatively |
There was a problem hiding this comment.
WARNING: Corrupt or unreadable manifest is treated as an active conflict, blocking installations indefinitely.
When LoadUserDataManifestByKeyAsync returns null but the manifest file still exists on disk (partial write, disk error, permission denied, JSON corruption), the else branch returns the stale installationKey as if there were a real conflict. Because the same code path then refuses to clean up the mapping, every subsequent CheckFileConflictAsync for that path and every subsequent InstallUserDataAsync that touches that file will keep failing until someone manually removes the manifest file or the index entry. For a corrupt manifest there is no live installation to conflict with; the conservative behavior is to treat manifest == null the same as !manifest.IsActive and prune the mapping (or at minimum log a LogError so the user can intervene). At minimum, distinguish "file exists but unreadable" from "file is intact and active" before deciding to block future installs.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| return OperationResult<UserDataSwitchInfo>.CreateSuccess(switchInfo); | ||
| } | ||
| catch (Exception ex) | ||
| private static ManifestFile CreateUserMapsFile(ManifestFile file) |
There was a problem hiding this comment.
WARNING: The adopt loop on line 198-204 bypasses this new CreateUserMapsFile helper and silently fails for pre-existing map manifests.
CreateUserMapsFile correctly remaps ContentInstallTarget.Workspace to ContentInstallTarget.UserMapsDirectory for ContentType.Map/MapPack and copies all metadata fields. The adopt loop in SwitchProfileUserDataAsync (line 198-204) does neither: it reuses f.InstallTarget verbatim and only copies RelativePath/Hash/Size/InstallTarget. Any UserDataManifest persisted before this PR (or built by code paths that did not apply the same map-detection) still has Workspace as the file target, and the tracker then filters those files out at UserDataTrackerService.cs:67-70, causing the adopt InstallUserDataAsync to fail with "No user data files to install". Route the adopted file through CreateUserMapsFile (or a shared GetUserDataFiles-style remap) so adopt behaves consistently with PrepareProfileUserDataAsync.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| Assert.True(conflictResult.Success); | ||
| Assert.Equal("mappack-id_profile-b", conflictResult.Data); | ||
|
|
||
| var indexPath = Path.Combine(_appDataDir, DirectoryNames.UserData, FileTypes.UserDataIndexFileName); |
There was a problem hiding this comment.
SUGGESTION: The new tests cover the deactivation/transfer flow in UserDataTrackerService but leave the new Workspace -> UserMapsDirectory remap in ProfileContentLinkerService.GetUserDataFiles (line 380-391) and CreateUserMapsFile (line 393-410) entirely untested. There is no ProfileContentLinkerServiceTests at all. Add at least one test that passes a ContentManifest with ContentType.Map (or MapPack) and InstallTarget.Workspace files into PrepareProfileUserDataAsync and asserts that the resulting UserDataManifest entries carry InstallTarget = UserMapsDirectory; otherwise a future change to the remap condition would silently re-break the map install path the PR is trying to fix.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
…letion receipts (#412)
|
ⓘ Your Qodo trial ends soon. Ask your workspace admin to set up billing to keep reviews running after the trial. Manage billing |
Code Review SummaryStatus: 7 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
WARNING
SUGGESTION
Files Reviewed (10 of 123 changed files)
Unreviewed changed areas (out of scope of this incremental diff focus): other ActionSet fixes, GenPatcherViewModel, GenPatcherTool, BasePackageDeploymentFix, DownloadSecurityValidator (other lines), ExternalUrls, ProcessConstants, CsvGenerator, DependencyResolver, build-release.ps1, and ~110 other files in the diff. Fix these issues in Kilo Cloud Previous Review Summaries (2 snapshots, latest commit bc929be)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit bc929be)Status: 10 Issues Found | Recommendation: Address before merge Overview
Incremental review of Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (12 files)
Fix these issues in Kilo Cloud Previous review (commit 1f48230)Status: 0 New Issues | Recommendation: Existing review coverage is comprehensive Overview
NotesThis PR (
Per the duplicate-prevention rule, this review pass did not re-report any of those defects. I attempted to verify the latest commit ( A full file-by-file review of 1,044 changed files exceeds the scope of a single read-only pass; the 64+ existing inline comments already identify every category of issue the project guidelines call out (security, race conditions, null-deref, resource leaks, theme-token violations, constants violations, result-pattern violations). No new inline comments were posted. Existing Review Coverage (representative)
Files Reviewed
Reviewed by minimax-m3:free · Input: 109.3K · Output: 25.1K · Cached: 5.4M |
…GameInstallationValidator (#426)
| } | ||
|
|
||
| int targetIndex = 0; | ||
| foreach (var (targetPath, targetGame) in targets) |
There was a problem hiding this comment.
WARNING: Core multi-target loop has no test coverage
The loop at lines 149-166 (and the per-target CSV queries it orchestrates) is the central behavior of this change, but no test exercises it. In GameInstallationValidatorTests, the progress test creates the Generals/Zero Hour directories without any game executables, so Fetch()/SetPaths leave HasGenerals/HasZeroHour false and every validation in the suite falls into the targets.Count == 0 single-target fallback (line 142). Consequently nothing asserts that SearchAsync runs once with TargetGame == Generals and once with TargetGame == ZeroHour, no test checks aggregated issue counts across targets, and the progress-reset defect flagged separately is invisible to the suite. Add a fixture that places a valid executable (game.dat / generals.exe, see GameInstallation.HasValidExecutable) so the two-target path actually runs.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| gameType, | ||
| normalizedLanguage); | ||
|
|
||
| progress?.Report(new ValidationProgress(1, 4, "Resolving manifest")); |
There was a problem hiding this comment.
WARNING: Progress reporting resets and becomes non-monotonic for multi-target installations
ValidateInstallationCoreAsync hardcodes a 1/4..4/4 scale but is invoked once per target from the loop above. For an installation with both Generals and Zero Hour, progress reaches 100 percent (4/4 Validation complete) and then restarts at 25 percent (1/4) for the second target — a regression from the previous implementation, which computed totalSteps dynamically (+1 per detected game) so the sequence stayed monotonic. Additionally, contentValidator.ValidateAllAsync (line 262) reports its own 0/3..3/3 scale through the same progress instance, interleaving two incompatible scales even for single-target runs. Consider passing a per-target offset into the core method, or wrapping the IProgress<ValidationProgress> handed to inner validators so child reports are rescaled.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
|
|
||
| // Use ContentValidator for full content validation (integrity + extraneous files) | ||
| progress?.Report(new ValidationProgress(2, 4, "Core manifest validation")); | ||
| var manifestValidationResult = await contentValidator.ValidateManifestAsync(manifest, cancellationToken); |
There was a problem hiding this comment.
SUGGESTION: Structural manifest issues are double-counted per target
ValidateManifestAsync runs ValidateManifestStructure (ContentValidator.cs:42), and ValidateAllAsync at line 262 runs the same structure validation again as its Step 1 (ContentValidator.cs:70), so each structural issue is appended to issues twice per target — up to four times across a Generals+Zero Hour validation. With the new ValidationResult.MissingFilesCount/CorruptedFilesCount/ExtraFilesCount properties added in this PR, the duplicates now surface to consumers as inflated counts. Consider dropping the standalone ValidateManifestAsync call here (its work is subsumed by ValidateAllAsync) or adding an option to skip its Step 1.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
|
|
||
| if (installation.HasZeroHour) | ||
| var searchResult = await _resolvedCsvProvider.SearchAsync(query, cancellationToken); |
There was a problem hiding this comment.
WARNING: Local installation validation now silently performs network I/O on every run
SearchAsync drives CsvDiscoverer to fetch the remote index.json (CsvConstants.DefaultIndexFileUrl) and CsvResolver to download the per-language CSV from entry.Url. Because IGameInstallationValidator, CsvContentProvider, and CsvDiscoverer are all registered transient (ValidationModule.cs:29, ContentPipelineModule.cs:354/358), CsvDiscoverer._cachedEntries never survives a validation run — every validation downloads fresh content over the network. Offline or firewalled users get MissingFile error issues (No CSV manifest found...) for perfectly valid local installations unless the IManifestProvider fallback happens to return a manifest. Consider caching the resolved manifest on disk (e.g. via the CAS pool or a configured local catalog) and treating network unavailability as use-cached-manifest rather than a validation error, consistent with docs/dev/result-pattern.md guidance on network failure states.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| { | ||
| throw; | ||
| } | ||
| catch (Exception ex) |
There was a problem hiding this comment.
SUGGESTION: Generic catch (Exception) in new code violates CS-R1008
This new catch-all converts any exception — including programming bugs such as a NullReferenceException while handling catalog data — into a MissingFile validation issue, masking defects; the coding standard forbids catching generic Exception outside top-level boundaries. Catch the specific expected failures (HttpRequestException, IOException, CsvHelper read failures — OperationCanceledException is already handled above) and let unexpected exceptions propagate. (The same pattern at line 276 is moved pre-existing code; only this method is newly introduced.)
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| /// <param name="progress">Progress reporter for MVVM integration.</param> | ||
| /// <param name="cancellationToken">A cancellation token.</param> | ||
| /// <returns>A <see cref="ValidationResult"/> representing the outcome of the validation.</returns> | ||
| Task<ValidationResult> ValidateAsync(GameInstallation installation, string language, IProgress<ValidationProgress>? progress = null, CancellationToken cancellationToken = default); |
There was a problem hiding this comment.
SUGGESTION: Overload pair makes the bare call ValidateAsync(installation, null) ambiguous (CS0121)
With (GameInstallation, IProgress<ValidationProgress>?, CancellationToken = default) at line 31 and this overload taking string second, a caller writing the two-argument form validator.ValidateAsync(installation, null) no longer compiles: the null literal converts equally well to IProgress<ValidationProgress> and string, and neither candidate wins the betterness tie-break (the no-default-substitution tie-break only rescues the three-argument form used in the tests). No current caller hits the trap, but every future consumer of IGameInstallationValidator passing a literal null progress will hit a confusing compile error. Consider removing the progress-only overload (callers can use named progress: arguments) or renaming the language-aware overload.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| if (files.Length > 0) | ||
| try | ||
| { | ||
| var files = Directory.GetFiles(installationPath, pattern, SearchOption.AllDirectories); |
There was a problem hiding this comment.
SUGGESTION: Wildcard branch and its new error fall-throughs are untested
The *ZH.big wildcard search — now correctly ordered last after the language-specific ZH patterns, fixing the old EN-before-DE precedence bug — is the detector's most expensive path (recursive SearchOption.AllDirectories scan), and this commit adds IOException/UnauthorizedAccessException fall-throughs (lines 140-147) that silently degrade to the EN fallback. LanguageDetectorTests has no test for a wildcard-only match, its precedence, or either error path; the English BIG patterns (English.big, AudioEnglish.big, SpeechEnglish.big) and INIZH.big are also uncovered. A regression here would silently mis-detect the language for any non-English Zero Hour install that only matches the generic wildcard.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| ### 1. `LanguageDetector` (`ILanguageDetector`) | ||
| Located in `GenHub.Core.Features.GameInstallations`: | ||
| - Analyzes game directory layout and file patterns to determine the installed language. | ||
| - Checks language directories: `Data\english\`, `Data\german\`, `Data\deutsch\`, `Data\french\`, `Data\spanish\`, `Data\italian\`, `Data\korean\`, `Data\polish\`, `Data\PortugueseBrazil\`, `Data\chinese\`, `Data\chinesetraditional\`. |
There was a problem hiding this comment.
WARNING: Documented language directories contradict the detection constants
The detector probes Data/portuguese (LanguageDirectoryNames.DataPortuguese), not Data\PortugueseBrazil\ as written here; Data/chinese-traditional with a hyphen (LanguageDirectoryNames.DataChineseTraditional), not Data\chinesetraditional\; and lowercase Data/german, Data/deutsch, Data/french, etc. The capitalized forms shown here and in the matrix rows below (Data\German\, Data\French\, Data\Chinese\, ...) exist as constants only for English (Data/English). Because Directory.Exists is case-sensitive on Linux and macOS (both first-class GenHub platforms), this matrix will mislead anyone debugging detection failures. Align the doc with the LanguageDirectoryNames values.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| ### 2. `CsvDiscoverer` (`IContentDiscoverer`) | ||
| Located in `GenHub.Features.Content.Services.ContentDiscoverers`: | ||
| - Discovers remote CSV catalogs matching the requested game type (`Generals` or `ZeroHour`) and language. | ||
| - First queries remote `index.json` metadata if available, falling back to configuration catalog URLs (`CsvConstants.DefaultGeneralsCsvUrl` / `DefaultZeroHourCsvUrl`). |
There was a problem hiding this comment.
SUGGESTION: References nonexistent constants and a non-generated ID format
CsvConstants.DefaultGeneralsCsvUrl and DefaultZeroHourCsvUrl do not exist anywhere in the codebase — the actual fallback chain is config IndexFilePath, then CsvConstants.DefaultIndexFileUrl (index.json), then config CsvValidationCatalogs (CsvDiscoverer.cs:310-329). The example manifest ID on line 43 (csv-generals-1.08-de) is also not what ManifestIdGenerator produces (5-segment format such as 1.0.csvregistry.gameinstallation.generals-1.08-de), and line 54 shows the provider source name as csv-registry while PublisherTypeConstants.CsvRegistry is csvregistry without a hyphen.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| `ValidationResult` includes comprehensive metrics for diagnosing game installation health: | ||
|
|
||
| ```csharp | ||
| public sealed record ValidationResult( |
There was a problem hiding this comment.
SUGGESTION: Code snippet misrepresents the ValidationResult API shape
The snippet shows a sealed record with a Path positional parameter; the actual type is a class whose first parameter/property is ValidatedTargetId, with Elapsed inherited from ResultBase and IsValid delegating to Success (ValidationResult.cs:8-21). Code copied from this documentation will not compile against the real API.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
…nd documentation suite (#427)
* fix(content): deliver CSV catalog files * fix(content): preserve HTTP delivery manifest * test(content): normalize HTTP delivery path
|
|
|
||
| if (Directory.Exists(localPath)) | ||
| { | ||
| Directory.CreateDirectory(cloudPath); |
|
|
||
| return Task.FromResult(new ActionSetResult(true, null, details)); | ||
| } | ||
| catch (Exception ex) |
There was a problem hiding this comment.
SUGGESTION: Generic catch (Exception ex) masks programming defects (CS-R1008)
This catch-all (and the matching one in ApplyInternalAsync) swallows everything including NullReferenceException, ArgumentException, etc., returning a failure result that hides real bugs. Catch the specific expected failures (IOException, UnauthorizedAccessException) and let unexpected exceptions propagate.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| { | ||
| var actualHash = await ComputeSha256Async(filePath, ct); | ||
| hashMatched = allowedSha256Hashes.Any(h => string.Equals(h, actualHash, StringComparison.OrdinalIgnoreCase)); | ||
| if (!hashMatched && !hasPublisherCheck) |
There was a problem hiding this comment.
CRITICAL: Hash mismatch is silently downgraded to a warning when a publisher check is also configured
At line 188 the if (!hashMatched && !hasPublisherCheck) guard means a SHA-256 mismatch is ignored when expectedAuthenticodePublisher is non-null; control then falls through to ValidateAuthenticodeSignature. A pinned-hash validator must fail-closed when the hash does not match — otherwise an attacker who can re-sign with a malicious Authenticode publisher bypasses the pin. Restructure so a hash mismatch is a hard failure regardless of publisher configuration.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| { | ||
| logger.LogError("An error occurred while removing tool with ID: {ToolId}", toolId); | ||
| logger.LogError(ex, "An error occurred while removing tool with ID: {ToolId}", toolId); | ||
| return OperationResult<bool>.CreateFailure("An error occurred while removing the tool."); | ||
| } | ||
| } |
| { | ||
| logger.LogError("An error occurred while removing tool with ID: {ToolId}", toolId); | ||
| logger.LogError(ex, "An error occurred while removing tool with ID: {ToolId}", toolId); | ||
| return OperationResult<bool>.CreateFailure("An error occurred while removing the tool."); | ||
| } | ||
| } |
There was a problem hiding this comment.
CRITICAL: Missing trailing newline at end of file (line 218 has no \n)
StyleCop's SA1518/SA1519 enforce a final newline; many CI lint configs fail the build on this. The diff shows }\ with no newline marker on the next line. Add \n after the closing brace.
| } | |
| } |
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| IInstallationInstructionsService installationInstructionsService) | ||
| : BaseContentProvider(contentValidator, installationInstructionsService, logger) | ||
| { | ||
| private readonly IContentDiscoverer _discoverer = discoverers.OfType<CsvDiscoverer>().FirstOrDefault() |
There was a problem hiding this comment.
WARNING: Field initialization silently falls back to any discoverer when CsvDiscoverer is not registered
The _discoverer = discoverers.OfType<CsvDiscoverer>().FirstOrDefault() ?? discoverers.FirstOrDefault(d => SourceName matches CsvConstants.SourceName) ?? throw ... fallback silently picks a non-CSV discoverer (e.g. a GeneralsOnline discoverer) if CsvDiscoverer is missing; the throw only fires when no discoverer exists at all. This makes the CSV provider wire itself to an unrelated source and ship the wrong manifests. Drop the fallback or restrict it to types that implement CSV resolution.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| } | ||
| catch (OperationCanceledException) | ||
| { | ||
| throw; | ||
| } | ||
| catch (Exception ex) |
There was a problem hiding this comment.
SUGGESTION: Generic catch (Exception ex) in new code violates CS-R1008
The new catch-all at line 108 (with the dedicated catch (OperationCanceledException) { throw; } already at line 104) hides programming defects (NullReferenceException, ArgumentException) behind a delivery failure. Catch the specific IO/HTTP failures (HttpRequestException, IOException, TaskCanceledException) and let unexpected exceptions propagate so real bugs surface.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| /// <summary> | ||
| /// Represents the result of an action set operation. | ||
| /// </summary> | ||
| public record ActionSetResult |
There was a problem hiding this comment.
WARNING: ActionSetResult does not extend ResultBase, breaking the result-pattern
Every other fallible operation in GenHub returns OperationResult<T> (or a specialized subtype like LaunchResult, ValidationResult). This new result type is a plain record with Success / ErrorMessage / Details and no relationship to ResultBase. Downstream consumers now need a parallel pattern (if (result.Success && result.ErrorMessage == null)), and this will grow into a permanent exception. Either have ActionSetResult extend ResultBase or expose an OperationResult<ActionSetResult> adapter at the orchestrator boundary.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
5 participants
Summary
Release GenHub Alpha 4 (
v0.0.4).What’s included
Testing