Harden category filter matching and validation

[thomhurst]

Summary

• match RunOnlyCategories and IgnoreCategories with StringComparer.OrdinalIgnoreCase
• validate category conflicts case-insensitively
• reject unknown run/ignore categories and run-only filters that select zero registered modules
• normalize caller-provided category sets regardless of their comparer
• preserve source and binary-facing compatibility for existing IOptionsValidator implementations through a default overload
• document matching and validation behavior

Validation

• RunnableCategoryTests: 5/5 passed
• ValidationTests: 39/39 passed
• legacy IOptionsValidator implementation regression passed
• scoped formatting/analyzers: clean at error severity
• ModularPipelines.sln Release build: 0 errors (227 existing warnings)
• Docusaurus production build: passed

Closes #3489

[thomhurst]

@codex review

[chatgpt-codex-connector]

Codex Review: Didn't find any major issues. 🚀

Reviewed commit: db0fe36a45

ℹ️ 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 cleanly onto current main, restarting gates after the prior Ubuntu run twice hung in the unrelated ExecuteCommandLineToolAsync_ForcefulCancellation_KillsDescendantProcesses test. @codex review

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 22f5c6a9c7

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

Code review

Reviewed the category-filtering hardening (case-insensitive matching + validation of unmatched/zero-match run/ignore categories).

Summary of changes

• ModuleConditionHandler: run/ignore category checks now use StringComparer.OrdinalIgnoreCase, matching the comparer already used by DependsOnModulesInCategoryAttribute and module tags elsewhere in the codebase.
• OptionsValidator: the existing Run/Ignore conflict check is now case-insensitive, and a new ValidateRegisteredCategories step resolves all registered modules' finalized metadata (via IModuleMetadataRegistry) to catch typo'd/unregistered categories and RunOnlyCategories filters that would silently select zero modules.
• Docs and tests updated to match.

What I checked

• Module resolution: services.GetServices<IModule>() in the new validator doesn't introduce new eager/expensive construction — modules are already registered as singletons and are already resolved earlier in PipelineImpl.CreateAsync before any IPipelineValidator runs (DependencyValidator/ModuleConfigurationValidator already do the same). FinalizeMetadata is idempotent (Lazy<T> cache keyed by module type), so calling it again here is a cheap no-op, not duplicated construction/side effects.
• Category resolution can't be derived from a pure Type attribute scan alone — it also honors registration-time WithCategory() overrides, Configuration.Category, and ITaggedModule.Category — so resolving through the already-constructed singleton instance, as this PR does, is the correct approach.
• Test coverage looks solid: case-insensitive match/skip behavior for both Run and Ignore categories, conflict detection with mixed case, fully-unmatched vs partially-unmatched RunOnlyCategories, unmatched IgnoreCategories, and a "no false positive" test confirming matching categories don't raise errors.
• This closes #3489 and brings category matching in line with the case-insensitivity already used for tags elsewhere in the codebase, so the new behavior is consistent rather than a one-off special case.

No bugs or CLAUDE.md violations found. This turns a previously silent "green but ran nothing" failure mode into an explicit validation error, which is a solid UX/correctness improvement. Nice work.

[thomhurst]

@codex review

[chatgpt-codex-connector]

Codex Review: Didn't find any major issues. Another round soon, please!

Reviewed commit: eabaa55dc2

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

Review

Solid, well-scoped fix for #3489. Case-insensitive matching in ModuleConditionHandler mirrors the existing Intersect(..., StringComparer.OrdinalIgnoreCase) conflict check, and the new "zero-selection"/"unknown category" validation is exactly the kind of fail-fast guard that prevents a typoed RunOnlyCategories from silently producing a green no-op pipeline. Test coverage for the new validation paths (unknown category, partially-unknown, case-insensitive match/conflict) is thorough, and the bot's earlier comment about the two test suites that intentionally filtered out all modules (ModuleHistoryTests, ResultsRepositoryTests.RunTwoFromHistory) was correctly addressed in eabaa55 by adding a runnable module in the affected category so those scenarios still exercise their original skip/history assertions instead of tripping the new validator.

One design gap worth considering before merge:

IOptionsValidator.ValidateOptions(PipelineOptions) no longer represents "the" options validation.
OptionsValidator.Validate(IServiceProvider) (src/ModularPipelines/Validation/OptionsValidator.cs:18-30) now runs ValidateOptions(options) and the new ValidateRegisteredCategories(services, options, result), but the registered-category/zero-selection check only happens in that DI-aware path. The public method ValidateOptions(PipelineOptions options) — documented on the public IOptionsValidator interface as "Validates the pipeline options" — still only runs the original checks (retry count, timeouts, RunOnly/Ignore conflicts) and has no way to see registered modules, since it doesn't take an IServiceProvider/module list.

That's a real footgun: IOptionsValidator is a public extensibility point, and any consumer who resolves IOptionsValidator and calls ValidateOptions(...) directly (e.g. to validate options before wiring up a full host, or in a custom IPipelineValidator that composes it) will silently miss the exact class of bug this PR sets out to catch — a typoed or now-empty RunOnlyCategories/IgnoreCategories. The two entry points now disagree about what "valid options" means, and nothing in the code or doc comments signals that ValidateOptions is now the partial/legacy path.

Suggested fix: either (a) extend the interface so category-vs-registered-module validation has an explicit signature that takes the known categories/modules (e.g. an overload ValidateOptions(PipelineOptions options, IReadOnlySet<string> registeredCategories) that Validate(IServiceProvider) computes and forwards to), so both entry points funnel through one implementation and the "registered categories" concept becomes part of the public contract, or (b) update the ValidateOptions XML doc to explicitly scope it to options-only validation and point callers at Validate(IServiceProvider)/ValidateAsync for the full picture. Option (a) is preferable — it keeps a single source of truth for "is this configuration valid" rather than two, and avoids a latent inconsistency the next time someone adds another services-dependent options check.

Nothing else stood out — the HashSet<string>(StringComparer.OrdinalIgnoreCase) / Distinct(StringComparer.OrdinalIgnoreCase) usage is consistent throughout, the "all unmatched" vs "some unmatched" distinction in ValidateRunOnlyCategories produces the right message in each case, and the docs update accurately describes the new behavior.

[thomhurst]

Investigated the Ubuntu pipeline failure: the only failing test was unrelated CommandLoggerTests.Fast_Command_Logs_Complete_Output_When_Result_Is_Truncated (1/1311 core tests); category-filtering tests were not implicated.

Rebased onto current main (including merged command/cancellation fixes); new head 3e914d952b. Whole-PR patch-id remained exact: 55adf802bcb11d04837fe440f2cdcbe6883f651e.

Validation after rebase:

• RunnableCategoryTests: 5/5
• ModuleHistoryTests: 14/14
• ResultsRepositoryTests: 2/2
• ValidationTests: 36/36
• ModularPipelines.sln Release build: 0 errors (227 pre-existing warnings)
• docs production build: passed
• git diff --check: passed

@codex review

[chatgpt-codex-connector]

Codex Review: Didn't find any major issues. Another round soon, please!

Reviewed commit: 3e914d952b

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

Code review

Re-reviewed at 3e914d95 (after the RunnableCategoryModule/OtherCategoryModule fixes to ModuleHistoryTests/ResultsRepositoryTests, which correctly address the earlier feedback about tests that intentionally filtered out all modules).

The rest of the change is solid: IsRunnableCategory/IsIgnoreCategory in ModuleConditionHandler.cs now match StringComparer.OrdinalIgnoreCase, consistent with the existing conflict check, and the new zero-selection/unknown-category validation is a good fail-fast guard against a typoed RunOnlyCategories silently no-opping the pipeline.

One design gap from the earlier review is still unresolved: OptionsValidator.ValidateOptions(PipelineOptions) — the public method on the public IOptionsValidator interface — still only runs the original checks (retry count, timeouts, RunOnly/Ignore conflicts). The new registered-category/zero-selection check lives exclusively in ValidateRegisteredCategories, which is only invoked from Validate(IServiceProvider).

