Make pipeline validation async-only

[thomhurst]

Closes #3808

Summary

• make IPipelineValidator async-only
• remove duplicated sync-over-async validation paths
• adapt synchronous validators with completed tasks and guard the public contract

Validation

• ModularPipelines.slnx Release build: 0 warnings, 0 errors
• ValidationTests: 40/40 passed
• targeted formatter passed

The focused test project used the same temporary local omission of the unrelated stale run-report option initializers fixed separately by #3864; those lines were restored and are not part of this PR.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 888a9414ea

ℹ️ 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".

[chatgpt-codex-connector]

Update the stale validator contract test

When the full ModularPipelines.UnitTests project is run, this new async-only assertion is mutually exclusive with the existing ValidationInterfaceTests.IPipelineValidator_ShouldHaveOrderAndValidateMembers test, which still asserts typeof(IPipelineValidator).GetMethod("Validate") is not null. Because this commit removes Validate, that unchanged test will now fail until it is updated or removed, so CI that runs all unit tests remains red.

Useful? React with 👍 / 👎.

[thomhurst]

Fixed in 6c4850c: removed the obsolete synchronous Validate assertion and renamed the contract test for ValidateAsync. Targeted validation tests pass (3/3 interface, 40/40 behavior).

[claude]

Code review

No issues found. Checked for bugs and CLAUDE.md compliance.

Notes from review:

• The removal of the synchronous Validate(IServiceProvider) method from IPipelineValidator is clean — verified no remaining callers rely on it, and each implementer (ArtifactContractValidator, DependencyValidator, ModuleConfigurationValidator, ModuleSelectionValidator, OptionsValidator) correctly replaces .GetAwaiter().GetResult() sync-over-async calls with proper await, preserving identical validation logic.
• PipelineValidationService already only invoked ValidateAsync, so the interface change doesn't ripple further.
• Good addition of PipelineValidatorContractIsAsyncOnly test to lock in the async-only contract going forward.

[thomhurst]

@codex review

[thomhurst]

@claude review

[claude]

Code review (re-review after latest commit)

No issues found — this looks good to merge.

Verified:

• Confirmed there are no remaining callers or implementers of the old sync Validate(IServiceProvider) method anywhere in the repo (checked all IPipelineValidator/IDependencyValidator/IOptionsValidator/IModuleConfigurationValidator implementers and DI registrations in DependencyInjectionSetup.cs) — the interface cleanup is complete, not partial.
• ArtifactContractValidator.ValidateAsync and DependencyValidator.ValidateAsync correctly replace the previous .GetAwaiter().GetResult() sync-over-async fallback path with a proper await, and the validation logic itself is untouched (same two-phase "fast path, then fall back to runnable-module resolution on error" structure).
• The new PipelineValidatorContractIsAsyncOnly test (ValidationTests.cs) is a good regression guard — it reflects over IPipelineValidator to assert no Validate method exists and that ValidateAsync returns Task<ValidationResult>, which will catch any future reintroduction of a sync member.
• ThrowingValidator in the tests now throws via Task.FromException<ValidationResult> instead of a synchronous throw — verified this still propagates as InvalidOperationException through await builder.BuildAsync() in BuildAsync_WhenCustomValidatorThrows_DisposesPipeline, so exception/disposal semantics are preserved.
• This is a deliberate breaking change to a public extension point (IPipelineValidator), scoped explicitly for v4 per the linked issue v4: IPipelineValidator dual sync/async API forces sync-over-async — make validation async-only #3808 (part of the v4 scoping audit umbrella v4 major upgrade: scoping audit umbrella #3239), so removing the default ValidateAsync implementation and the sync Validate method without a compat shim is appropriate here rather than a concern.
• Rebuilt ModularPipelines.slnx locally (Release): 0 warnings, 0 errors, consistent with the PR description's validation notes.

