fix(macos): register the platform services the merged state requires - #331
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe macOS dependency injection module now registers game path, symlink, and Unix file operation services. Composition assertions require the new services, and CI runs macOS tests before publishing. ChangesmacOS service registration
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 @.github/workflows/ci.yml:
- Around line 418-421: Update the generic test loop in the CI workflow to also
exclude the macOS test project, while preserving the dedicated “Run macOS Tests”
pre-publish step. Use the existing project-name filtering alongside the Windows
and Linux exclusions so GenHub.Tests.MacOS.csproj is executed only once.
🪄 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: 9aa6e09a-7550-409a-9858-bb2deecd78c4
📒 Files selected for processing (3)
.github/workflows/ci.ymlGenHub/GenHub.MacOS/Infrastructure/DependencyInjection/MacOSServicesModule.csGenHub/GenHub.Tests/Shared/CompositionRootAssertions.cs
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (3 files)
Previous Review Summary (commit da7102a)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit da7102a)Status: No Issues Found | Recommendation: Merge Files Reviewed (3 files)
Reviewed by glm-4.7 · Input: 21.9K · Output: 2.9K · Cached: 271K |
#328 made IGamePathProvider a required dependency and #329 added the macOS host, but neither could carry the macOS registrations: they reference types from the first and a module from the second, so they only compile once both are in. Each PR was green in isolation and the merged state is not — GenHub.app now aborts at container build with 'Unable to resolve service for type IGamePathProvider while attempting to activate GameSettingsService', cascading across roughly twenty descriptors including MainViewModel and IGameLauncher. Registers IGamePathProvider, ISymlinkCapabilityProvider and IFileOperationsService for macOS. Only the first breaks startup; without the other two macOS silently copies instead of hard-linking and cannot reach symlink strategies, which are the behaviours #328 set out to fix. Also moves the macOS test run before publish. GenHub.Tests.MacOS was already covered by the job's final 'Run Tests' step, but that sits after Publish and Smoke Test App Launch, so the startup crash killed the job first and the assertion naming the service never ran. MacOSHost_ResolvesEveryRequiredService already asserted this; it was reached too late to be useful. The project is skipped in the later sweep so it runs once.
da7102a to
e82b75a
Compare
Summary
developmentis currently broken on macOS.GenHub.appaborts at container build:This cascades across roughly twenty service descriptors, including
MainViewModel,IGameLauncherandIGameProfileManager. TheBuild macOSjob ondevelopmentis red.Why neither #328 nor #329 could have caught it
#328 made
IGamePathProvidera required constructor dependency — deliberately, so an unregistered provider fails at container build instead of silently resolving to the Windows implementation. #329 added the macOS host and its DI module.The macOS registrations reference types introduced by #328 and a module introduced by #329, so they compile only once both are merged. Neither PR could carry them, and both were genuinely green in isolation. The failure exists only in the merged state, which per-branch CI cannot reach.
Changes
Register
IGamePathProvider,ISymlinkCapabilityProviderandIFileOperationsServicefor macOS.Only
IGamePathProviderbreaks startup. Without the other two, macOS silently copies instead of hard-linking and cannot reach symlink strategies — the exact behaviours feat(platform): make paths, manifests, and Unix workspaces platform-aware #328 set out to fix — so all three land together.Add the missing
typeofentries to the shared composition-root assertions.Move the macOS test run before publish.
GenHub.Tests.MacOSwas already covered by the job's finalRun Testsstep, but that step sits afterPublish macOS AppandSmoke Test App Launch. The startup crash killed the job at the smoke test, so the assertion that names the unresolvable service never executed — the breakage surfaced only as a status-134 crash after packaging.MacOSHost_ResolvesEveryRequiredServicealready asserted exactly this; it was simply reached too late to be useful. Running it before publish fails in seconds and names the service. The project is now skipped in the later sweep so it runs once rather than twice.Corrected from the original description of this PR, which claimed the project ran nowhere in CI. It did run — just too late to matter.
Testing
MacOSCompositionRootTests: 3 passed. Fails ondevelopmentwithout this change.GenHub.Tests.Core: 1408 passed.GenHub.MacOSbuilds with 0 warnings.Build macOSpasses on this branch, against red ondevelopment.Risks and rollback
Registration-only, scoped to the macOS module; no behaviour change on Windows or Linux. Revert-safe, though reverting restores the startup crash.
Related
Unblocks the native-launch PR, which is where these registrations were originally staged. Alpha 4 cannot ship until this lands — #229's promote gate requires a green CI run for the exact release SHA.
Greptile Summary
Registers the macOS implementations required by shared game-settings and workspace services.
Confidence Score: 5/5
The PR appears safe to merge, with the new macOS registrations and CI coverage consistent with existing platform patterns.
The macOS composition root now supplies each required service with available dependencies, all platform modules satisfy the expanded assertions, and the only current macOS test project remains executed exactly once in its native CI job.
Important Files Changed
Reviews (1): Last reviewed commit: "fix(macos): register the platform servic..." | Re-trigger Greptile
Context used (3)