fix: address Copilot review comments on PR #456/#458

- Remove empty try/finally block in ClientUpdateStrategy download path
- Add thread-safety note for shared custom executor in DefaultDownloadOrchestrator
- Add ResolveExtensionType<T>() to avoid wasteful instantiation for pipeline factory
- Restore ClientUpdateStrategy(IDownloadOrchestrator?) constructor for backward compat

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

Author: JusterZhu

src/c#/GeneralUpdate.Core/Bootstrap/GeneralUpdateBootstrap.cs

@@ -94,35 +94,26 @@ private async Task<GeneralUpdateBootstrap> LaunchWithStrategy(IStrategy roleStra
             token.ThrowIfCancellationRequested();
             ApplyRuntimeOptions();
 
-            // Resolve hooks and reporter from extensions
-            var hooks = ResolveExtension<Hooks.IUpdateHooks>() ?? new Hooks.NoOpUpdateHooks();
-            var reporter = ResolveExtension<Download.Reporting.IUpdateReporter>() ??
-                           new Download.Reporting.NoOpUpdateReporter();
-
             // ── Network-level extensions (global, applied before any HTTP call) ──
             var sslPolicy = ResolveExtension<Security.ISslValidationPolicy>();
             if (sslPolicy != null) Network.VersionService.SetSslValidationPolicy(sslPolicy);
             var authProvider = ResolveExtension<Security.IHttpAuthProvider>();
             if (authProvider != null) Network.VersionService.SetDefaultAuthProvider(authProvider);
 
-            // ── Phase 1: inject all dependencies before Create ──
+            // Resolve hooks from extensions
+            var hooks = ResolveExtension<Hooks.IUpdateHooks>() ?? new Hooks.NoOpUpdateHooks();
             roleStrategy.Hooks = hooks;
-            roleStrategy.Reporter = reporter;
 
-            var binaryDiffer = ResolveExtension<IBinaryDiffer>();
-            var dirtyStrategy = ResolveExtension<IDirtyStrategy>();
+            // ── Download components ──
             var downloadOrchestrator = ResolveExtension<Download.Abstractions.IDownloadOrchestrator>();
             var downloadPolicy = ResolveExtension<Download.Abstractions.IDownloadPolicy>();
             var downloadExecutor = ResolveExtension<Download.Abstractions.IDownloadExecutor>();
 
-            // Build download pipeline factory from registered extension type.
-            // Tries a string constructor first (for passing the expected hash, à la DefaultDownloadPipeline),
-            // falls back to parameterless constructor otherwise.
+            // Build download pipeline factory from registered extension type
             Func<string?, Download.Abstractions.IDownloadPipeline>? downloadPipelineFactory = null;
-            var downloadPipeline = ResolveExtension<Download.Abstractions.IDownloadPipeline>();
-            if (downloadPipeline != null)
+            var pipelineType = ResolveExtensionType<Download.Abstractions.IDownloadPipeline>();
+            if (pipelineType != null)
             {
-                var pipelineType = downloadPipeline.GetType();
                 var stringCtor = pipelineType.GetConstructor([typeof(string)]);
                 if (stringCtor != null)
                     downloadPipelineFactory = hash => (Download.Abstractions.IDownloadPipeline)stringCtor.Invoke([hash]);
@@ -142,8 +133,6 @@ private async Task<GeneralUpdateBootstrap> LaunchWithStrategy(IStrategy roleStra
 
                     await CallSmallBowlHomeAsync(_configInfo.Bowl).ConfigureAwait(false);
 
-                    if (binaryDiffer != null) cs.SetBinaryDiffer(binaryDiffer);
-                    if (dirtyStrategy != null) cs.SetDirtyStrategy(dirtyStrategy);
                     cs.SetDiffPipeline(diffPipeline);
                     if (downloadOrchestrator != null) cs.SetOrchestrator(downloadOrchestrator);
                     if (downloadPolicy != null) cs.SetDownloadPolicy(downloadPolicy);
@@ -152,10 +141,6 @@ private async Task<GeneralUpdateBootstrap> LaunchWithStrategy(IStrategy roleStra
                     break;
 
                 case UpgradeUpdateStrategy us:
-                    if (binaryDiffer != null)
-                        us.SetBinaryDiffer(binaryDiffer);
-                    if (dirtyStrategy != null)
-                        us.SetDirtyStrategy(dirtyStrategy);
                     us.SetDiffPipeline(diffPipeline);
                     break;
             }

src/c#/GeneralUpdate.Core/Configuration/AbstractBootstrap.cs

@@ -129,36 +129,6 @@ protected T GetOption<T>(UpdateOption<T>? option)
             return (TBootstrap)this;
         }
 
-        public TBootstrap DirtyStrategy<T>() where T : Differential.IDirtyStrategy, new()
-        {
-            _extensions[typeof(Differential.IDirtyStrategy)] = typeof(T);
-            return (TBootstrap)this;
-        }
-
-        public TBootstrap BinaryDiffer<T>() where T : IBinaryDiffer, new()
-        {
-            _extensions[typeof(IBinaryDiffer)] = typeof(T);
-            return (TBootstrap)this;
-        }
-
-        public TBootstrap ConfigureBlackList(BlackListConfig config)
-        {
-            _instances[typeof(BlackListConfig)] = config ?? BlackListConfig.Empty;
-            return (TBootstrap)this;
-        }
-
-        /// <summary>
-        /// Configure blacklist via fluent builder action.
-        /// Usage: <c>.ConfigureBlackList(cfg => cfg.AddBlackFiles("*.log").AddBlackFormats(".pdb"))</c>
-        /// </summary>
-        public TBootstrap ConfigureBlackList(Action<FileSystem.BlackListConfigBuilder> configure)
-        {
-            var builder = new FileSystem.BlackListConfigBuilder();
-            configure(builder);
-            _instances[typeof(BlackListConfig)] = builder.Build();
-            return (TBootstrap)this;
-        }
-
         protected TExtension? ResolveExtension<TExtension>() where TExtension : class
         {
             if (_extensions.TryGetValue(typeof(TExtension), out var t))
@@ -167,5 +137,11 @@ public TBootstrap ConfigureBlackList(Action<FileSystem.BlackListConfigBuilder> c
                 return instance as TExtension;
             return null;
         }
+
+        /// <summary>Resolves the registered extension type without instantiating it.</summary>
+        protected Type? ResolveExtensionType<TExtension>() where TExtension : class
+        {
+            return _extensions.TryGetValue(typeof(TExtension), out var t) ? t : null;
+        }
     }
 }

src/c#/GeneralUpdate.Core/Download/Orchestrators/DefaultDownloadOrchestrator.cs

@@ -30,6 +30,9 @@ public class DefaultDownloadOrchestrator : IDownloadOrchestrator
     private readonly DownloadOrchestratorOptions _options;
     private readonly IDownloadExecutor? _customExecutor;
     private readonly Func<string?, IDownloadPipeline>? _pipelineFactory;
+    // Note: _customExecutor is a single shared instance used across parallel asset downloads.
+    // Implementations of IDownloadExecutor must be thread-safe. HttpClient-based executors
+    // satisfy this as HttpClient is designed for concurrent use.
 
     public DefaultDownloadOrchestrator(
         HttpClient httpClient,

src/c#/GeneralUpdate.Core/Strategy/ClientUpdateStrategy.cs

@@ -62,6 +62,9 @@ private enum UpdateScenario
 
     public ClientUpdateStrategy() { }
 
+    public ClientUpdateStrategy(Download.Abstractions.IDownloadOrchestrator? orchestrator)
+        => _orchestrator = orchestrator;
+
     /// <summary>Sets a custom OS-level strategy (injected via <c>.Strategy&lt;T&gt;()</c>).
     /// When set, this replaces the automatic platform detection in <see cref="ResolveOsStrategy"/>.</summary>
     public void SetOsStrategy(IStrategy? strategy) => _customOsStrategy = strategy;
@@ -84,8 +87,6 @@ public void Create(GlobalConfigInfo parameter)
         _osStrategy = ResolveOsStrategy();
         if (_osStrategy is AbstractStrategy abs)
         {
-            if (_pendingDirtyStrategy != null) abs.DirtyStrategy = _pendingDirtyStrategy;
-            if (_pendingBinaryDiffer != null) abs.BinaryDiffer = _pendingBinaryDiffer;
             if (_pendingDiffPipeline != null) abs.DiffPipeline = _pendingDiffPipeline;
         }
     }
@@ -110,29 +111,8 @@ public async Task ExecuteAsync()
         }
     }
 
