feat: implement ActionSet orchestration framework and GenPatcher tool integration by undead2146 · Pull Request #242 · community-outpost/GenHub

undead2146 · 2026-01-05T09:13:04Z

This PR adds a new Tool under the tools tab that applies all the core fixes which GenPatcher applies aswell.

Greptile Summary

This PR introduces a comprehensive ActionSet orchestration framework that ports GenPatcher functionality into GenHub as a new tool. The implementation adds 36+ automated fixes for Command & Conquer Generals/Zero Hour, including registry fixes, file system operations, DirectX/VC++ redistributable installers, and game configuration optimizations.

Key Changes:

Added IActionSet interface and BaseActionSet abstract class for extensible fix implementation
Implemented ActionSetOrchestrator to manage sequential fix application with crucial fix handling
Created GenPatcherTool plugin with UI (GenPatcherViewModel) for fix management
Added 36+ individual fixes covering registry patches, system compatibility, network optimization, and game configuration
Integrated IHttpClientFactory for downloading patches and redistributables (now properly registered in DI)
Centralized constants in ActionSetConstants, RegistryConstants, ExternalUrls, and GameSettingsConstants

Architecture Strengths:

Clean separation between core logic (GenHub.Core) and Windows-specific implementations
Proper dependency injection with all services registered
Parallel status checks prevent UI blocking
Admin privilege validation before applying registry-based fixes
Comprehensive logging with structured log codes for troubleshooting

Critical Issues Previously Addressed:

IHttpClientFactory registration added to WindowsServicesModule.cs:36
NahimicFix process checking logic corrected
Most path construction and error handling issues have been addressed in previous review rounds

Remaining Concerns:

Several fixes have logic issues documented in previous threads that should be verified as resolved
Documentation in docs/features/actionsets.md contains duplicate sections
Some fixes like RemoveReadOnlyFix have marker file logic inconsistencies
Path escaping vulnerabilities exist in PowerShell command execution

Build Status:

✓ No compiler warnings or errors detected
✓ All dependencies properly registered
✓ Follows conventional commit format in PR title

Confidence Score: 4/5

This PR is generally safe to merge with careful monitoring of reported issues
Score reflects clean build with zero warnings, proper architecture, and comprehensive functionality. However, multiple logic concerns from previous review threads remain documented (serial key handling, path construction issues, PowerShell escaping vulnerabilities, marker file inconsistencies). The core framework is solid but individual fixes need verification that previous feedback was addressed.
Pay close attention to EAAppRegistryFix.cs (serial key logic), OptionsINIFix.cs (resolution reporting), RemoveReadOnlyFix.cs (marker file logic), DirectXRuntimeFix.cs (argument assignment), and OneDriveFix.cs (folder merge handling)

Important Files Changed

Filename	Overview
GenHub/GenHub.Core/Features/ActionSets/ActionSetOrchestrator.cs	Core orchestrator for applying action sets sequentially with crucial fix handling and error recovery
GenHub/GenHub.Windows/Features/ActionSets/UI/GenPatcherViewModel.cs	ViewModel for GenPatcher UI with parallel status checks, admin validation, and batch fix application
GenHub/GenHub.Windows/Features/ActionSets/Fixes/EAAppRegistryFix.cs	Registry fix for EA app compatibility, serial key handling needs verification
GenHub/GenHub.Windows/Features/ActionSets/Fixes/OptionsINIFix.cs	Game settings optimizer with resolution and audio fixes, has reporting logic concerns
GenHub/GenHub.Windows/Features/ActionSets/Fixes/DirectXRuntimeFix.cs	DirectX runtime installer with download logic, has argument assignment issue
GenHub/GenHub.Windows/Features/ActionSets/Fixes/OneDriveFix.cs	OneDrive folder redirection with symlinks, folder merge scenario needs handling
GenHub/GenHub.Windows/Features/ActionSets/Fixes/RemoveReadOnlyFix.cs	PowerShell-based read-only removal, path escaping vulnerability and marker file logic issues
docs/features/actionsets.md	Comprehensive documentation with duplicate sections and categorization inconsistencies

