Align fluent and attribute skip semantics

[thomhurst]

Summary

• make repeated WithSkipWhen calls compose OR-to-skip, matching repeated [SkipIf<T>] attributes
• add explicit sync and async WithSkipWhenAll(params ...) groups for AND-to-skip behavior
• evaluate attribute conditions in the execution pipeline after dependency waiting, so attribute and fluent skips invoke the same direct hooks, lifecycle handlers, receivers, and notifications
• preserve first-class CLI module-selection skips during discovery, then apply category-only discovery skips
• update skip/run-condition documentation

Validation

• 110/110 tests passed across configuration, attribute conditions, lifecycle events, direct hooks, retrieval, execution-pipeline, and validation coverage
• ModularPipelines.sln Release build passed with 0 errors (existing warnings only)
• git diff --check passed

Fixes #3487

Validation behavior

Fluent dependencies are validated before execution conditions. Attribute-conditioned modules must therefore register every DependsOn<T>() dependency even when the condition will skip them on the current platform or environment.

Distributed OS-routing follow-up

• Keep worker-routable OS conditions deferred while filtering impossible intersections during master discovery, before assignments are published.
• ModuleConditionHandlerTests: 11/11; OperatingSystemConditionsTests: 3/3.

[thomhurst]

@codex review

[chatgpt-codex-connector]

Codex Review: Didn't find any major issues. Already looking forward to the next diff.

Reviewed commit: 353cf9282a

ℹ️ 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]

@codex review

CI regression fixed in c132a28: execution-time [RunIfAll] skips now correctly retain dependency validation expectations. ValidationTests: 31/31 passed; scoped format clean.

[chatgpt-codex-connector]

Codex Review: Didn't find any major issues. Can't wait for the next one!

Reviewed commit: c132a28a42

ℹ️ 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]

Rebased onto current main; new head 17dd8d211f. Whole-PR patch-id remained exact: e39df0c51dea5755601300d31a8cd7b3c96efba2.

Validation after rebase:

• ValidationTests: 31/31 passed
• ModularPipelines.sln Release build: 0 errors (227 pre-existing warnings)
• git diff --check: passed

@codex review

[claude]

Review: Align fluent and attribute skip semantics (#3571)

Solid, well-tested change. The OR-to-skip default for repeated WithSkipWhen (matching [SkipIf<T>]) plus the explicit WithSkipWhenAll(...) opt-in for AND semantics is the right call from the issue's two proposed fixes — it keeps the common case (SkipIf<A> + SkipIf<B> migrated to fluent) behaving the same way instead of silently inverting to AND. The ComposeSkipConditions/ComposeAllSkipConditions split (outer OR across registered groups, inner AND within a WithSkipWhenAll group) is clean and the short-circuit tests (WithSkipWhen_RepeatedCalls_OrComposeAndShortCircuit, WithSkipWhenAll_StopsWhenConditionDoesNotSkip) cover the important edge cases.

Worth calling out explicitly (not a bug, but a hidden consequence of the unification)

Moving SkipIf/RunIfAll/RunIfAny evaluation out of discovery and into ModuleExecutionPipeline (after dependency waiting) has a side effect on validation that isn't mentioned in the summary or docs:

• ModuleRetriever.DiscoverModules now calls _moduleConditionHandler.ShouldIgnoreByCategory (category-only) instead of ShouldIgnore (category and attribute conditions). GetRunnableModulesForValidation uses the same discovery path, so PipelineValidator's fluent-dependency check now runs against every module, including ones an attribute condition provably skips.
• Before this PR, a module like [OnWindows] [DependsOn-via-fluent<WindowsOnlyModule>] that would never run on Linux was excluded from discovery entirely (cascaded into IgnoredModules), so a missing fluent dependency never surfaced as a validation error. After this PR, that same module now throws PipelineValidationException at BuildAsync/ValidateAsync time regardless of platform, because attribute-gated modules are no longer treated as statically ignorable for validation purposes.
• The test suite tracks this correctly (BuildAsync_Rejects_Fluent_Missing_Dependency_For_Execution_Skipped_Module / ValidateAsync_Rejects_Fluent_Missing_Dependency_For_Execution_Skipped_Module replace the old ..._Ignores_Fluent_Dependencies_For_Discovery_Skipped_Modules tests), so this isn't an oversight — but it's a real behavior change for anyone who used attribute conditions as a way to guard fluent DependsOn<T>() chains that only exist on some platforms/environments. Given the issue is already flagged Breaking: yes, I'd suggest adding a line to the PR description/changelog (and maybe a doc note in run-conditions.md or skipping.md) explicitly stating that fluent dependencies declared on attribute-conditioned modules are now always validated, independent of whether the condition will skip the module at runtime. Silent breaking changes in validation strictness are the kind of thing that turn into confusing GitHub issues down the line ("this pipeline built fine last version, now it throws on CI").

Arguably this is a correctness improvement (an IRunCondition can depend on runtime/HTTP/env state, so "provably skipped at discovery" was already a fragile guarantee to build validation leniency on), but it should be a documented, intentional decision rather than an emergent side effect of the pipeline unification.

Minor

• ModuleExecutionPipeline.ExecuteAsync's fallback SkipDecision.Skip("Module was ignored") duplicates the same fallback string in ModuleRetriever.DiscoverModules. Not worth a shared helper for one string, just noting the duplication.
• ModuleConditionHandler.EvaluateShouldIgnore re-evaluates EvaluateCategoryConditions for every module reaching ModuleExecutionPipeline, even though ModuleRetriever already ran ShouldIgnoreByCategory for the same module during discovery. Category conditions are cheap/pure so this is harmless, but it is a redundant second evaluation per module now that the two check-paths are separate — worth a one-line comment if intentional, since a future reader may assume ShouldIgnore and ShouldIgnoreByCategory are mutually exclusive call sites.

Everything else (the WithSkipWhenAll null/empty validation, the lifecycle/hook-invocation test additions in LifecycleEventIntegrationTests and DirectModuleHooksTests, and the doc updates) looks accurate and consistent with the stated goal of unifying skip hook/notification behavior between attributes and the fluent API.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 17dd8d211f

ℹ️ 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]

