test(coverage): wave 7 — async handlers + final push (80.8% branch)#136
Merged
Prekzursil merged 1 commit intomainfrom Apr 4, 2026
Merged
test(coverage): wave 7 — async handlers + final push (80.8% branch)#136Prekzursil merged 1 commit intomainfrom
Prekzursil merged 1 commit intomainfrom
Conversation
…ranch) Wave 7A: RuntimeAdapter async execution paths (116 test cases) - ExecuteByRouteAsync all backend switch arms - ExecuteExtenderBackendActionAsync success/null guard - ExecuteLegacyBackendActionAsync all ExecutionKind arms - ProbeCapabilitiesAsync inference paths - TryCreateMechanicBlockedResultAsync exception catches - AttachAsync/DetachAsync lifecycle Wave 7B: Package gap-filling (88 tests) - Flow 94.8->97.9%, Meg 87.1->88.7%, Saves 92.8->93.6% - Core, Profiles, DataIndex, App record constructors + edge cases Overall: Line 81.8%, Branch 80.8%, 3,900+ tests. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Unable to trigger custom agent "Code Reviewer". You have run out of credits 😔 |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughThis PR adds comprehensive test coverage across 8 subsystem areas (App, Core, DataIndex, Flow, Meg, Profiles, Runtime, Saves) through new Wave 7 final test classes, targeting edge cases, failure paths, and coverage gaps in view models, services, and core components. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
✨ 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 |
Review Summary by Qodotest(coverage): wave 7 — async handlers + final push (80.8% branch coverage)
WalkthroughsDescription• Comprehensive test coverage wave 7 with 200+ new tests achieving 80.8% branch coverage and 81.8% line coverage • **RuntimeAdapter async execution paths**: 1868 lines covering ExecuteByRouteAsync, ExecuteExtenderBackendActionAsync, ExecuteMemoryActionAsync, ExecuteSdkActionAsync, and ExecuteLegacyBackendActionAsync with lifecycle methods and exception handling • **Core layer**: Record constructors, default interface method overloads for IActionReliabilityService, IRuntimeAdapter, ISavePatchPackService, and IModOnboardingService • **Save operations**: Exception handling for SavePatchApplyService helpers, contract validation for SavePatchPackService, and edge cases with locked files and schema mismatches • **Profile updates**: GitHubProfileUpdateService extraction paths, security validation for path traversal, and IO exception handling • **MEG archive reader**: Edge case coverage for locked files, payload parsing exceptions, format fallback paths, and entry record validation • **App layer**: View model record constructors and runtime mode override helpers with file persistence edge cases • **DataIndex service**: Loose entries path handling, MEG file diagnostics propagation, and fallback behavior • **Flow service**: Story flow graph models, exporter mode-specific feature ID generation, and null root handling Diagramflowchart LR
A["Test Coverage Wave 7"] --> B["RuntimeAdapter Async Paths"]
A --> C["Core Layer Models"]
A --> D["Save Operations"]
A --> E["Profile Updates"]
A --> F["MEG Archive Reader"]
A --> G["App Layer VMs"]
A --> H["DataIndex Service"]
A --> I["Flow Service"]
B --> J["80.8% Branch Coverage"]
C --> J
D --> J
E --> J
F --> J
G --> J
H --> J
I --> J
File Changes1. tests/SwfocTrainer.Tests/Runtime/RuntimeAdapterAsyncWave7Tests.cs
|
Code Review by Qodo
1. CreateAttachedAdapter too many params
|
 |