Sequence Diagram

sequenceDiagram
    participant User
    participant GenPatcherViewModel
    participant ActionSetOrchestrator
    participant IActionSet
    participant RegistryService
    participant FileSystem
    participant HttpClient

    User->>GenPatcherViewModel: Open GenPatcher Tool
    GenPatcherViewModel->>GenPatcherViewModel: Check Admin Privileges
    GenPatcherViewModel->>ActionSetOrchestrator: GetAllActionSets()
    ActionSetOrchestrator-->>GenPatcherViewModel: List of ActionSets
    
    loop For each ActionSet
        GenPatcherViewModel->>IActionSet: IsApplicableAsync(installation)
        IActionSet-->>GenPatcherViewModel: bool
        GenPatcherViewModel->>IActionSet: IsAppliedAsync(installation)
        IActionSet-->>GenPatcherViewModel: bool
    end
    
    GenPatcherViewModel-->>User: Display ActionSets with Status
    
    User->>GenPatcherViewModel: Apply All Fixes
    GenPatcherViewModel->>RegistryService: IsRunningAsAdministrator()
    alt Not Admin
        GenPatcherViewModel-->>User: Show Error (Admin Required)
    else Is Admin
        loop For each unapplied fix
            GenPatcherViewModel->>IActionSet: ApplyAsync(installation)
            
            alt Registry Fix
                IActionSet->>RegistryService: SetStringValue/SetIntValue
                RegistryService-->>IActionSet: Success/Failure
            else File System Fix
                IActionSet->>FileSystem: Copy/Move/Create Files
                FileSystem-->>IActionSet: Success/Failure
            else Download Fix
                IActionSet->>HttpClient: Download Patch
                HttpClient-->>IActionSet: Patch File
                IActionSet->>FileSystem: Extract & Install
                FileSystem-->>IActionSet: Success/Failure
            end
            
            IActionSet-->>GenPatcherViewModel: ActionSetResult
            
            alt Crucial Fix Failed
                GenPatcherViewModel-->>User: Abort & Show Error
            else Non-Crucial Fix Failed
                GenPatcherViewModel-->>User: Continue & Log Warning
            else Fix Succeeded
                GenPatcherViewModel->>IActionSet: IsAppliedAsync(installation)
                IActionSet-->>GenPatcherViewModel: bool
                GenPatcherViewModel-->>User: Show Success
            end
        end
        
        GenPatcherViewModel-->>User: Show Final Summary
    end

Context used (3)

Rule from dashboard - What: All compiler warnings and linter warnings across the entire codebase must be resolved before m... (source)
Context from dashboard - Custom context (source)
Context from dashboard - Use dedicated constants classes instead of hardcoding constants string, integers or variables in ser... (source)

…oymentFix

… up marker line reading

…nd DeepSource exceptions - Fix WinVerifyTrust expired certificate validation in DownloadSecurityValidator - Ensure trailing directory separator in patch traversal containment checks - Implement persistent per-installation backups and undo restoration for BasePackageDeploymentFix - Scope deployment markers per installation to prevent cross-installation overwriting - Resolve DeepSource CS-R1008 generic exception catch blocks across all ActionSets - Restore Steam-aware IsApplicableAsync check in ProxyLauncher - Restore detailed netsh logging and error reporting in FirewallExceptionFix - Fix asynchronous execution gating and admin check offloading in GenPatcherViewModel - Standardize file names and clean up unused usings

…nd build errors - Fix non-static Logger access in BaseVCRedistFix.IsProductInstalled - Add missing System.IO namespace in VCRedist2005Fix - Reduce cognitive complexity in DownloadSecurityValidator and BasePackageDeploymentFix - Clean up unused variables and redundant exception rethrows - Fix StyleCop SA1202 and SA1204 member ordering - Use await using for SharpCompress archive entry streams

