fix: address copilot review cleanup (#491)

Synchronizes ObjectPool rent/return/dispose paths to avoid post-disposal retention, adds a regression scenario, and cleans up older Copilot review nits around culture-sensitive facade assertions, scenario titles, and Composer/Proxy async docs.

Author: JerrettDavis

.github/workflows/ci.yml

@@ -99,9 +99,9 @@ jobs:
       - name: Upload coverage to Codecov
         uses: codecov/codecov-action@v6
         with:
-          files: |
-            **/TestResults/*/coverage.cobertura.xml
+          files: ./coverage-report/Cobertura.xml
           flags: unittests
+          disable_search: true
           fail_ci_if_error: true
           verbose: true
         env:

.github/workflows/pr-validation.yml

@@ -95,17 +95,6 @@ jobs:
             TestResults/**/*.trx
           check_name: Test Results
 
-      - name: Upload coverage to Codecov
-        uses: codecov/codecov-action@v6
-        if: always()
-        with:
-          files: ./TestResults/**/coverage.opencover.xml
-          flags: unittests
-          name: pr-${{ github.event.pull_request.number }}
-          fail_ci_if_error: false
-        env:
-          CODECOV_TOKEN: ${{ secrets.CODECOV_TOKEN }}
-
       - name: Build Documentation
         run: |
           dotnet tool update -g docfx

docs/generators/composer.md

@@ -90,7 +90,7 @@ ValueTask<TOut> StepNameAsync(TIn input, Func<TIn, ValueTask<TOut>> next, Cancel
 
 - `input`: The pipeline input (not `in` for async)
 - `next`: Async delegate to call the next step
-- `ct`: CancellationToken for cooperative cancellation
+- `ct`: Optional CancellationToken for cooperative cancellation. Async steps may omit it when they do not need cancellation.
 
 ## Terminal Method Signature
 
@@ -106,6 +106,8 @@ TOut TerminalName(in TIn input)
 ValueTask<TOut> TerminalNameAsync(TIn input, CancellationToken ct)
 ```
 
+Async terminals may omit the `CancellationToken` parameter when they do not need cancellation.
+
 ## Attributes
 
 ### `[Composer]`
@@ -116,7 +118,7 @@ Main attribute for marking pipeline host types.
 |---|---|---|---|
 | `InvokeMethodName` | `string` | `"Invoke"` | Name of generated sync method |
 | `InvokeAsyncMethodName` | `string` | `"InvokeAsync"` | Name of generated async method |
-| `GenerateAsync` | `bool` | Inferred when omitted | Explicit async control |
+| `GenerateAsync` | `bool` | Inferred by generator when the named property is omitted | Explicit async control |
 | `ForceAsync` | `bool` | `false` | Force async generation even if all steps are sync |
 | `WrapOrder` | `ComposerWrapOrder` | `OuterFirst` | Determines wrapping order |
 
@@ -206,7 +208,7 @@ public ValueTask<string> InvokeAsync(string input, CancellationToken ct = defaul
 | **PKCOM006** | Error | Invalid step method signature |
 | **PKCOM007** | Error | Invalid terminal method signature |
 | **PKCOM008** | Error | Async step detected but async generation disabled |
-| **PKCOM009** | Warning | Async method missing CancellationToken parameter |
+| **PKCOM009** | Warning | Async method declares a final extra parameter with the wrong type |
 
 ## Best Practices
 

src/PatternKit.Core/Creational/ObjectPool/ObjectPool.cs

@@ -8,6 +8,7 @@ namespace PatternKit.Creational.ObjectPool;
 public sealed class ObjectPool<T> : IDisposable
 {
     private readonly ConcurrentQueue<T> _items = new();
+    private readonly object _gate = new();
     private readonly Func<T> _factory;
     private readonly Action<T>? _onRent;
     private readonly Action<T>? _onReturn;
@@ -34,17 +35,38 @@ private ObjectPool(Func<T> factory, Action<T>? onRent, Action<T>? onReturn, Func
     /// <summary>Rents an item from the pool. Dispose the returned lease to return the item.</summary>
     public ObjectPoolLease<T> Rent()
     {
-        ThrowIfDisposed();
-
         T value;
-        if (_items.TryDequeue(out var pooled))
+        var hasValue = false;
+        lock (_gate)
         {
-            Interlocked.Decrement(ref _retained);
-            value = pooled;
+            ThrowIfDisposed();
+
+            if (_items.TryDequeue(out var pooled))
+            {
+                Interlocked.Decrement(ref _retained);
+                value = pooled;
+                hasValue = true;
+            }
+            else
+            {
+                value = default!;
+            }
         }
-        else
-        {
+
+        if (!hasValue)
             value = _factory();
+
+        var disposeAfterRent = false;
+        lock (_gate)
+        {
+            if (_disposed)
+                disposeAfterRent = true;
+        }
+
+        if (disposeAfterRent)
+        {
+            DisposeIfNeeded(value);
+            throw new ObjectDisposedException(nameof(ObjectPool<T>));
         }
 
         _onRent?.Invoke(value);
@@ -53,44 +75,82 @@ public ObjectPoolLease<T> Rent()
 
     internal void Return(T value)
     {
-        if (_disposed)
+        var disposeImmediately = false;
+        lock (_gate)
+        {
+            disposeImmediately = _disposed;
+        }
+
+        if (disposeImmediately)
         {
             DisposeIfNeeded(value);
             return;
         }
 
-        _onReturn?.Invoke(value);
-        if (_shouldReturn is not null && !_shouldReturn(value))
+        try
+        {
+            _onReturn?.Invoke(value);
+            if (_shouldReturn is not null && !_shouldReturn(value))
+            {
+                DisposeIfNeeded(value);
+                return;
+            }
+        }
+        catch
         {
             DisposeIfNeeded(value);
-            return;
+            throw;
         }
 
-        var retained = Interlocked.Increment(ref _retained);
-        if (retained <= _maxRetained)
+        var dispose = false;
+        lock (_gate)
         {
-            _items.Enqueue(value);
-            return;
+            if (_disposed)
+            {
+                dispose = true;
+            }
+            else
+            {
+                var retained = Interlocked.Increment(ref _retained);
+                if (retained <= _maxRetained)
+                {
+                    _items.Enqueue(value);
+                    return;
+                }
+
+                Interlocked.Decrement(ref _retained);
+                dispose = true;
+            }
         }
 
-        Interlocked.Decrement(ref _retained);
-        DisposeIfNeeded(value);
+        if (dispose)
+            DisposeIfNeeded(value);
     }
 
     /// <inheritdoc />
     public void Dispose()
     {
-        _disposed = true;
-        while (_items.TryDequeue(out var value))
+        var drained = new List<T>();
+        lock (_gate)
         {
-            Interlocked.Decrement(ref _retained);
-            DisposeIfNeeded(value);
+            if (_disposed)
+                return;
+
+            _disposed = true;
+            while (_items.TryDequeue(out var value))
+            {
+                Interlocked.Decrement(ref _retained);
+                drained.Add(value);
+            }
         }
+
+        foreach (var value in drained)
+            DisposeIfNeeded(value);
     }
 
     private void ThrowIfDisposed()
     {
-        if (_disposed)
+        if (Volatile.Read(ref _disposed))
             throw new ObjectDisposedException(nameof(ObjectPool<T>));
     }
 

src/PatternKit.Generators.Abstractions/Composer/ComposerAttribute.cs

@@ -21,8 +21,8 @@ public sealed class ComposerAttribute : Attribute
 
     /// <summary>
     /// Gets or sets whether to generate async methods.
-    /// When omitted, async generation is inferred from the presence of async steps or terminal.
-    /// Set to true/false explicitly to control async generation.
+    /// When this named property is omitted, async generation is inferred from async steps or terminal.
+    /// Set to true or false explicitly in source to control async generation.
     /// </summary>
     public bool GenerateAsync { get; set; }
 

src/PatternKit.Generators.Abstractions/Proxy/GenerateProxyAttribute.cs

@@ -73,7 +73,7 @@ public sealed class GenerateProxyAttribute : Attribute
 
     /// <summary>
     /// Gets or sets whether async interceptor hooks should be generated.
-    /// If not specified, async support is inferred from the contract
+    /// When this named property is omitted, async hook generation is inferred from the contract
     /// (enabled if any member returns Task/ValueTask or has a CancellationToken parameter).
     /// </summary>
     public bool GenerateAsync { get; set; }

src/PatternKit.Generators/ComposerGenerator.cs

@@ -90,8 +90,8 @@ public sealed class ComposerGenerator : IIncrementalGenerator
 
     private static readonly DiagnosticDescriptor MissingCancellationTokenDescriptor = new(
         id: DiagIdMissingCancellationToken,
-        title: "CancellationToken parameter required",
-        messageFormat: "Method '{0}' is async but missing CancellationToken parameter. Async methods should have a CancellationToken parameter.",
+        title: "CancellationToken parameter is invalid",
+        messageFormat: "Method '{0}' is async and declares a final extra parameter, but it is not System.Threading.CancellationToken",
         category: "PatternKit.Generators.Composer",
         defaultSeverity: DiagnosticSeverity.Warning,
         isEnabledByDefault: true);

test/PatternKit.Examples.Tests/Chain/MediatedTransactionPipelineDemoTests.cs

@@ -284,7 +284,7 @@ public void ComputeDelta_Rounds_To_Nearest_Nickel()
 
 public sealed class MediatedTransactionPipelineCoverageTests
 {
-    [Scenario("TransactionPipelineBuilder FluentHooks Cover CustomStagesRulesAndCouponDiscounts")]
+    [Scenario("Transaction pipeline builder fluent hooks cover custom stages rules and coupon discounts")]
     [Fact]
     public void TransactionPipelineBuilder_FluentHooks_Cover_CustomStagesRulesAndCouponDiscounts()
     {
@@ -327,7 +327,7 @@ public void TransactionPipelineBuilder_FluentHooks_Cover_CustomStagesRulesAndCou
         ScenarioExpect.Equal("custom", customResult.Result.Code);
     }
 
-    [Scenario("TransactionPipelineBuilder Preauth Blocks EmptyBaskets")]
+    [Scenario("Transaction pipeline builder preauth blocks empty baskets")]
     [Fact]
     public void TransactionPipelineBuilder_Preauth_Blocks_EmptyBaskets()
     {
@@ -347,7 +347,7 @@ public void TransactionPipelineBuilder_Preauth_Blocks_EmptyBaskets()
         ScenarioExpect.True(result.Ctx.Log.Contains("preauth: empty basket"));
     }
 
-    [Scenario("CardTenderHandlers Surface AuthorizationAndCaptureFailures")]
+    [Scenario("Card tender handlers surface authorization and capture failures")]
     [Fact]
     public void CardTenderHandlers_Surface_AuthorizationAndCaptureFailures()
     {
@@ -396,7 +396,7 @@ public void CardTenderHandlers_Surface_AuthorizationAndCaptureFailures()
         ScenarioExpect.Equal("capture-failed", strategyCaptureResult!.Value.Code);
     }
 
-    [Scenario("CharityRoundUpRule NotifiesTrackerWhenApplied")]
+    [Scenario("Charity round up rule notifies tracker when applied")]
     [Fact]
     public void CharityRoundUpRule_NotifiesTracker_WhenApplied()
     {

test/PatternKit.Examples.Tests/Generators/FacadeSpecsTests.cs

@@ -1,3 +1,4 @@
+using System.Globalization;
 using PatternKit.Examples.Generators.Facade;
 using TinyBDD;
 
@@ -9,7 +10,7 @@ namespace PatternKit.Examples.Tests.Generators;
 /// </summary>
 public class FacadeSpecsTests
 {
-    [Scenario("ShippingFacade CalculatesCostCorrectly")]
+    [Scenario("Shipping facade calculates cost correctly")]
     [Fact]
     public void ShippingFacade_CalculatesCostCorrectly()
     {
@@ -28,7 +29,7 @@ public void ShippingFacade_CalculatesCostCorrectly()
         ScenarioExpect.Equal(5.99m, cost); // local base rate, 3.5 lbs (under 5 lbs, no surcharge)
     }
 
-    [Scenario("ShippingFacade ValidatesShipmentCorrectly")]
+    [Scenario("Shipping facade validates shipment correctly")]
     [Fact]
     public void ShippingFacade_ValidatesShipmentCorrectly()
     {
@@ -53,7 +54,7 @@ public void ShippingFacade_ValidatesShipmentCorrectly()
         ScenarioExpect.False(isInvalid);
     }
 
-    [Scenario("ShippingFacade EstimatesDeliveryCorrectly")]
+    [Scenario("Shipping facade estimates delivery correctly")]
     [Fact]
     public void ShippingFacade_EstimatesDeliveryCorrectly()
     {
@@ -74,7 +75,7 @@ public void ShippingFacade_EstimatesDeliveryCorrectly()
         ScenarioExpect.True(days > 0 && days <= 12);
     }
 
-    [Scenario("ShippingSubsystems CoverFallbackDestinationsAndSpeeds")]
+    [Scenario("Shipping subsystems cover fallback destinations and speeds")]
     [Fact]
     public void ShippingSubsystems_CoverFallbackDestinationsAndSpeeds()
     {
@@ -88,7 +89,7 @@ public void ShippingSubsystems_CoverFallbackDestinationsAndSpeeds()
         ScenarioExpect.Equal(29.99m, rates.CalculateBaseRate("international"));
         ScenarioExpect.Equal(1.50m, rates.CalculateWeightSurcharge(8m));
         ScenarioExpect.Equal(12, estimator.EstimateDays("international", "economy"));
-        ScenarioExpect.Equal("$31.49 - Delivery in 12 business days", quote);
+        ScenarioExpect.Equal($"${31.49m.ToString("F2", CultureInfo.CurrentCulture)} - Delivery in 12 business days", quote);
         ScenarioExpect.Equal("Invalid shipment parameters", invalidQuote);
     }