-    private IDirtyStrategy? _pendingDirtyStrategy;
-    private IBinaryDiffer? _pendingBinaryDiffer;
     private DiffPipeline? _pendingDiffPipeline;
 
-    /// <summary>Sets the directory-level dirty strategy on the underlying OS-level strategy for differential patch updates.
-    /// Safe to call before or after Create(). If called before, the strategy is cached and applied when Create() resolves _osStrategy.</summary>
-    public void SetDirtyStrategy(IDirtyStrategy? dirtyStrategy)
-    {
-        if (_osStrategy is AbstractStrategy abs)
-            abs.DirtyStrategy = dirtyStrategy;
-        else
-            _pendingDirtyStrategy = dirtyStrategy;
-    }
-
-    /// <summary>Sets the file-level binary differ on the underlying OS-level strategy.</summary>
-    public void SetBinaryDiffer(IBinaryDiffer? binaryDiffer)
-    {
-        if (_osStrategy is AbstractStrategy abs)
-            abs.BinaryDiffer = binaryDiffer;
-        else
-            _pendingBinaryDiffer = binaryDiffer;
-    }
-
     /// <summary>Sets the DiffPipeline on the underlying OS-level strategy for parallel patch application.</summary>
     public void SetDiffPipeline(DiffPipeline? diffPipeline)
     {
@@ -292,16 +272,10 @@ private async Task ExecuteStandardWorkflowAsync()
         else
         {
             var httpClient = GeneralUpdate.Core.Network.HttpClientProvider.Shared;
-            try
-            {
-                var orchestrator = new Download.Orchestrators.DefaultDownloadOrchestrator(
-                    httpClient, orchOptions, _customDownloadPolicy,
-                    _customDownloadExecutor, _customDownloadPipelineFactory);
-                await orchestrator.ExecuteAsync(downloadPlan, _configInfo.TempPath).ConfigureAwait(false);
-            }
-            finally
-            {
-            }
+            var orchestrator = new Download.Orchestrators.DefaultDownloadOrchestrator(
+                httpClient, orchOptions, _customDownloadPolicy,
+                _customDownloadExecutor, _customDownloadPipelineFactory);
+            await orchestrator.ExecuteAsync(downloadPlan, _configInfo.TempPath).ConfigureAwait(false);
         }
 
         await SafeReportDownloadCompletedAsync(hooksCtx).ConfigureAwait(false);

src/c#/GeneralUpdate.Core/Strategy/UpgradeUpdateStrategy.cs

@@ -45,8 +45,6 @@ public void Create(GlobalConfigInfo parameter)
         _osStrategy = ResolveOsStrategy();
         if (_osStrategy is AbstractStrategy abs)
         {
-            if (_pendingDirtyStrategy != null) abs.DirtyStrategy = _pendingDirtyStrategy;
-            if (_pendingBinaryDiffer != null) abs.BinaryDiffer = _pendingBinaryDiffer;
             if (_pendingDiffPipeline != null) abs.DiffPipeline = _pendingDiffPipeline;
         }
     }