This means the two public entry points on IOptionsValidator now disagree about what "valid options" means: any consumer who resolves IOptionsValidator and calls ValidateOptions(options) directly — e.g. to validate configuration before wiring up a full host, or from a custom IPipelineValidator that composes it — will silently miss exactly the class of bug this PR sets out to catch (a typoed or now-empty RunOnlyCategories/IgnoreCategories), and nothing in the code or XML docs signals that ValidateOptions is now the partial/legacy path.

Suggested fix (still preferred over just updating the doc comment): give the category-vs-registered-module check an explicit signature that takes the known categories (e.g. ValidateOptions(PipelineOptions options, IReadOnlySet<string> registeredCategories)), which Validate(IServiceProvider) computes and forwards to. That keeps a single source of truth for "is this configuration valid" behind both entry points, rather than two definitions that can drift further apart the next time a services-dependent options check is added.

Nothing else stood out on this pass — the OrdinalIgnoreCase usage is consistent throughout, and the docs update in categories.md accurately describes the new matching/validation behavior.

[thomhurst]

Rebased onto current main after #3564 merged. Patch equivalence preserved (55adf802bcb11d04837fe440f2cdcbe6883f651e). New head: daa2768acd.

@codex review

[chatgpt-codex-connector]

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

Reviewed commit: daa2768acd

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

Code review

Re-reviewed after the rebase onto current main (now at daa2768ac, commits 5b7cfa8 + daa2768a). The diff content is unchanged from the version reviewed at 3e914d95 before the rebase — same case-insensitive matching in ModuleConditionHandler, same ValidateRegisteredCategories addition in OptionsValidator.cs, same test coverage.

The previously flagged design gap is still unresolved: OptionsValidator.ValidateOptions(PipelineOptions) — the public method on the public IOptionsValidator interface — only runs the original checks (retry count, timeouts, RunOnly/Ignore conflicts). The new registered-category / zero-selection check lives exclusively in ValidateRegisteredCategories, which is only reachable via Validate(IServiceProvider).

This still means the two public entry points on IOptionsValidator disagree about what "valid options" means: any consumer that resolves IOptionsValidator and calls ValidateOptions(options) directly — e.g. to validate configuration before wiring up a full host, or from a custom IPipelineValidator composing it — will silently miss exactly the class of bug this PR sets out to catch (a typoed or now-empty RunOnlyCategories/IgnoreCategories), with nothing in the code or XML docs signalling that ValidateOptions is the partial/legacy path.

Suggested fix (unchanged from before, still preferred over just updating the doc comment): give the category-vs-registered-module check an explicit signature that takes the known categories (e.g. ValidateOptions(PipelineOptions options, IReadOnlySet<string> registeredCategories)), which Validate(IServiceProvider) computes and forwards to. That keeps one source of truth behind both entry points instead of two definitions that can drift further apart.

Nothing else new — the rebase didn't introduce any regressions, and everything else (OrdinalIgnoreCase consistency, the ModuleHistoryTests/ResultsRepositoryTests fixes for tests that previously filtered out all modules, docs update) still looks correct.

[thomhurst]

Closed the remaining validation-contract gap: IOptionsValidator now exposes category-aware validation, and the DI path routes through the same implementation. Added direct-consumer regression coverage. ValidationTests pass (37/37); core Release build has 0 errors. The previously fixed P1 thread is now resolved. Branch updated from main. @codex review

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 11d75b4bc9

ℹ️ 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: f62d339fc7

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

Code review

Re-reviewed at f62d339f (merge of 6c1cc8f "fix(validation): normalize category sets" onto current main).

The previously flagged P1 design gap is now resolved. IOptionsValidator now exposes two explicit entry points (IOptionsValidator.cs):

• ValidateOptions(PipelineOptions) — options-only checks, clearly documented as not depending on registered modules.
• ValidateOptions(PipelineOptions, IReadOnlySet<string> registeredCategories) — the full check, including the new unknown-category/zero-selection validation.

