Skip to content

Fix high-priority Pipeline review findings (immutability, async contract, docs)#137

Merged
Chris-Wolfgang merged 1 commit into
feature/83-pipeline-apifrom
fix/pipeline-review-high-priority
Apr 25, 2026
Merged

Fix high-priority Pipeline review findings (immutability, async contract, docs)#137
Chris-Wolfgang merged 1 commit into
feature/83-pipeline-apifrom
fix/pipeline-review-high-priority

Conversation

@Chris-Wolfgang
Copy link
Copy Markdown
Owner

Stacks on top of #134. Addresses the high-priority findings from the code review on the Pipeline API.

Fixes

# Finding Fix
1, 2 WithProgress silently allowed double-binding + stage instances were stateful/aliasable ExtractStageWithProgress / TransformStageWithProgress are now fully immutable (no _progress field, no SourceAsync). WithProgress returns a fresh downstream stage with the sink captured. PipelineWithLoadProgress.WithProgress now throws InvalidOperationException on a second call (it can't be made immutable cleanly because it owns Name/one-shot state).
3, 4 PipelineImpl.RunAsync let sync throws escape synchronously; one-shot flag set before _run invocation RunAsync now wraps body in try/catch and returns Task.FromException for any failure (including the one-shot InvalidOperationException). Every exit now goes through the Task, as the docs promise.
5 Overload-resolution footgun on Pipeline.Extract/.Transform/.Load — a full-capability instance passed via a bare static type silently drops cancellation/progress Added <remarks> on Pipeline class docs warning about static-type overload resolution.
8, 9 NoProgressPathTests asserted negative (!ProgressOverloadWasCalled) without a positive counterpart; no test for WithProgress double-call Added ParameterlessOverloadWasCalled flags on ProgressOnlyExtractor/Transformer/Loader; updated existing tests to assert positives; added WithProgressSemanticsTests covering builder immutability (Extract + Transform), pipeline one-shot WithProgress, and the async-contract normalization.

Stats

  • 10 files changed
  • 214/214 tests pass in Release on net10.0 (+6 new tests)
  • 0 build warnings / errors

Not included

Low-priority review findings (#6 Volatile.Read, #7 sentinel exception, #11 pre-cancel Assert.Empty) will land in a separate PR to keep this one focused.

🤖 Generated with Claude Code

@Chris-Wolfgang Chris-Wolfgang merged commit a05750b into feature/83-pipeline-api Apr 25, 2026
1 check passed
@Chris-Wolfgang Chris-Wolfgang deleted the fix/pipeline-review-high-priority branch April 25, 2026 01:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant