feat(launching): launch the native BGFX client against retail archives by bobtista · Pull Request #332 · community-outpost/GenHub

bobtista · 2026-07-30T02:39:58Z

Summary

Launches the community's native BGFX Zero Hour client against the user's retail data — no Wine. Final PR of the mac stack, after #328 (platform-neutral paths, manifests and Unix workspaces), #329 (macOS host) and #331 (the macOS service registrations the merged state required).

Changes

Surface native launch failures and set the loader search path. A launch that dies before drawing a window previously reported success.
Reach retail archives via the install-path environment instead of copying them. The engine reads CNC_ZH_INSTALLPATH / CNC_GENERALS_INSTALLPATH on non-Windows and mounts *.big from those roots. Zero Hour needs its own and the base Generals archives — roughly 3 GB — so without this every profile workspace had to materialise all of them.
Validate retail archive roots before spawning. Checked before spawn so a misconfigured root fails with the path named, rather than as a generic engine abort the host has to interpret.

Corrected from this PR's original description, which claimed the engine reaches its main loop with zero content and no error and that nothing was observable. Verified against bobtista/GeneralsGameCode at bobtista/topic/trunk, that is false: a root holding no archives aborts during initialisation with exit 1 and a ReleaseCrashInfo.txt, and an unmountable archive also writes [ggc] ARCHIVE MOUNT FAILED to stderr. Neither reaches the main loop.

Validating first still earns its place for reasons that hold: exit 1 is generic; the stderr sentinel exists only in StdBIGFileSystem, not Win32BIGFileSystem; ReleaseCrash shows a system-modal dialog before _exit(1), so a host-launched child on Windows hangs rather than dying; and GenHub's own immediate-exit detection is a 500 ms window. An earlier check with an actionable message avoids depending on any of that. Validation runs against the installation's declared paths, not the filtered environment — the environment builder drops a path that does not exist, so validating it alone would skip the stale-installation case this exists to catch.
Stop setting DYLD_LIBRARY_PATH / LD_LIBRARY_PATH. Measured unnecessary: the BGFX build declares every dependency as @executable_path/… with a matching rpath and launches with the variable cleared. Two costs outweighed a fallback for a build we do not ship — dyld consults it before @executable_path for leaf-name references, so a same-named library elsewhere silently wins; and the hardened runtime ignores it, so behaviour would change silently once GenHub is signed and notarized (Playwright exclusion, inside-out signing and notarization (blocker for public macOS builds) #322).
Capture complete stderr when a launch fails. The buffer was read as soon as the process exited, but the asynchronous handlers had not necessarily delivered their final lines — truncating diagnostics precisely where they explain the failure. Now drains via the parameterless WaitForExit(), which also waits for redirected-output handlers. The buffer treats a null line as end-of-stream rather than content, retains the head as well as the tail so startup context survives alongside the symptom, and bounds line length and total size.

Testing

GenHub.Tests.Core: 1427 passed, 0 failed.
GenHub.Tests.MacOS composition root: 3 passed.
Builds with 0 warnings.

The launch tests pass locally and skip in CI — please read this before trusting a green tick

Six tests exercise the real client: three launch it and assert it is still alive after 14 seconds, three assert a misconfigured launch does not survive. All six pass on a machine with a native install (GENHUB_NATIVE_CLIENT_DIR, 6/6 in 54s).

They skip in CI, which has no native fixture and no retail data. A green CI run on this PR therefore says nothing about whether launching works — it only covers the unit and composition tests. The engine-only CI smoke test that would close this gap is #324, and it is deliberately out of scope here.

Fork dependency worth stating

The install-path mechanism this PR validates is fork-only. CNC_ZH_INSTALLPATH and
CNC_GENERALS_INSTALLPATH are read by bobtista/GeneralsGameCode on bobtista/topic/trunk
and do not exist in TheSuperHackers/GeneralsGameCode@main — upstream resolves install paths
from the Windows registry only.

That is fine, since GenHub launches our client, but it bounds what the validation means:
against an upstream build these variables are ignored and the roots have no effect. The
constants and their rationale live in RetailArchiveConstants so this is discoverable rather
than implicit.

The engine behaviour the validation complements is upstream, though — the INI
filesRead == 0 throw, the unnamed enum that routes it to catch (...), and ReleaseCrash
showing a system-modal dialog before _exit(1) are all in upstream main. So the reasons
for checking before spawn rather than relying on the child's exit hold for any build.

Risks and rollback

Revert-safe; no persisted-data conversion. The archive-root validation stops launches before spawn that would previously have failed only after spawn, with a generic engine crash naming nothing — intended, and the validation message names the offending root.

Centralizes the retail archive environment-variable contract and normalizes configured roots.
Validates applicable archive roots before spawning native clients, with Windows excluded from the non-Windows contract.
Captures bounded stderr output and reports immediate process failures.
Adds unit and opt-in native integration coverage for archive roots, manifests, permissions, and diagnostics.

Confidence Score: 3/5

The PR is not yet safe to merge because native Zero Hour can still be spawned without any validated source of its required base Generals archives.

The current validation skips an absent GeneralsPath, while supported engine-only workspaces contain no archives and rely entirely on environment-provided retail roots; that leaves the previously reported missing-base-content startup failure reachable.

Files Needing Attention: GenHub/GenHub/Features/Launching/GameLauncher.cs; GenHub/GenHub.Tests/GenHub.Tests.Core/Features/Launching/RetailArchiveRootValidationTests.cs

Important Files Changed

Filename	Overview
GenHub/GenHub/Features/Launching/GameLauncher.cs	Builds and validates native retail archive environments; the previous missing-Generals-root failure remains reachable.
GenHub/GenHub/Features/GameProfiles/Infrastructure/GameProcessManager.cs	Adds Unix execute-permission checks, bounded asynchronous stderr capture, and clearer immediate-exit handling.
GenHub/GenHub.Core/Constants/RetailArchiveConstants.cs	Centralizes archive environment names and case-insensitive top-level archive enumeration settings.
GenHub/GenHub.Tests/GenHub.Tests.Core/Features/Launching/RetailArchiveRootValidationTests.cs	Covers archive-root validation behavior but codifies acceptance of a Zero Hour launch with no Generals root.
GenHub/GenHub.Tests/GenHub.Tests.Core/Features/GameProfiles/RetailArchiveRootTests.cs	Adds opt-in native coverage proving engine-only workspaces rely on both retail archive environment roots.

Sequence Diagram

sequenceDiagram
    participant Launcher as GameLauncher
    participant Install as GameInstallation
    participant Process as GameProcessManager
    participant Engine as Native BGFX Client
    Launcher->>Install: Read Zero Hour / Generals roots
    Launcher->>Launcher: Build normalized environment
    Launcher->>Launcher: Validate declared archive roots
    alt Validation fails
        Launcher-->>Launcher: Return actionable launch error
    else Validation passes
        Launcher->>Process: StartProcessAsync(configuration)
        Process->>Engine: Spawn with redirected stderr
        alt Immediate exit
            Process->>Process: Drain and bound stderr
            Process-->>Launcher: Return failure diagnostics
        else Process survives startup window
            Process-->>Launcher: Return tracked process
        end
    end

_{Reviews (10): Last reviewed commit: "chore(launching): reference launch const..." | Re-trigger Greptile}

Context used:

Knowledge Base — Game Profiles and Launching

…arch path

…tance lock

…nstead of copying them

Measured unnecessary: the BGFX build declares every dependency as @executable_path/ with a matching rpath and launches with the variable cleared. Two costs outweighed a fallback for a build we do not ship: dyld consults DYLD_LIBRARY_PATH before @executable_path for leaf-name references, so a same-named library elsewhere silently wins; and the hardened runtime ignores it, so behaviour would change silently once GenHub is signed and notarized. Closes #317.

…gine The engine reaches its main loop with zero content and no error when the archive roots are wrong: no exit code, no log line, nothing a host process can observe. Launch success therefore cannot distinguish a working game from an empty one, so the roots are checked before spawn and the launch fails with the offending root named. Presence of at least one .big archive is the sentinel rather than a specific filename, which varies by localisation and version. An archive that exists but fails to mount still needs a machine-readable failure from the engine. Closes #315.

The buffer was read as soon as the process exited, but the asynchronous stderr handlers had not necessarily delivered their final lines — truncating diagnostics precisely on the path where they explain the failure. Wait via the parameterless WaitForExit overload, which also waits for redirected-output handlers, before reading. The buffer now treats a null line as the end-of-stream signal rather than content, retains the head as well as the tail so startup context survives alongside the failure, and bounds line length and total size so a pathological writer cannot exhaust memory. Only stderr is redirected, so there is no stdout stream to drain. Closes #318.

Adds an end-to-end check that a process failing immediately still reports both the head and tail of its stderr. It does not deterministically pin the end-of-stream drain: removing the WaitForExit() call leaves it passing on macOS and .NET 8, because the handlers complete before the capture is read even at twenty thousand lines. The drain stands on the documented contract instead. The GameLauncher fixture now uses a real retail root holding an archive. It previously pointed at a Windows path that does not exist off Windows, which the new pre-spawn validation correctly rejected. Clears the warnings this stack introduced; development itself builds clean. Two remain, both predating these fixes and needing invasive reordering of long files: SA1204 in GameLauncher and SA1202 in WorkspaceStrategyBase.

…alization change EnsureExecutableAsync became protected when the rethrow was added, which put it after a private helper. Moved it above, and dropped the trailing blank line the earlier reordering left.

coderabbitai · 2026-07-30T02:40:09Z

📝 Walkthrough

Summary by CodeRabbit

New Features
- Improved non-Windows retail-archive discovery using exported install-root environment variables and a standard archive search pattern.
Bug Fixes
- Added fast non-Windows checks for missing/unexecutable engine files.
- Added pre-launch validation for required archive roots, failing early when archives are missing or unreadable.
- Enhanced startup failure diagnostics with more reliable stderr capture and bounded error excerpts.
Tests
- Expanded integration and unit coverage for native client launching, archive discovery/validation, stderr capture timing, and bounded error buffering.

Walkthrough

Adds cross-platform retail archive-root constants and pre-launch validation, bounded stderr diagnostics and Unix permission checks, case-insensitive installation discovery, and native-client integration tests.

Changes

Native launch support

Layer / File(s)	Summary
Retail archive contract and validation `GenHub/GenHub.Core/Constants/RetailArchiveConstants.cs`, `GenHub/GenHub/Features/Launching/GameLauncher.cs`, `GenHub/GenHub.Tests/GenHub.Tests.Core/Features/Launching/*`, `GenHub/GenHub.Tests/GenHub.Tests.Core/Features/GameProfiles/RetailArchiveRootTests.cs`	Defines archive variables, propagates and validates non-Windows archive roots, and tests valid, invalid, missing, and engine-only archive scenarios.
Native process diagnostics and bounded stderr `GenHub/GenHub/Features/GameProfiles/Infrastructure/GameProcessManager.cs`, `GenHub/GenHub.Tests/GenHub.Tests.Core/Features/GameProfiles/ErrorTests.cs`, `GenHub/GenHub.Tests/GenHub.Tests.Core/Features/GameProfiles/NativeLaunchDiagnosticsTests.cs`	Checks execute permissions, captures bounded stderr, drains output deterministically, and tests truncation, race handling, diagnostics, and deadlock prevention.
Native client discovery and integration `GenHub/GenHub.Tests/GenHub.Tests.Core/Features/GameProfiles/NativeClient*.cs`	Adds optional native-client discovery, serialized launch tests, manifest resolution, runtime filtering, and real-client startup checks.
Installation and validation coverage `GenHub/GenHub/Features/GameProfiles/ViewModels/GameProfileLauncherViewModel.cs`, `GenHub/GenHub.Tests/GenHub.Tests.Core/Features/Manifest/ContentManifestPoolTests.cs`, `GenHub/GenHub.Tests/GenHub.Tests.Core/Features/Workspace/WorkspaceValidatorTests.cs`	Uses case-insensitive executable detection and extends manifest-ingestion and workspace test coverage.

Estimated code review effort: 4 (Complex) | ~60 minutes

Possibly related issues

community-outpost/GenHub#330 — Shares the retail archive discovery constants and archive-root contract.
community-outpost/GenHub#327 — Covers overlapping native launch, archive validation, and stderr diagnostic changes.
community-outpost/GenHub#333 — Matches the Zero Hour and Generals archive-root validation behavior.
community-outpost/GenHub#324 — Covers native engine launch and failure diagnostics on macOS CI.

Possibly related PRs

community-outpost/GenHub#261 — Modifies the same immediate-exit and stderr diagnostic path in GameProcessManager.

Suggested labels: Enhancement, Testing

Poem

I’m a rabbit with archives tucked tight,
Launching the engine by moonlight.
Stderr trails from tail to head,
Empty roots are caught instead.
Hop, test, and run just right!

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	The changes implement prelaunch retail-root validation, remove launch-path env vars, and fix stderr draining and buffering as requested.
Out of Scope Changes check	✅ Passed	The added tests, constants, and docs all support the launch changes; no unrelated code changes stand out.
Docstring Coverage	✅ Passed	Docstring coverage is 86.79% which is sufficient. The required threshold is 50.00%.
Title check	✅ Passed	The title uses conventional commit format and accurately summarizes the native BGFX retail-archive launch work.
Description check	✅ Passed	The description is clearly related to the changeset and matches the native launch, retail archive, and stderr diagnostics work.

📋 Issue Planner

Let us write the prompt for your AI agent so you can ship faster (with fewer bugs).

View plans for tickets: #315, #317, #318

✨ Finishing Touches

📝 Generate docstrings

Create stacked PR
Commit on current branch

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch feat/native-launch

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

…alize profile overrides Two defects from review, both introduced by the earlier validation change. Windows never reads CNC_ZH_INSTALLPATH or CNC_GENERALS_INSTALLPATH — it resolves install paths from the registry — so a Windows layout without loose top-level archives is not a misconfiguration. BuildEnvironmentVariables returns before setting them there, but validation reads the installation's declared paths, so it needed the same guard rather than inheriting it. Without it, valid Windows launches were rejected with a misleading message about missing archives. A profile setting a root explicitly chooses the directory, not whether the trailing separator is applied. AddArchiveRoot returned early on an existing value and never normalized it, so the engine concatenated it with the archive filename to produce paths like '/path/toINIZH.big' and silently mounted nothing — precisely the failure this validation exists to prevent, reachable through the override path. The variable names are an external contract with the engine, so they move to RetailArchiveConstants alongside the archive search pattern. The normalization test fails against the previous early-return. The Windows guard asserts only on Windows and is a no-op elsewhere.

bobtista · 2026-07-30T02:47:33Z

Addressed the Greptile findings. Both P1s were real and both were introduced by the archive-root validation change in this PR, so thanks — these would have shipped.

P1: Windows archive validation blocks launches. Confirmed. BuildEnvironmentVariables returns before setting the variables on Windows, but the validation reads the installation's declared paths rather than the built environment — deliberately, to catch a stale root that the environment builder silently drops — so it did not inherit that guard. On Windows, where install paths come from the registry and these variables are never read, a layout without loose top-level archives would have been rejected with a misleading message. Now guarded explicitly, with the reason recorded at the guard.

P1: profile archive roots remain unnormalized. Confirmed, and it defeated the purpose of the check. AddArchiveRoot returned early when the profile had already set the variable, so it never appended the separator; validation then accepted the value because it only tests existence and archive presence. The engine concatenates the value with the archive filename directly, so /path/to became /path/toINIZH.big and mounted nothing — the exact silent failure this PR exists to prevent, reachable through the override path. A profile override now chooses the directory but not whether the separator is applied.

P2: centralize archive environment constants. Agreed — these names are an external contract with the engine, not local strings. Moved to RetailArchiveConstants with the *.big search pattern, and the rationale for why Windows is excluded lives there too.

Verification: the normalization test fails against the previous early-return, so it pins the fix rather than merely passing. 1423 unit tests pass, 0 warnings. The Windows guard asserts only on Windows and is a no-op elsewhere, so CI's Windows job is what exercises it.

Two of these asserted that validation rejects a bad root, which stopped being true on Windows once validation was guarded there — the platform guard was added without updating the tests that asserted the previous cross-platform behaviour, and Build Windows caught it. Rejection is non-Windows behaviour by design; the Windows side is asserted separately by Validate_OnWindows_SkipsEntirely. The guards do not hollow the tests out: on non-Windows they still assert, and three still fail if the validation fix is reverted.

kilo-code-bot · 2026-07-30T03:05:10Z