Rebased onto current main after #3564 merged. Patch equivalence preserved (e39df0c51dea5755601300d31a8cd7b3c96efba2). New head: da5bf7d44c.

@codex review

[claude]

Follow-up review

I reviewed this PR previously. The diff is still patch-identical to that review (e39df0c51dea5755601300d31a8cd7b3c96efba2 — confirmed via git patch-id against the current head da5bf7d44c); the only changes since then have been rebases onto main, not code changes. No new issues to raise.

One item from the earlier review is still outstanding: ModuleRetriever.DiscoverModules/GetRunnableModulesForValidation now call ShouldIgnoreByCategory instead of ShouldIgnore, so PipelineValidator's fluent-dependency check runs against every module — including ones an attribute condition (SkipIf/OnWindows/etc.) provably skips at runtime. Before this PR, a module like [OnWindows] [DependsOn-via-fluent<WindowsOnlyModule>] was excluded from discovery on other platforms, so a missing fluent dependency never surfaced as a validation error; after this PR it now throws PipelineValidationException at BuildAsync/ValidateAsync time regardless of platform. This is a deliberate, test-covered behavior change (the _Rejects_Fluent_Missing_Dependency_For_Execution_Skipped_Module tests confirm it's intentional), and arguably a correctness improvement — but it's still not called out anywhere in the PR description or in run-conditions.md/skipping.md, both of which were updated in this PR for the hook-timing unification but not for this validation-strictness change.

Why this is worth documenting explicitly rather than leaving as an emergent side effect: anyone currently relying on attribute conditions to guard platform/environment-specific fluent DependsOn<T>() chains will see previously-passing pipelines start throwing PipelineValidationException after upgrading, with no changelog line pointing at the cause. That's the kind of silent breaking change that turns into a confusing "worked in the last version, now it throws on CI" issue report. Since the issue this PR fixes is already flagged Breaking: yes, adding one sentence to the PR description (and a short note in the docs) about validation now being independent of runtime-skippability would close the gap between "the tests prove this is intentional" and "a consumer reading the docs would know to expect it."

No other issues found; the rest of the change (the WithSkipWhen/WithSkipWhenAll OR/AND composition, the lifecycle/hook test additions, and the discovery→execution-pipeline unification) still looks correct and well covered by tests.

[chatgpt-codex-connector]

Codex Review: Didn't find any major issues. You're on a roll.

Reviewed commit: da5bf7d44c

ℹ️ 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]

Addressed review feedback: async WithSkipWhenAll arrays are snapshotted, with regression coverage; unconditional fluent-dependency validation is documented in the guide and PR description. Configuration tests pass (33/33), validation tests pass (31/31), core Release build has 0 errors. Branch updated from main. @codex review

[chatgpt-codex-connector]

Codex Review: Didn't find any major issues. What shall we delve into next?

Reviewed commit: 1c54ec6163

ℹ️ 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]

@codex review

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 1c54ec6163

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

[claude]

Follow-up review (post-rebase, head f735223)