…ests - Retain backups when rollback restoration encounters errors - Do not delete backup files until marker persistence succeeds during undo - Retain destination files and fail safely when a recorded backup is missing - Add comprehensive unit tests covering transactional undo and missing backups

…duce cognitive complexity - Handle Win32Exception 1223 when user declines UAC prompt in BaseVCRedistFix - Extract TryRestoreBackup helper to reduce cognitive complexity in BasePackageDeploymentFix - Simplify LINQ loops and remove redundant null-forgiving operators

- Simplify tag collection with Where LINQ in GenPatcherDatCatalogParser - Pass caught exception as first parameter in GameProcessManager logger

bobtista · 2026-08-24T14:39:07Z

BasePackageDeploymentFix.ExtractArchiveEntriesAsync passes MaximumAddonPackageSizeBytes as both the per-entry limit and the remaining aggregate budget for every entry. Because that budget resets on each loop iteration, the intended 200 MB archive-wide limit is actually 200 MB per entry; an archive containing N entries can expand to N × 200 MB. Please track cumulative extracted bytes and pass MaximumAddonPackageSizeBytes - expandedBytes, as the existing Map/Replay extraction callers do, with a multi-entry aggregate-limit test.

…udget in BasePackageDeploymentFix

deepsource-io · 2026-08-26T01:37:18Z

+            await using (var stream2 = entry2.Open())
+            {
+                await stream2.WriteAsync(new byte[2048]);
+            }


`Using` block can be simplified

The using statement defines a scope at the end of which an object will be disposed. The downside is that this increases the indentation level of your code. However, with C# 8.0, you can use the new using declaration that no longer requires you to explicitly mention the braces. Although this reduces your code's indentation and nesting, the downside of this approach, however, is that the resource's lifetime may increase.

deepsource-io · 2026-08-26T01:37:18Z

+            await using (var stream2 = entry2.Open())
+            {
+                for (var i = 0; i < 110; i++)
+                {
+                    await stream2.WriteAsync(chunk);
+                }
+            }


`Using` block can be simplified

The using statement defines a scope at the end of which an object will be disposed. The downside is that this increases the indentation level of your code. However, with C# 8.0, you can use the new using declaration that no longer requires you to explicitly mention the braces. Although this reduces your code's indentation and nesting, the downside of this approach, however, is that the resource's lifetime may increase.

…implify using declarations

deepsource-io · 2026-08-26T01:46:41Z

+        await using (var stream2 = entry2.Open())
+        {
+            await stream2.WriteAsync(new byte[2048]);
+        }


`Using` block can be simplified

The using statement defines a scope at the end of which an object will be disposed. The downside is that this increases the indentation level of your code. However, with C# 8.0, you can use the new using declaration that no longer requires you to explicitly mention the braces. Although this reduces your code's indentation and nesting, the downside of this approach, however, is that the resource's lifetime may increase.

deepsource-io · 2026-08-26T01:46:41Z

+        await using (var stream2 = entry2.Open())
+        {
+            for (var i = 0; i < 110; i++)
+            {
+                await stream2.WriteAsync(chunk);
+            }
+        }


`Using` block can be simplified

The using statement defines a scope at the end of which an object will be disposed. The downside is that this increases the indentation level of your code. However, with C# 8.0, you can use the new using declaration that no longer requires you to explicitly mention the braces. Although this reduces your code's indentation and nesting, the downside of this approach, however, is that the resource's lifetime may increase.

…ymentFix and tests

… OpenEntryStream

…ix and GenToolFix

bobtista · 2026-08-28T06:42:21Z

Now my comments are all resolved, and there are just a handful of bot comments remaining unaddressed eg:
Marker-controlled undo can modify/delete arbitrary rooted paths.
A successful failed-reapply rollback can delete backups belonging to the previous deployment.
Changing installations while applying can trigger an assignment ping-pong.
The legacy global marker is still shared across installations.

…ack backups, and selection revert

qodo-code-review · 2026-08-30T08:29:08Z

ⓘ Your Qodo trial ends soon. Ask your workspace admin to set up billing to keep reviews running after the trial. Manage billing