One very minor note, not blocking: IOptionsValidator.ValidateOptions(PipelineOptions, IReadOnlySet<string>) still has a default implementation that delegates to the sync single-arg overload — that's unrelated to this PR's scope (it's a different validator surface than IPipelineValidator.ValidateAsync), so no action needed here, just flagging for awareness if a future pass wants to fully align all validator surfaces to async.

[chatgpt-codex-connector]

Codex Review: Didn't find any major issues. Chef's kiss.

Reviewed commit: 6c4850c03f

ℹ️ 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".

[thomhurst]

Refreshed with current main at 93f1302, removing the stale run-report compiler blocker. Merge was clean. Validation: ValidationTests 40/40, ValidationInterfaceTests 3/3, lightweight core Release build 0 warnings/errors. Please re-review current head.

[claude]

Code review

No issues found. Checked for bugs and CLAUDE.md compliance.

The latest commit (93f130253f) is a merge of origin/main into this branch. I diffed it against the previously-reviewed commit (6c4850c03f) and confirmed the merge only pulls in unrelated upstream changes (e.g. the non-throwing pipeline failure mode work) — the actual PR content in src/ModularPipelines/Validation/** and test/ModularPipelines.UnitTests/Validation/** is byte-for-byte unchanged. The prior reviews already confirmed:

• Clean removal of the synchronous Validate(IServiceProvider) path from IPipelineValidator, with all implementers converted to async-only.
• No remaining sync callers/implementers of the old contract.
• ValidationTests (40/40) and ValidationInterfaceTests (3/3) pass.

No new issues to add for this refresh.

[thomhurst]

Refreshed onto current main after Ubuntu's core test host ran 15m52s and exited without a failing-test summary. Validation on e80a81c: ValidationTests 40/40, ValidationInterfaceTests 3/3, and ModularPipelines.slnx Release build 0 warnings/errors.

[claude]

Code review (re-review at e80a81c)

No issues found — this is a clean, well-scoped change and ready to merge as far as this review is concerned.

Verified independently:

• git diff 93f130253f e80a81c441 -- src/ModularPipelines/Validation test/ModularPipelines.UnitTests/Validation is empty — the latest push is purely a merge from main picking up the run-report fix from Fix stale run-report test options #3864; no new changes to review in the validation surface.
• Confirmed via repo-wide grep that no caller or implementer anywhere references the removed sync Validate(IServiceProvider) member — the IPipelineValidator contract is fully async-only, not partially migrated.
• ArtifactContractValidator.ValidateAsync and DependencyValidator.ValidateAsync now await their fallback resolution (GetRunnableModulesForArtifactValidationAsync / GetRunnableModulesForValidation) instead of the previous .GetAwaiter().GetResult() sync-over-async call — this is a genuine improvement, not just cosmetic: sync-over-async on a thread-pool-bound host (e.g. under a synchronization context or constrained thread pool) risks deadlock/starvation, so removing it is the right call architecturally, not merely a style change.
• PipelineValidationService.ValidateAsync (the only caller of IPipelineValidator.ValidateAsync) already awaited each validator in sequence, so the interface tightening doesn't ripple into any new async-over-sync or blocking call elsewhere.
• The new PipelineValidatorContractIsAsyncOnly test in ValidationTests.cs reflects over the interface to assert no Validate method exists and ValidateAsync returns Task<ValidationResult> — a good regression guard against reintroducing the dual sync/async surface.
• ThrowingValidator in the tests correctly switched from a synchronous throw to Task.FromException<ValidationResult>(...), preserving the exception/disposal semantics exercised by BuildAsync_WhenCustomValidatorThrows_DisposesPipeline.
• This is a deliberate breaking change to a public extension point (IPipelineValidator), explicitly scoped for v4 per the linked issue v4: IPipelineValidator dual sync/async API forces sync-over-async — make validation async-only #3808 under the v4 scoping audit (v4 major upgrade: scoping audit umbrella #3239) — removing the default ValidateAsync implementation and the sync Validate method without a compatibility shim is appropriate here rather than a concern, since a shim would just reintroduce the sync-over-async pattern this PR exists to remove.

One pre-existing, out-of-scope observation (not blocking, already flagged in an earlier review pass on this PR): IOptionsValidator.ValidateOptions(PipelineOptions, IReadOnlySet<string>) remains a synchronous surface separate from IPipelineValidator.ValidateAsync. It's a different validator interface than the one this PR targets, so no action needed here — just worth a follow-up issue if the project wants to fully align all validator surfaces to async for consistency.