I reviewed this PR twice before; this pass verifies the current head against those earlier rounds rather than re-deriving everything from scratch.

Both previously-raised items are now resolved:

1. Async WithSkipWhenAll snapshot (Codex, ModuleConfigurationBuilder.cs): the async overload now does ComposeAllSkipConditions([.. conditions]), which spreads into a fresh array before storing it — mutating the caller's original array after Build() no longer changes composed skip behavior. Matches the sync overload's Array.ConvertAll snapshot.
2. Undocumented validation-strictness change (my earlier review): run-conditions.md now has an explicit paragraph — "Fluent dependencies are validated before execution conditions are evaluated. Every dependency declared with DependsOn<T>() must therefore be registered, even when an attribute condition will skip the consuming module..." — and the PR description carries the same note under "Validation behavior." That closes the gap between "tests prove this is intentional" and "a consumer reading the docs would know to expect it."

Checked the post-merge state of ModuleRetriever.cs specifically, since the merge from main touched that file (conflict noted in the merge commit) — DiscoverModules/GetRunnableModulesForValidation still correctly call ShouldIgnoreByCategory (not ShouldIgnore), so the validation-strictness behavior survived the rebase onto #3569 ("Harden category filter matching and validation") cleanly.

Design read

The ComposeSkipConditions (OR across groups, stops on first skip) / ComposeAllSkipConditions (AND within a WithSkipWhenAll group, short-circuits on first non-skip) split is the right shape: it keeps WithSkipWhen's default behavior aligned with repeated [SkipIf<T>] attributes (OR-to-skip) rather than silently flipping to AND, while still giving callers who want AND semantics an explicit, discoverable API (WithSkipWhenAll) instead of forcing them to hand-roll a combinator inside a single lambda. Routing attribute conditions through ModuleExecutionPipeline (after dependency waiting) instead of evaluating them only at discovery is the correct fix for the underlying bug — it's what makes attribute-driven skips invoke the same OnSkippedAsync/lifecycle notifications as fluent skips, which is the whole point of the issue this closes.

Still-minor, not blocking

• ModuleConditionHandler.EvaluateShouldIgnore (called from ModuleExecutionPipeline) re-runs EvaluateCategoryConditions per module even though ModuleRetriever already called ShouldIgnoreByCategory for the same module during discovery. Category conditions are cheap and pure, so this is harmless today, but the two now-separate call sites (ShouldIgnoreByCategory for discovery, ShouldIgnore for execution) aren't obviously non-overlapping to a future reader — a one-line comment on ShouldIgnore noting it deliberately re-checks category conditions would save someone a "wait, is this a bug?" moment later.
• The SkipDecision.Skip("Module was ignored") fallback string is now duplicated between ModuleRetriever.DiscoverModules and ModuleExecutionPipeline.ExecuteAsync. Not worth a shared constant for one string, just flagging it stayed as-is.

Nothing else to add — the OR/AND composition tests, the LifecycleEventIntegrationTests/DirectModuleHooksTests additions covering attribute-triggered OnSkippedAsync/event-log invocation, and the validation test renames (..._Rejects_Fluent_Missing_Dependency_For_Execution_Skipped_Module) all line up with the stated goal and confirm the behavior is intentional rather than emergent. Approving from a code standpoint.

[thomhurst]

Fixed distributed discovery: master-side category filtering now also rejects impossible OS intersections, while a single routable OS target remains deferred to workers. Condition handler: 11/11; OS mapping: 3/3. @codex review

[claude]

Follow-up review

Reviewed the latest changes since the previous review (head now 0734e1bc7, adding the "distributed OS-routing follow-up" commit on top of da5bf7d44c/1c54ec6163).

Previously flagged issue is resolved. The unconditional fluent-dependency-validation behavior change is now called out explicitly in the PR description ("Validation behavior" section) and in run-conditions.md ("Fluent dependencies are validated before execution conditions are evaluated..."). The WithSkipWhenAll async-array snapshot fix also has dedicated regression coverage (WithSkipWhenAll_SnapshotsAsyncConditionGroups), confirming a mutated caller-owned array no longer changes evaluation after Build().

New in this round — distributed OS-routing fix:

• OperatingSystemConditions.HasImpossibleCombination correctly intersects OS-capability sets both within a single RunIfAll<T1, T2> attribute and across multiple stacked RunIfAll<>/OnX attributes on the same module, flagging the module only when the resulting set is empty (not merely absent). Verified this holds for both a single contradictory attribute and two separate contradictory attributes.
• ModuleConditionHandler.ShouldIgnoreByCategory applies this check only when IsDistributedMaster(), so single-machine and worker instances still resolve genuinely-impossible combinations the normal way at execution time (they'll simply evaluate false on whichever OS actually runs them) — this proactive filter exists purely to stop the master from ever trying to publish a distributed work item that no worker could satisfy.
• The routable single-OS-target case (CreateForeignOsModule) is correctly not filtered at discovery, preserving the existing defer-to-worker routing path in EvaluateAllConditions (deferOperatingSystemConditions).
• Test coverage (Distributed_Master_Discovery_Filters_Module_With_Contradictory_Os_Conditions, Distributed_Master_Discovery_Does_Not_Filter_Routable_Os_Condition) directly exercises the new ShouldIgnoreByCategory method for both cases.

Architecture check on the discovery/execution split: ModuleRetriever.DiscoverModules now only filters on category + (master-only) impossible-OS-combination via ShouldIgnoreByCategory, while ModuleExecutionPipeline.ExecuteAsync evaluates the full attribute condition set via ShouldIgnore after dependency waiting. This cleanly unifies attribute-based and fluent WithSkipWhen skips onto one hook/notification path (the PR's stated goal) without duplicating condition evaluation logic — ShouldIgnore's ConditionalWeakTable-backed cache means each module's full condition set is still evaluated at most once even though it's invoked from a different call site than before. FinalizeMetadata now runs twice per module (once in discovery's category check, once in execution's), but that's an existing idempotent operation already called from several other sites in the engine, so it's not a correctness concern.

No new issues found. The composition of WithSkipWhen (OR-to-skip) and WithSkipWhenAll (AND-to-skip, short-circuiting) is consistent between the builder, its doc comments, and skipping.md/run-conditions.md, and the sync/async snapshot behavior is symmetric and tested.

[chatgpt-codex-connector]

Codex Review: Didn't find any major issues. Swish!

Reviewed commit: 0734e1bc7e

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

[claude]

Follow-up review (head d8617e4b3f)

I've reviewed this PR three times before; this pass focuses on what's new since the last round (head f735223): the merge from main (bringing in the unrelated typed-ModuleResult PR #3573, which merged cleanly with no touch on this branch's own files) and one new commit, fix(distributed): reject impossible OS routes (0734e1bc7e).

New commit: rejecting impossible OS routes — correct, and verified end-to-end

ModuleConditionHandler.ShouldIgnoreByCategory now also flags a module as ignored when running as the distributed master and OperatingSystemConditions.HasImpossibleCombination(...) finds that its RunIfAll-style attributes require mutually exclusive operating systems (e.g. a module stacked with both [RunIfAll<OnWindows>] and [RunIfAll<OnLinux>]).

This is a real bug fix, not a hypothetical: DistributedWorkPublisher.CreateAssignment builds RequiredCapabilities by unioning OperatingSystemConditions.GetTargets(...) across each condition attribute independently (DistributedWorkPublisher.cs:38-44), so a module with contradictory OS attributes would previously end up with a capability set like {"windows", "linux"} — an assignment no single worker could ever satisfy, meaning the master would publish it and then wait forever.

I traced the actual call graph to confirm the fix takes effect (not just satisfied by the new unit tests in isolation):
ExecutionOrchestrator → ModuleRetriever.GetOrganizedModules() → DiscoverModules → ShouldIgnoreByCategory (now catching the contradiction) → module lands in modulesToIgnore, never modulesToProcess → runnableModules (excludes it) → PipelineExecutor.ExecuteAsync → DistributedModuleExecutor.ExecuteAsync(modules) → scheduler → only modules that reach ReadyModules ever hit _publisher.CreateAssignment/PublishAsync. There's no separate/parallel enumeration that would let the flagged module reach the publisher some other way. So this is correctly wired, not dead code guarded only by its own unit test.

The intersection logic in HasImpossibleCombination itself is sound: it walks every ConditionLogic.All attribute on the module, skips non-OS conditions (GetSupportedOperatingSystems returns null for those, so they don't pollute the intersection), and only flags Count == 0 when at least one OS-bearing attribute was actually seen (a bare null accumulator, meaning no OS constraints existed at all, correctly does not trigger). I checked this against a few edge cases (three-way pairwise-compatible-but-jointly-impossible attribute stacks, ConditionGroup-based conditions like OnUnix, single-attribute cases) and the sequential-intersection approach handles all of them correctly.

Minor / non-blocking

• This fix is scoped to IsDistributedMaster(), so standalone execution and workers never run this check — that's fine, since in those contexts the existing local EvaluateAllConditions path already correctly fails a contradictory RunIfAll at execution time (no worker to route to, so no wasted round trip to worry about).
• There's no test at the DistributedModuleExecutor/DistributedWorkPublisher level asserting that a contradictory-OS module never results in a published assignment — coverage stops at the ModuleConditionHandler unit tests (Distributed_Master_Discovery_Filters_Module_With_Contradictory_Os_Conditions / ..._Does_Not_Filter_Routable_Os_Condition). Given I had to trace the wiring manually to be confident it's connected, a thin executor-level regression test (assert PublishAsync is never called for such a module) would make that guarantee self-evident to future readers without needing to re-derive the call graph. Not blocking — the unit tests plus the wiring I traced give good confidence today.
• Minor scope note: this fix addresses a distributed-master routing bug that's conceptually distinct from the PR's core subject (skip-condition composition semantics for v4: WithSkipWhen composes AND-to-skip while [SkipIf] attributes compose OR-to-skip #3487). It's small and low-risk enough that I wouldn't ask for it to be split out, but it's worth flagging in case there's a preference for keeping this PR's diff focused on one fix per PR.

Prior review items — still resolved

Confirmed again that both previously-raised items remain fixed after the rebase/merge: the async WithSkipWhenAll snapshot in ModuleConfigurationBuilder.cs, and the validation-strictness documentation in run-conditions.md/PR description (fluent dependencies on attribute-conditioned modules are now always validated, independent of runtime-skippability). ModuleRetriever.DiscoverModules/GetRunnableModulesForValidation still correctly call ShouldIgnoreByCategory, not ShouldIgnore.

No new issues found. The rest of the diff (the WithSkipWhen/WithSkipWhenAll OR/AND composition, lifecycle/hook unification, and docs) is unchanged from what I reviewed previously and still looks correct and well covered by tests.

[claude]

Follow-up review (head 284705f2b)

I've reviewed this PR four times now. Since the last review (head d8617e4b3f), the only change is one more merge from main picking up commit c658da445 ("Correct generated CLI enum values", #3572) — this touches only unrelated Docker/Minikube/Pulumi generated option/enum files and the CLI options generator/scraper, none of which overlap with this PR's own files (ModuleConditionHandler, ModuleConfigurationBuilder, DistributedWorkPublisher, run-conditions.md, skipping.md, etc.). No new code from this PR's own branch to review.

All previously raised items remain resolved:

• The validation-strictness behavior change (fluent dependencies now validated independent of runtime-skippability) is documented in the PR description and run-conditions.md.
• The WithSkipWhenAll async-array snapshot fix has dedicated regression coverage.
• The distributed OS-routing fix (ShouldIgnoreByCategory rejecting impossible RunIfAll OS combinations for the distributed master) is correctly wired end-to-end and test-covered.

No new issues found.

[claude]

Follow-up review (head 476f733ca6)

I've reviewed this PR five times now. Checked the current head against the prior round (head 284705f2b, reviewed 2026-07-30T23:38): the PR still has exactly the same four own-branch commits (923954c1c "align skip condition semantics", 6ff0cc8e2 "align execution skip checks", e16587542 "snapshot async skip groups", 476f733ca "reject impossible OS routes"), all authored before that last review, and the diff's file list and content are unchanged — this is a rebase/re-merge preview onto current main, not new code.

Spot-checked the two previously-central fixes directly in the current diff to confirm they weren't regressed by the rebase:

• ModuleConditionHandler.ShouldIgnoreByCategory still applies OperatingSystemConditions.HasImpossibleCombination(...) only when IsDistributedMaster(), and ModuleRetriever.DiscoverModules still calls it (not ShouldIgnore) for discovery — the distributed OS-routing fix is intact.
• The async WithSkipWhenAll overload still snapshots via ComposeAllSkipConditions([.. conditions]), matching the sync overload's Array.ConvertAll copy — the mutable-array issue Codex flagged stays fixed.

All previously raised items remain resolved:

• Validation-strictness behavior change (fluent dependencies now validated independent of runtime-skippability) is documented in the PR description and run-conditions.md.
• WithSkipWhenAll async-array snapshot has dedicated regression coverage.
• Distributed OS-routing fix is correctly wired end-to-end (traced discovery → scheduler → publisher in an earlier pass) and test-covered.

No new issues found. Nothing in this round changes my prior assessment: the WithSkipWhen/WithSkipWhenAll OR/AND composition design is sound, the discovery/execution-pipeline split cleanly unifies attribute and fluent skip hooks without duplicating condition-evaluation logic, and the test coverage (configuration, attribute conditions, lifecycle events, direct hooks, validation) matches the stated goal.