kilo-code-bot

Incremental review of 8ff682f..2cafecd (2cafecd).

kilo-code-bot · 2026-08-30T08:39:20Z

+                    Directory.CreateDirectory(markerDir);
+                }
+
+                File.Copy(globalMarker, scopedMarker, overwrite: false);


[SUGGESTION]: Migrated legacy global marker is never removed, so it resurrects stale records after every completed undo

The migration copies the global marker to the scoped marker, but nothing ever deletes or marks the global marker as consumed. Once UpdateMarkerAfterUndo deletes the scoped marker after a successful undo, the next GetMarkerPath call re-copies the stale global marker, recreating a deployment record for an already-undone installation. In the worst case, records whose backup files were consumed by the first undo now fail TryRestoreBackup (missing backup → record retained forever), so every subsequent undo reports a permanent partial failure until the fix is re-applied. Deleting the global marker after its first successful migration would also stop other installations from inheriting copies of records that point into a different installation's directories — those copies are rejected by the new containment checks and poison their scoped markers the same way.

Reply with @kilocode-bot fix it to have Kilo Code address this issue.

kilo-code-bot · 2026-08-30T08:39:20Z

+
+        if (!hasRollbackError)
+        {
+            if (Directory.Exists(backupDir) && !Directory.EnumerateFileSystemEntries(backupDir).Any())


[SUGGESTION]: Backup-directory emptiness probe can throw inside the exception and cancellation rollback paths

Directory.EnumerateFileSystemEntries(backupDir).Any() sits outside the per-entry try/catch and can throw IOException/UnauthorizedAccessException (ACL or antivirus lock on the backup dir, or the directory disappearing between the Directory.Exists check and the enumeration). RollbackDeployment runs inside ApplyInternalAsync's catch handlers, so a throw here escapes the result-pattern boundary: BaseActionSet.ApplyAsync catches it and returns new ActionSetResult(false, ex.Message), discarding all accumulated details (including the rollback narrative) and masking the original failure. Consider wrapping the probe in the same IOException/UnauthorizedAccessException handling and treating an enumeration failure as "not empty" so the directory is retained.

Reply with @kilocode-bot fix it to have Kilo Code address this issue.

…irectory probe

bobtista · 2026-08-30T15:00:07Z

Looking good - it needs a rebase, then if the bots are happy let's merge

…solver

sonarqubecloud · 2026-08-30T15:18:57Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
2.7% Duplication on New Code

See analysis details on SonarQube Cloud

kilo-code-bot · 2026-08-30T15:51:34Z

+            logger.LogError(
+                ex,
+                "Failed to resolve Community Outpost content: {Name}",
+                discoveredItem.Name);


SUGGESTION: New catch block dereferences discoveredItem without a null guard.

If a caller passes a null discoveredItem (contract violation), the NullReferenceException thrown at the top of ResolveAsync is caught here, but discoveredItem.Name in the LogError call throws a second NRE that escapes the method. The previous catch (LogError(ex, "Failed to resolve Community Outpost content")) still returned a failure result, so this refactor turns a graceful OperationResult failure into an unhandled exception for null input. Per docs/dev/result-pattern.md, contract invariants should fail eagerly, e.g. ArgumentNullException.ThrowIfNull(discoveredItem) at method entry.

Reply with @kilocode-bot fix it to have Kilo Code address this issue.

kilo-code-bot · 2026-08-30T15:51:34Z

            InstallTarget = ContentInstallTarget.Workspace,
        },

+        ["genl"] = new GenPatcherContentMetadata


SUGGESTION: New genl entry has no test coverage.

GenPatcherContentRegistryTests parameterizes the sibling Tools entries (gent, gena) across GetMetadata, GetKnownContentCodes, and category assertions, but no genl rows were added for the new GenLauncher entry. Without coverage, a typo in the code or a regression in its Tools categorization would go unnoticed.

Reply with @kilocode-bot fix it to have Kilo Code address this issue.

bobtista

LGTM 🚀