Validate(IServiceProvider) now funnels through the same ValidateOptions(options, registeredCategories) overload (OptionsValidator.cs:17-30), so there's a single source of truth for "is this configuration valid" rather than two definitions that could drift — exactly the option (a) fix suggested in earlier rounds, and the right call over just re-documenting the gap away.

The follow-up normalization fix is also correct. ValidateOptions(options, registeredCategories) now builds normalizedRegisteredCategories via new HashSet<string>(registeredCategories, StringComparer.OrdinalIgnoreCase) (OptionsValidator.cs:106-111) before doing any lookups. That closes the gap where a direct caller (e.g. testing IOptionsValidator standalone, as ValidateOptions_WithCaseSensitiveCategorySet_MatchesIgnoringCase now does) could pass an ordinary case-sensitive HashSet<string> and get spurious "unknown category" errors purely because of the caller's comparer choice, not an actual mismatch. Good regression coverage for both a case-sensitive input set and the direct-consumer path (ValidateOptions_WithRegisteredCategories_RejectsUnknownCategory).

Everything else holds up from prior passes:

• ModuleConditionHandler.IsRunnableCategory/IsIgnoreCategory matching via StringComparer.OrdinalIgnoreCase is consistent with the conflict check and with tag/category comparisons elsewhere in the codebase.
• ValidateRunOnlyCategories/ValidateIgnoreCategories correctly distinguish "all categories unmatched → would select zero modules" from "some categories unmatched" with distinct messages.
• GetRegisteredCategories resolving through constructed IModule singletons (rather than a pure type/attribute scan) is the right approach since category can be overridden at registration time (WithCategory(), Configuration.Category, ITaggedModule.Category); FinalizeMetadata is idempotent so calling it again here isn't duplicating work.
• ModuleHistoryTests/ResultsRepositoryTests fixes (adding a runnable module in the filtered category) correctly preserve the original skip/history assertions now that a RunOnlyCategories filter selecting zero modules is a validation error rather than a silent no-op.
• Docs update in categories.md accurately reflects the new matching/validation behavior.

No further issues. This is a solid, well-scoped hardening of category filtering — nice work closing out the review thread across the iterations.

[thomhurst]

@codex review

[chatgpt-codex-connector]

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

Reviewed commit: 212956952a

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

Code review

Re-reviewed at 212956952a1d1ab33cf7562b7555d23dc6adc818. The only new change since the last review round is 9e716250 ("fix(validation): preserve validator compatibility"), which turns the category-aware overload into a default interface method:

ValidationResult ValidateOptions(
    PipelineOptions options,
    IReadOnlySet<string> registeredCategories) => ValidateOptions(options);

This is a good, low-risk fix for a real gap: eb5f9aec added ValidateOptions(PipelineOptions, IReadOnlySet<string>) as a plain abstract interface member on the public IOptionsValidator interface, which would have been a source-breaking change for any existing external implementation that only had the single-arg ValidateOptions(PipelineOptions). Giving it a default body that forwards to the single-arg overload means:

• Existing third-party IOptionsValidator implementations keep compiling without modification.
• New/updated implementations can still opt in to the full category-aware check by overriding the two-arg overload.
• The framework's own OptionsValidator still explicitly overrides both methods, so Validate(IServiceProvider) continues to funnel through the full ValidateOptions(options, registeredCategories) path — the "single source of truth" fix from the previous review round (f62d339f) is unaffected by this change.
• net10.0 is the only TFM for this project, so default interface methods are fully supported at runtime — no netstandard2.0 concern.

Test coverage (CategoryAwareOverloadUsesLegacyValidatorImplementationByDefault) directly exercises the fallback behavior with a minimal LegacyOptionsValidator that only implements the single-arg overload, confirming the default correctly delegates.

No bugs or CLAUDE.md violations found in this delta. Combined with the prior rounds (case-insensitive category matching, zero-selection/unknown-category validation, normalization of caller-supplied comparers, and the dual-entrypoint validator split), this PR looks solid and ready to merge.