|
Overall Grade |
Security Reliability Complexity Hygiene |
Code Review Summary
| Analyzer | Status | Updated (UTC) | Details |
|---|---|---|---|
| Terraform |  |
Apr 4, 2026 3:01p.m. | Review ↗ |
| SQL | |
Apr 4, 2026 3:01p.m. | Review ↗ |
| Rust | |
Apr 4, 2026 3:01p.m. | Review ↗ |
| Shell | |
Apr 4, 2026 3:01p.m. | Review ↗ |
| Ruby | |
Apr 4, 2026 3:01p.m. | Review ↗ |
| PHP | |
Apr 4, 2026 3:01p.m. | Review ↗ |
| Kotlin | |
Apr 4, 2026 3:01p.m. | Review ↗ |
| Swift | |
Apr 4, 2026 3:01p.m. | Review ↗ |
| Scala | |
Apr 4, 2026 3:01p.m. | Review ↗ |
| Python | |
Apr 4, 2026 3:01p.m. | Review ↗ |
| JavaScript | |
Apr 4, 2026 3:01p.m. | Review ↗ |
| Java | |
Apr 4, 2026 3:01p.m. | Review ↗ |
| Go | |
Apr 4, 2026 3:01p.m. | Review ↗ |
| Docker | |
Apr 4, 2026 3:01p.m. | Review ↗ |
| C & C++ | |
Apr 4, 2026 3:01p.m. | Review ↗ |
| Ansible | |
Apr 4, 2026 3:01p.m. | Review ↗ |
❌ 12 blocking issues (12 total)
|
![qltysh[bot]](https://avatars.githubusercontent.com/in/890766?s=60&v=4){kind=small}
| IHelperBridgeBackend? helperBackend = null, | ||
| IModMechanicDetectionService? mechanicService = null, | ||
| ISdkOperationRouter? sdkRouter = null, | ||
| IExecutionBackend? executionBackend = null, |
| CooldownMs: 0), | ||
| Payload: payload, | ||
| ProfileId: "profile", | ||
| RuntimeMode: runtimeMode); |
| finally | ||
| { | ||
| Environment.SetEnvironmentVariable("SWFOC_EXPERT_MUTATION_OVERRIDES", previous); | ||
| } |
| finally | ||
| { | ||
| Environment.SetEnvironmentVariable("SWFOC_EXPERT_MUTATION_OVERRIDES", previous); | ||
| } |
| CancellationToken.None); | ||
|
|
||
| result.Succeeded.Should().BeTrue(); | ||
| result.Diagnostics.Should().ContainKey("helperEntryPoint"); |
| CancellationToken.None); | ||
|
|
||
| result.Diagnostics.Should().ContainKey("contextRoutedAction"); | ||
| result.Diagnostics!["contextRoutedAction"]!.ToString().Should().Be("spawn_galactic_entity"); |
| CancellationToken.None); | ||
|
|
||
| result.Diagnostics.Should().ContainKey("contextRoutedAction"); | ||
| result.Diagnostics!["contextRoutedAction"]!.ToString().Should().Be("set_selected_owner_faction"); |
| finally | ||
| { | ||
| Directory.Delete(tempDir, true); | ||
| } |
| finally | ||
| { | ||
| Directory.Delete(tempDir, true); | ||
| } |
| finally | ||
| { | ||
| Directory.Delete(tempDir, true); | ||
| } |
|
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| Compatibility | 8 medium |
| UnusedCode | 2 medium |
| BestPractice | 35 medium |
| Security | 3 high |
| CodeStyle | 2 minor |
| Complexity | 2 medium |
🔴 Metrics 264 complexity · 95 duplication
Metric Results Complexity ⚠️ 264 (≤ 10 complexity)Duplication ✅ 95 (≤ 0 duplication)
TIP This summary will be updated as you push new changes. Give us feedback
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+33
to
+109
| private static RuntimeAdapter CreateAttachedAdapter( | ||
| RuntimeMode mode = RuntimeMode.Galactic, | ||
| TrainerProfile? profile = null, | ||
| IBackendRouter? router = null, | ||
| IHelperBridgeBackend? helperBackend = null, | ||
| IModMechanicDetectionService? mechanicService = null, | ||
| ISdkOperationRouter? sdkRouter = null, | ||
| IExecutionBackend? executionBackend = null, | ||
| bool includeExecutionBackend = true) | ||
| { | ||
| profile ??= BuildProfile("set_credits"); | ||
| var services = new Dictionary<Type, object> | ||
| { | ||
| [typeof(IBackendRouter)] = router ?? new StubBackendRouter( | ||
| new BackendRouteDecision(true, ExecutionBackendKind.Memory, RuntimeReasonCode.CAPABILITY_PROBE_PASS, "ok")), | ||
| [typeof(IHelperBridgeBackend)] = helperBackend ?? new StubHelperBridgeBackend(), | ||
| [typeof(IModDependencyValidator)] = new StubDependencyValidator( | ||
| new DependencyValidationResult(DependencyValidationStatus.Pass, "", new HashSet<string>(StringComparer.OrdinalIgnoreCase))), | ||
| [typeof(ITelemetryLogTailService)] = new StubTelemetryLogTailService() | ||
| }; | ||
|
|
||
| if (includeExecutionBackend) | ||
| { | ||
| services[typeof(IExecutionBackend)] = executionBackend ?? new StubExecutionBackend(); | ||
| } | ||
|
|
||
| if (mechanicService is not null) | ||
| { | ||
| services[typeof(IModMechanicDetectionService)] = mechanicService; | ||
| } | ||
|
|
||
| if (sdkRouter is not null) | ||
| { | ||
| services[typeof(ISdkOperationRouter)] = sdkRouter; | ||
| } | ||
|
|
||
| var adapter = new RuntimeAdapter( | ||
| new StubProcessLocator(), | ||
| new StubProfileRepository(profile), | ||
| new StubSignatureResolver(), | ||
| NullLogger<RuntimeAdapter>.Instance, | ||
| new MapServiceProvider(services)); | ||
|
|
||
| var symbolMap = new SymbolMap(new Dictionary<string, SymbolInfo>(StringComparer.OrdinalIgnoreCase) | ||
| { | ||
| ["credits"] = new SymbolInfo("credits", (nint)0x1000, SymbolValueType.Int32, AddressSource.Signature, Confidence: 0.95), | ||
| ["unit_cap"] = new SymbolInfo("unit_cap", (nint)0x2000, SymbolValueType.Int32, AddressSource.Signature, Confidence: 0.95), | ||
| ["fog_reveal"] = new SymbolInfo("fog_reveal", (nint)0x3000, SymbolValueType.Byte, AddressSource.Signature, Confidence: 0.95), | ||
| ["test_float"] = new SymbolInfo("test_float", (nint)0x7000, SymbolValueType.Float, AddressSource.Signature, Confidence: 0.95), | ||
| ["test_bool"] = new SymbolInfo("test_bool", (nint)0x9000, SymbolValueType.Bool, AddressSource.Signature, Confidence: 0.95) | ||
| }); | ||
|
|
||
| var session = new AttachSession( | ||
| "profile", | ||
| new ProcessMetadata( | ||
| ProcessId: Environment.ProcessId, | ||
| ProcessName: "swfoc", | ||
| ProcessPath: @"C:\Games\swfoc.exe", | ||
| CommandLine: null, | ||
| ExeTarget: ExeTarget.Swfoc, | ||
| Mode: mode, | ||
| Metadata: new Dictionary<string, string>(StringComparer.OrdinalIgnoreCase)), | ||
| new ProfileBuild("profile", "build", @"C:\Games\swfoc.exe", ExeTarget.Swfoc), | ||
| symbolMap, | ||
| DateTimeOffset.UtcNow); | ||
|
|
||
| typeof(RuntimeAdapter) | ||
| .GetProperty("CurrentSession", BindingFlags.Instance | BindingFlags.Public | BindingFlags.NonPublic)! | ||
| .SetValue(adapter, session); | ||
| SetField(adapter, "_attachedProfile", profile); | ||
|
|
||
| var memType = typeof(RuntimeAdapter).Assembly.GetType("SwfocTrainer.Runtime.Interop.ProcessMemoryAccessor")!; | ||
| var accessor = RuntimeHelpers.GetUninitializedObject(memType); | ||
| SetField(adapter, "_memory", accessor); | ||
|
|
||
| return adapter; | ||
| } |
There was a problem hiding this comment.
1. createattachedadapter too many params 📘 Rule violation ⚙ Maintainability
CreateAttachedAdapter has 8 parameters and spans well over 50 lines, exceeding the repo’s method size/parameter limits. This increases maintenance burden and makes future changes harder to test and review.
Agent Prompt
## Issue description
`CreateAttachedAdapter` exceeds the method-size and parameter-count limits (over 50 lines and more than 5 parameters).
## Issue Context
This helper is newly added in the PR and will be a central test utility; keeping it small/composable reduces future test brittleness.
## Fix Focus Areas
- tests/SwfocTrainer.Tests/Runtime/RuntimeAdapterAsyncWave7Tests.cs[33-109]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+26
to
+106
| private static void SetField(object target, string name, object? value) | ||
| { | ||
| var field = target.GetType().GetField(name, BindingFlags.Instance | BindingFlags.NonPublic); | ||
| field.Should().NotBeNull($"field '{name}' should exist"); | ||
| field!.SetValue(target, value); | ||
| } | ||
|
|
||
| private static RuntimeAdapter CreateAttachedAdapter( | ||
| RuntimeMode mode = RuntimeMode.Galactic, | ||
| TrainerProfile? profile = null, | ||
| IBackendRouter? router = null, | ||
| IHelperBridgeBackend? helperBackend = null, | ||
| IModMechanicDetectionService? mechanicService = null, | ||
| ISdkOperationRouter? sdkRouter = null, | ||
| IExecutionBackend? executionBackend = null, | ||
| bool includeExecutionBackend = true) | ||
| { | ||
| profile ??= BuildProfile("set_credits"); | ||
| var services = new Dictionary<Type, object> | ||
| { | ||
| [typeof(IBackendRouter)] = router ?? new StubBackendRouter( | ||
| new BackendRouteDecision(true, ExecutionBackendKind.Memory, RuntimeReasonCode.CAPABILITY_PROBE_PASS, "ok")), | ||
| [typeof(IHelperBridgeBackend)] = helperBackend ?? new StubHelperBridgeBackend(), | ||
| [typeof(IModDependencyValidator)] = new StubDependencyValidator( | ||
| new DependencyValidationResult(DependencyValidationStatus.Pass, "", new HashSet<string>(StringComparer.OrdinalIgnoreCase))), | ||
| [typeof(ITelemetryLogTailService)] = new StubTelemetryLogTailService() | ||
| }; | ||
|
|
||
| if (includeExecutionBackend) | ||
| { | ||
| services[typeof(IExecutionBackend)] = executionBackend ?? new StubExecutionBackend(); | ||
| } | ||
|
|
||
| if (mechanicService is not null) | ||
| { | ||
| services[typeof(IModMechanicDetectionService)] = mechanicService; | ||
| } | ||
|
|
||
| if (sdkRouter is not null) | ||
| { | ||
| services[typeof(ISdkOperationRouter)] = sdkRouter; | ||
| } | ||
|
|
||
| var adapter = new RuntimeAdapter( | ||
| new StubProcessLocator(), | ||
| new StubProfileRepository(profile), | ||
| new StubSignatureResolver(), | ||
| NullLogger<RuntimeAdapter>.Instance, | ||
| new MapServiceProvider(services)); | ||
|
|
||
| var symbolMap = new SymbolMap(new Dictionary<string, SymbolInfo>(StringComparer.OrdinalIgnoreCase) | ||
| { | ||
| ["credits"] = new SymbolInfo("credits", (nint)0x1000, SymbolValueType.Int32, AddressSource.Signature, Confidence: 0.95), | ||
| ["unit_cap"] = new SymbolInfo("unit_cap", (nint)0x2000, SymbolValueType.Int32, AddressSource.Signature, Confidence: 0.95), | ||
| ["fog_reveal"] = new SymbolInfo("fog_reveal", (nint)0x3000, SymbolValueType.Byte, AddressSource.Signature, Confidence: 0.95), | ||
| ["test_float"] = new SymbolInfo("test_float", (nint)0x7000, SymbolValueType.Float, AddressSource.Signature, Confidence: 0.95), | ||
| ["test_bool"] = new SymbolInfo("test_bool", (nint)0x9000, SymbolValueType.Bool, AddressSource.Signature, Confidence: 0.95) | ||
| }); | ||
|
|
||
| var session = new AttachSession( | ||
| "profile", | ||
| new ProcessMetadata( | ||
| ProcessId: Environment.ProcessId, | ||
| ProcessName: "swfoc", | ||
| ProcessPath: @"C:\Games\swfoc.exe", | ||
| CommandLine: null, | ||
| ExeTarget: ExeTarget.Swfoc, | ||
| Mode: mode, | ||
| Metadata: new Dictionary<string, string>(StringComparer.OrdinalIgnoreCase)), | ||
| new ProfileBuild("profile", "build", @"C:\Games\swfoc.exe", ExeTarget.Swfoc), | ||
| symbolMap, | ||
| DateTimeOffset.UtcNow); | ||
|
|
||
| typeof(RuntimeAdapter) | ||
| .GetProperty("CurrentSession", BindingFlags.Instance | BindingFlags.Public | BindingFlags.NonPublic)! | ||
| .SetValue(adapter, session); | ||
| SetField(adapter, "_attachedProfile", profile); | ||
|
|
||
| var memType = typeof(RuntimeAdapter).Assembly.GetType("SwfocTrainer.Runtime.Interop.ProcessMemoryAccessor")!; | ||
| var accessor = RuntimeHelpers.GetUninitializedObject(memType); | ||
| SetField(adapter, "_memory", accessor); |
There was a problem hiding this comment.
2. Null-forgiving ! in runtime tests 📘 Rule violation ☼ Reliability
The new tests introduce multiple uses of the null-forgiving operator (!), which violates the explicit null-safety requirement. This can mask real nullability issues and bypass required guards.
Agent Prompt
## Issue description
The tests use the null-forgiving operator (`!`) in multiple places, which is disallowed by the null-safety compliance rule.
## Issue Context
Most of these are reflection lookups and nullable values where a direct `?? throw` or other explicit guard can preserve safety without suppressing nullability.
## Fix Focus Areas
- tests/SwfocTrainer.Tests/Runtime/RuntimeAdapterAsyncWave7Tests.cs[26-31]
- tests/SwfocTrainer.Tests/Runtime/RuntimeAdapterAsyncWave7Tests.cs[99-106]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+74
to
+116
| [Fact] | ||
| public void Load_FileNotFound_ShouldReturnAuto() | ||
| { | ||
| // Temporarily override the settings path to a nonexistent location | ||
| // Since GetSettingsPath is private, we test via the public Load method | ||
| // which will use the actual app data path | ||
| var result = MainViewModelRuntimeModeOverrideHelpers.Load(); | ||
| // Should return a valid string (either "auto" or whatever is currently saved) | ||
| result.Should().NotBeNullOrWhiteSpace(); | ||
| } | ||
|
|
||
| [Fact] | ||
| public void Save_ThenLoad_ShouldRoundTrip() | ||
| { | ||
| // Save a known value then load it back | ||
| var originalValue = MainViewModelRuntimeModeOverrideHelpers.Load(); | ||
| try | ||
| { | ||
| MainViewModelRuntimeModeOverrideHelpers.Save("Galactic"); | ||
| var loaded = MainViewModelRuntimeModeOverrideHelpers.Load(); | ||
| loaded.Should().Be("Galactic"); | ||
| } | ||
| finally | ||
| { | ||
| // Restore original | ||
| MainViewModelRuntimeModeOverrideHelpers.Save(originalValue); | ||
| } | ||
| } | ||
|
|
||
| [Fact] | ||
| public void Save_NullValue_ShouldNormalizeToAuto() | ||
| { | ||
| var originalValue = MainViewModelRuntimeModeOverrideHelpers.Load(); | ||
| try | ||
| { | ||
| MainViewModelRuntimeModeOverrideHelpers.Save(null); | ||
| var loaded = MainViewModelRuntimeModeOverrideHelpers.Load(); | ||
| loaded.Should().Be("Auto"); | ||
| } | ||
| finally | ||
| { | ||
| MainViewModelRuntimeModeOverrideHelpers.Save(originalValue); | ||
| } |
There was a problem hiding this comment.
3. Appdata settings test race 🐞 Bug ☼ Reliability
AppWave7FinalTests reads/writes the real LocalAppData runtime-mode settings file, which can race with other test classes that also call MainViewModelRuntimeModeOverrideHelpers.Save/Load and cause flaky assertions and persistent user-machine side effects. This is especially risky when tests execute concurrently.
Agent Prompt
### Issue description
`MainViewModelRuntimeModeOverrideHelpers.Save/Load` persists to a real per-user LocalAppData file (`runtime-mode-settings.json`). Multiple test classes call these methods, so tests can interfere with each other (and with a developer’s real settings) when the runner executes classes concurrently.
### Issue Context
- The helper writes to `Path.Join(TrustedPathPolicy.GetOrCreateAppDataRoot(), "runtime-mode-settings.json")`.
- `AppWave7FinalTests` adds more Save/Load tests, increasing the collision window.
### Fix Focus Areas
- tests/SwfocTrainer.Tests/App/AppWave7FinalTests.cs[74-116]
- tests/SwfocTrainer.Tests/App/MainViewModelRuntimeModeOverrideBranchTests.cs[86-119]
- src/SwfocTrainer.App/ViewModels/MainViewModelRuntimeModeOverrideHelpers.cs[65-109]
### Suggested fix
1. Put *all* tests that touch `MainViewModelRuntimeModeOverrideHelpers.Save/Load` into a shared xUnit collection with parallelization disabled (e.g., `[Collection("RuntimeModeOverrideSettings")]` + `[CollectionDefinition(..., DisableParallelization=true)]`).
2. In each test, snapshot the settings file contents (or whether it exists) at start and restore it in `finally` (restore the file contents, not just the mode string), so developer machines/CI agents are not left mutated.
3. (Optional but best) Refactor `MainViewModelRuntimeModeOverrideHelpers` to allow overriding the settings path for tests (e.g., internal setter / internal `GetSettingsPath` delegate) so tests can use a temp directory instead of LocalAppData.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+729
to
+757
| [Fact] | ||
| public async Task ExecuteAsync_ExpertOverride_PanicDisableActive_NoOverride() | ||
| { | ||
| var previousOverride = Environment.GetEnvironmentVariable("SWFOC_EXPERT_MUTATION_OVERRIDES"); | ||
| var previousPanic = Environment.GetEnvironmentVariable("SWFOC_EXPERT_MUTATION_OVERRIDES_PANIC"); | ||
| try | ||
| { | ||
| Environment.SetEnvironmentVariable("SWFOC_EXPERT_MUTATION_OVERRIDES", "1"); | ||
| Environment.SetEnvironmentVariable("SWFOC_EXPERT_MUTATION_OVERRIDES_PANIC", "1"); | ||
| var profile = BuildProfile("set_unit_cap"); | ||
| var adapter = CreateAttachedAdapter( | ||
| profile: profile, | ||
| router: new StubBackendRouter( | ||
| new BackendRouteDecision(false, ExecutionBackendKind.Extender, RuntimeReasonCode.CAPABILITY_REQUIRED_MISSING, "blocked"))); | ||
|
|
||
| var result = await adapter.ExecuteAsync( | ||
| BuildRequest("set_unit_cap", RuntimeMode.Galactic), | ||
| CancellationToken.None); | ||
|
|
||
| // Panic disable overrides the expert override — should be blocked | ||
| result.Succeeded.Should().BeFalse(); | ||
| result.Diagnostics.Should().ContainKey("panicDisableState"); | ||
| result.Diagnostics!["panicDisableState"]!.ToString().Should().Be("active"); | ||
| } | ||
| finally | ||
| { | ||
| Environment.SetEnvironmentVariable("SWFOC_EXPERT_MUTATION_OVERRIDES", previousOverride); | ||
| Environment.SetEnvironmentVariable("SWFOC_EXPERT_MUTATION_OVERRIDES_PANIC", previousPanic); | ||
| } |
There was a problem hiding this comment.
4. Env var tests interfere 🐞 Bug ☼ Reliability
RuntimeAdapterAsyncWave7Tests mutates process-global environment variables SWFOC_EXPERT_MUTATION_OVERRIDES and SWFOC_EXPERT_MUTATION_OVERRIDES_PANIC, which are also mutated/read by other RuntimeAdapter test classes. Concurrent execution can observe transient values and fail nondeterministically.
Agent Prompt
### Issue description
Multiple test classes set `SWFOC_EXPERT_MUTATION_OVERRIDES*` environment variables. Because environment variables are process-global, concurrent tests can read the wrong value mid-test, causing flakes.
### Issue Context
`RuntimeAdapter.ResolveExpertMutationOverrideState()` reads the env vars to control expert override behavior. Both `RuntimeAdapterAsyncWave7Tests` and existing `RuntimeAdapterWave6Tests` set these env vars.
### Fix Focus Areas
- tests/SwfocTrainer.Tests/Runtime/RuntimeAdapterAsyncWave7Tests.cs[705-860]
- tests/SwfocTrainer.Tests/Runtime/RuntimeAdapterWave6Tests.cs[1294-1357]
- src/SwfocTrainer.Runtime/Services/RuntimeAdapter.cs[2055-2086]
### Suggested fix
1. Put all tests that set/read these env vars into the same xUnit collection with parallelization disabled.
2. Optionally add a shared static lock helper (e.g., `EnvironmentVariableLock`) and wrap every env-var mutation/read-sensitive test section in that lock to enforce mutual exclusion.
3. Keep the existing try/finally restoration, but ensure it’s used consistently across all tests that touch these env vars.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
7 issues found across 8 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="tests/SwfocTrainer.Tests/Runtime/RuntimeAdapterAsyncWave7Tests.cs">
<violation number="1" location="tests/SwfocTrainer.Tests/Runtime/RuntimeAdapterAsyncWave7Tests.cs:978">
P2: These exception tests don't await `ThrowAsync`, so the assertions can be skipped and produce false-positive passing tests.</violation>
</file>
<file name="tests/SwfocTrainer.Tests/DataIndex/DataIndexWave7FinalTests.cs">
<violation number="1" location="tests/SwfocTrainer.Tests/DataIndex/DataIndexWave7FinalTests.cs:42">
P2: This test name claims mod loose entries are skipped, but it only asserts `report` is non-null, so regressions in whitespace MODPATH handling would still pass unnoticed.</violation>
<violation number="2" location="tests/SwfocTrainer.Tests/DataIndex/DataIndexWave7FinalTests.cs:64">
P2: This test does not verify `MegaFiles.xml` diagnostics propagation: the XML input is valid, and the assertion matches MEG file-not-found diagnostics instead of checking propagated `MegaFiles.xml:` diagnostics.</violation>
</file>
<file name="tests/SwfocTrainer.Tests/Profiles/ProfilesWave7FinalTests.cs">
<violation number="1" location="tests/SwfocTrainer.Tests/Profiles/ProfilesWave7FinalTests.cs:29">
P2: This test claims to verify empty-entry handling but only exercises a normal `valid.txt` extraction path, so it does not validate the intended branch.</violation>
<violation number="2" location="tests/SwfocTrainer.Tests/Profiles/ProfilesWave7FinalTests.cs:173">
P2: This test is labeled as integration coverage for exception catch paths, but it never invokes `ModOnboardingService`, so the targeted catch blocks are not tested.</violation>
</file>
<file name="tests/SwfocTrainer.Tests/Saves/SavesWave7FinalTests.cs">
<violation number="1" location="tests/SwfocTrainer.Tests/Saves/SavesWave7FinalTests.cs:431">
P3: Avoid creating a new unmanaged schema temp directory per call; this helper currently leaks temp folders across test runs.</violation>
</file>
<file name="tests/SwfocTrainer.Tests/App/AppWave7FinalTests.cs">
<violation number="1" location="tests/SwfocTrainer.Tests/App/AppWave7FinalTests.cs:80">
P2: These tests hit the real app-data settings file without any skip/guard. Per tests/AGENTS.md, live tests must skip with an explicit reason when prerequisites (writable settings path) are absent. Add an explicit skip/guard or isolate the settings path so this doesn’t fail or mutate developer settings on CI/local runs.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.
| } | ||
|
|
||
| [Fact] | ||
| public void ExecuteAsync_NullRequest_Throws() |
There was a problem hiding this comment.
P2: These exception tests don't await ThrowAsync, so the assertions can be skipped and produce false-positive passing tests.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/SwfocTrainer.Tests/Runtime/RuntimeAdapterAsyncWave7Tests.cs, line 978:
<comment>These exception tests don't await `ThrowAsync`, so the assertions can be skipped and produce false-positive passing tests.</comment>
<file context>
@@ -0,0 +1,1868 @@
+ }
+
+ [Fact]
+ public void ExecuteAsync_NullRequest_Throws()
+ {
+ var adapter = CreateAttachedAdapter();
</file context>
| } | ||
|
|
||
| [Fact] | ||
| public void Build_WithWhitespaceModPath_ShouldSkipModLooseEntries() |
There was a problem hiding this comment.
P2: This test name claims mod loose entries are skipped, but it only asserts report is non-null, so regressions in whitespace MODPATH handling would still pass unnoticed.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/SwfocTrainer.Tests/DataIndex/DataIndexWave7FinalTests.cs, line 42:
<comment>This test name claims mod loose entries are skipped, but it only asserts `report` is non-null, so regressions in whitespace MODPATH handling would still pass unnoticed.</comment>
<file context>
@@ -0,0 +1,140 @@
+ }
+
+ [Fact]
+ public void Build_WithWhitespaceModPath_ShouldSkipModLooseEntries()
+ {
+ var tempDir = Path.Combine(Path.GetTempPath(), "di-w7-ws-" + Guid.NewGuid().ToString("N"));
</file context>
| #region MegaFiles.xml diagnostics propagation (lines 99-102) | ||
|
|
||
| [Fact] | ||
| public void Build_WhenMegaFilesXmlHasDiagnostics_ShouldPropagate() |
There was a problem hiding this comment.
P2: This test does not verify MegaFiles.xml diagnostics propagation: the XML input is valid, and the assertion matches MEG file-not-found diagnostics instead of checking propagated MegaFiles.xml: diagnostics.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/SwfocTrainer.Tests/DataIndex/DataIndexWave7FinalTests.cs, line 64:
<comment>This test does not verify `MegaFiles.xml` diagnostics propagation: the XML input is valid, and the assertion matches MEG file-not-found diagnostics instead of checking propagated `MegaFiles.xml:` diagnostics.</comment>
<file context>
@@ -0,0 +1,140 @@
+ #region MegaFiles.xml diagnostics propagation (lines 99-102)
+
+ [Fact]
+ public void Build_WhenMegaFilesXmlHasDiagnostics_ShouldPropagate()
+ {
+ var tempDir = Path.Combine(Path.GetTempPath(), "di-w7-diag-" + Guid.NewGuid().ToString("N"));
</file context>
| #region GitHubProfileUpdateExtractionHelpers (lines 24-25, 70-71) | ||
|
|
||
| [Fact] | ||
| public void ExtractToDirectorySafely_EntryWithEmptyName_ShouldSkipEntry() |
There was a problem hiding this comment.
P2: This test claims to verify empty-entry handling but only exercises a normal valid.txt extraction path, so it does not validate the intended branch.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/SwfocTrainer.Tests/Profiles/ProfilesWave7FinalTests.cs, line 29:
<comment>This test claims to verify empty-entry handling but only exercises a normal `valid.txt` extraction path, so it does not validate the intended branch.</comment>
<file context>
@@ -0,0 +1,226 @@
+ #region GitHubProfileUpdateExtractionHelpers (lines 24-25, 70-71)
+
+ [Fact]
+ public void ExtractToDirectorySafely_EntryWithEmptyName_ShouldSkipEntry()
+ {
+ var tempDir = Path.Combine(Path.GetTempPath(), "prof-w7-" + Guid.NewGuid().ToString("N"));
</file context>
| #region ModOnboardingService — exception catch blocks (lines 212-227) | ||
|
|
||
| [Fact] | ||
| public void ModOnboardingService_ExceptionCatchPaths_AreTestedViaIntegration() |
There was a problem hiding this comment.
P2: This test is labeled as integration coverage for exception catch paths, but it never invokes ModOnboardingService, so the targeted catch blocks are not tested.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/SwfocTrainer.Tests/Profiles/ProfilesWave7FinalTests.cs, line 173:
<comment>This test is labeled as integration coverage for exception catch paths, but it never invokes `ModOnboardingService`, so the targeted catch blocks are not tested.</comment>
<file context>
@@ -0,0 +1,226 @@
+ #region ModOnboardingService — exception catch blocks (lines 212-227)
+
+ [Fact]
+ public void ModOnboardingService_ExceptionCatchPaths_AreTestedViaIntegration()
+ {
+ // The catch blocks at lines 212-227 handle IOException, InvalidOperationException,
</file context>
| // Temporarily override the settings path to a nonexistent location | ||
| // Since GetSettingsPath is private, we test via the public Load method | ||
| // which will use the actual app data path | ||
| var result = MainViewModelRuntimeModeOverrideHelpers.Load(); |
There was a problem hiding this comment.
P2: These tests hit the real app-data settings file without any skip/guard. Per tests/AGENTS.md, live tests must skip with an explicit reason when prerequisites (writable settings path) are absent. Add an explicit skip/guard or isolate the settings path so this doesn’t fail or mutate developer settings on CI/local runs.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/SwfocTrainer.Tests/App/AppWave7FinalTests.cs, line 80:
<comment>These tests hit the real app-data settings file without any skip/guard. Per tests/AGENTS.md, live tests must skip with an explicit reason when prerequisites (writable settings path) are absent. Add an explicit skip/guard or isolate the settings path so this doesn’t fail or mutate developer settings on CI/local runs.</comment>
<file context>
@@ -0,0 +1,142 @@
+ // Temporarily override the settings path to a nonexistent location
+ // Since GetSettingsPath is private, we test via the public Load method
+ // which will use the actual app data path
+ var result = MainViewModelRuntimeModeOverrideHelpers.Load();
+ // Should return a valid string (either "auto" or whatever is currently saved)
+ result.Should().NotBeNullOrWhiteSpace();
</file context>
|
|
||
| private static SavePatchPackService CreatePackService() | ||
| { | ||
| var tempDir = Path.Combine(Path.GetTempPath(), "saves-w7-schemas-" + Guid.NewGuid().ToString("N")); |
There was a problem hiding this comment.
P3: Avoid creating a new unmanaged schema temp directory per call; this helper currently leaks temp folders across test runs.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/SwfocTrainer.Tests/Saves/SavesWave7FinalTests.cs, line 431:
<comment>Avoid creating a new unmanaged schema temp directory per call; this helper currently leaks temp folders across test runs.</comment>
<file context>
@@ -0,0 +1,451 @@
+
+ private static SavePatchPackService CreatePackService()
+ {
+ var tempDir = Path.Combine(Path.GetTempPath(), "saves-w7-schemas-" + Guid.NewGuid().ToString("N"));
+ Directory.CreateDirectory(tempDir);
+ return new SavePatchPackService(new SaveOptions { SchemaRootPath = tempDir });
</file context>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3ccc21be58
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| method.Should().NotBeNull(); | ||
|
|
||
| // Pass a nonexistent zip path to trigger IOException from ZipFile.OpenRead | ||
| var result = method!.Invoke(null, new object[] { "testProfile", "/nonexistent/file.zip", "/tmp/extract" }); |
There was a problem hiding this comment.
This test hardcodes the extract destination as "/tmp/extract", but TryExtractPackage creates the extraction directory before opening the zip. That means the test writes outside its own temp sandbox and can fail with UnauthorizedAccessException on environments that cannot write to a root-level path (especially Windows drive roots), producing a failure unrelated to the branch being tested. Using a path under tempDir would keep the test deterministic and self-cleaning.
Useful? React with 👍 / 👎.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Wave 7: 200+ tests. Branch 80.8%, Line 81.8%.
Summary by cubic
Adds the final Wave 7 test suite (200+ tests) targeting async handlers and edge cases across Runtime, Saves, Meg, DataIndex, Flow, Profiles, Core, and App. Raises coverage to 80.8% branch and 81.8% line.
ExecuteByRouteAsync, backend handlers), attach/detach lifecycle, calibration scan, and exception branches.BinarySaveCodecwrite guards and float endianness;SavePatchApplyServiceapply/restore/rollback IO and permissions paths.MegArchiveReaderformat fallbacks and range/parse errors; loose entry/path resolution guards; flow exporter switch arms and harness defaults.Written for commit 3ccc21b. Summary will update on new commits.
Summary by CodeRabbit