fix: use fallback defaults instead of hardcoded overrides for runtime options#401
Merged
Merged
Conversation
… options Closes #400 - Remove dead 'if (true)' branch in ClientUpdateStrategy.ExecuteWorkflowAsync() (silent routing now handled by GeneralUpdateBootstrap.LaunchSilentAsync()) - Replace unconditional '= GetOption(...)' with '??=' or conditional assignment in Bootstrap.ApplyRuntimeOptions() to avoid overwriting InitializeFromEnvironment() values in the Upgrade path - Replace '= Encoding.UTF8' / '= Format.ZIP' / '= 60' with '??=' fallback pattern in ClientUpdateStrategy, UpgradeUpdateStrategy, OSSUpdateStrategy - Fix hardcoded 'Format = "ZIP"' in ClientUpdateStrategy ProcessInfo builder - Add 15 hook/extension injection tests to BootstrapFullParameterMatrixTests covering all 14 extension methods + chained injection
Bootstrap.ApplyRuntimeOptions() is the single source of truth for reading UpdateOptions and applying defaults. Strategy classes should not duplicate this logic. - Remove ApplyStrategyDefaults() from ClientUpdateStrategy - Remove ApplyRuntimeOptions() from UpgradeUpdateStrategy - Inline OSSUpdateStrategy timeout fallback
Contributor
There was a problem hiding this comment.
Pull request overview
This PR aims to stop silently overwriting user-specified UpdateOptions/environment-populated runtime settings by switching from unconditional assignments to fallback-default behavior in bootstrapping and strategy runtime option handling, while also removing dead code and expanding bootstrap extension-injection test coverage.
Changes:
- Update bootstrap/strategies to apply runtime options via null-coalescing / conditional assignment rather than unconditional overrides.
- Simplify
ClientUpdateStrategyworkflow by removing dead branching and centralizing “fallback defaults” inApplyStrategyDefaults(). - Add tests covering all extension injection methods in the bootstrap parameter matrix.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
src/c#/GeneralUpdate.Core/Bootstrap/GeneralUpdateBootstrap.cs |
Changes ApplyRuntimeOptions() to use fallback-default patterns instead of unconditional overrides. |
src/c#/GeneralUpdate.Core/Strategy/ClientUpdateStrategy.cs |
Removes dead workflow branch and adds strategy-level fallback defaults; adjusts ProcessInfo format mapping. |
src/c#/GeneralUpdate.Core/Strategy/UpgradeUpdateStrategy.cs |
Switches runtime option defaults to null-coalescing assignments. |
src/c#/GeneralUpdate.Core/Strategy/OSSUpdateStrategy.cs |
Uses config-driven timeout with a default fallback instead of a hardcoded constant. |
tests/CoreTest/Bootstrap/BootstrapFullParameterMatrixTests.cs |
Adds coverage for bootstrap extension injection methods and chaining. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+302
to
+309
| // Download behaviour — preserve existing values (Upgrade path or property initializers) | ||
| // and only apply UpdateOptions when the current value is at its unset default | ||
| if (_configInfo.MaxConcurrency <= 0) | ||
| _configInfo.MaxConcurrency = GetOption(UpdateOptions.MaxConcurrency); | ||
| if (_configInfo.RetryCount <= 0) | ||
| _configInfo.RetryCount = GetOption(UpdateOptions.RetryCount); | ||
| if (_configInfo.RetryInterval <= TimeSpan.Zero) | ||
| _configInfo.RetryInterval = GetOption(UpdateOptions.RetryInterval); |
Comment on lines
+290
to
+295
| // Core runtime — use ??= so Upgrade path (InitializeFromEnvironment) values | ||
| // are not overwritten by GetOption defaults | ||
| _configInfo.Encoding ??= GetOption(UpdateOptions.Encoding); | ||
| _configInfo.Format ??= GetOption(UpdateOptions.Format); | ||
| if (_configInfo.DownloadTimeOut <= 0) | ||
| _configInfo.DownloadTimeOut = GetOption(UpdateOptions.DownloadTimeout) ?? 60; |
| private void ApplyStrategyDefaults() | ||
| { | ||
| _configInfo!.Encoding ??= Encoding.UTF8; | ||
| _configInfo.Format ??= "ZIP"; |
Comment on lines
180
to
187
| var downloadVersions = downloadPlan.Assets.Select(a => new VersionInfo | ||
| { | ||
| Name = a.Name, | ||
| Hash = a.SHA256, | ||
| Url = a.Url, | ||
| Version = a.Version, | ||
| Format = "ZIP" | ||
| Format = _configInfo.Format ?? "ZIP" | ||
| }).ToList(); |
Comment on lines
120
to
122
| _configInfo.Format ??= Format.ZIP; | ||
| } | ||
|
|
| private const int DefaultTimeOut = 60; | ||
|
|
||
| private int ResolveTimeout() | ||
| => _configInfo.DownloadTimeOut > 0 ? _configInfo.DownloadTimeOut : DefaultTimeOut; |
Remove the broken '<= 0' guards on MaxConcurrency/RetryCount/RetryInterval. These guards checked C# type defaults (0) but GlobalConfigInfo property initializers already set functionally reasonable values (3, 3, 1s), so the guards would never trigger in the Client path — user-configured values via .Option(MaxConcurrency, 8) were silently ignored. Only Encoding/Format/DownloadTimeOut need ??= protection — they are the only options that InitializeFromEnvironment() may populate on the Upgrade path before ApplyRuntimeOptions() runs.
UpdateOptions.Format default value is "ZIP", but pipeline constructs zip
file paths as `{name}{format}` and CompressProvider.Decompress switches
on Format.ZIP (".zip"). If Format stays "ZIP", paths become "MyAppZIP"
and decompression fails.
Previously, ClientUpdateStrategy.ApplyRuntimeOptions(encoding, timeout)
silently overwrote this with Format.ZIP (".zip"). After removing that
redundant override in PR #401, the bug becomes visible.
Normalize to Format.ZIP in Bootstrap.ApplyRuntimeOptions() — the single
source of truth for runtime options.
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
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.
Summary
Fixes unconditional hardcoded overrides in strategy and bootstrap methods that silently overwrite user-configured UpdateOptions values with defaults. Also removes dead code and adds missing extension injection tests.
Closes #400
Changes
Source files (5)
GeneralUpdateBootstrap.csApplyRuntimeOptions(): replace= GetOption(...)with??=or conditional assignment to preserve Upgrade path values fromInitializeFromEnvironment()ClientUpdateStrategy.csif (true)branch; delete redundantApplyRuntimeOptions(encoding, timeout)override; addApplyStrategyDefaults()with??=fallback; fixFormat = "ZIP"→Format = _configInfo.Format ?? "ZIP"UpgradeUpdateStrategy.csEncoding = UTF8→Encoding ??= UTF8;Format = ZIP→Format ??= ZIPOSSUpdateStrategy.csconst int TimeOut = 60→ResolveTimeout()reads_configInfo.DownloadTimeOutwith fallbackBootstrapFullParameterMatrixTests.csTest results
Key pattern change
This is critical for the Upgrade path where
InitializeFromEnvironment()already sets Encoding/Format/DownloadTimeOut from the client's ProcessInfo.