@@ -116,29 +114,8 @@ public async Task ExecuteAsync()
         }
     }
 
-    private IDirtyStrategy? _pendingDirtyStrategy;
-    private IBinaryDiffer? _pendingBinaryDiffer;
     private DiffPipeline? _pendingDiffPipeline;
 
-    /// <summary>Sets the directory-level dirty strategy on the underlying OS-level strategy for differential patch updates.
-    /// Safe to call before or after Create(). If called before, the strategy is cached and applied when Create() resolves _osStrategy.</summary>
-    public void SetDirtyStrategy(IDirtyStrategy? dirtyStrategy)
-    {
-        if (_osStrategy is AbstractStrategy abs)
-            abs.DirtyStrategy = dirtyStrategy;
-        else
-            _pendingDirtyStrategy = dirtyStrategy;
-    }
-
-    /// <summary>Sets the file-level binary differ on the underlying OS-level strategy.</summary>
-    public void SetBinaryDiffer(IBinaryDiffer? binaryDiffer)
-    {
-        if (_osStrategy is AbstractStrategy abs)
-            abs.BinaryDiffer = binaryDiffer;
-        else
-            _pendingBinaryDiffer = binaryDiffer;
-    }
-
     /// <summary>Sets the DiffPipeline on the underlying OS-level strategy for parallel patch application.</summary>
     public void SetDiffPipeline(DiffPipeline? diffPipeline)
     {