Classify missing modules by exception type

[thomhurst]

Closes #3771

Summary

• add an internal NoModulesRegisteredException
• throw it from both empty-module discovery paths
• catch the dedicated type during builder validation instead of matching exception text
• prove an unrelated PipelineException containing “No modules” still propagates

Validation

• ValidationTests: 40/40 passed
• ModularPipelines.slnx Release build: 0 warnings, 0 errors
• targeted formatting and git diff --check passed

The unit-test project currently needs the pending stale PipelineOptions.Console correction from #3864 to compile; that unrelated temporary local adjustment was removed and is not part of this PR.

[thomhurst]

@claude review

[claude]

Code review

Reviewed the diff (ModuleRetriever.cs, PipelinePlanner.cs, NoModulesRegisteredException.cs, PipelineBuilder.cs, ValidationTests.cs).

No issues found. This is a clean, well-scoped fix:

• Replacing catch (PipelineException ex) when (ex.Message.Contains("No modules")) with a dedicated NoModulesRegisteredException type removes a real anti-pattern (message-string matching as control flow), and the new regression test (ValidateAsync_DoesNotClassifyUnrelatedPipelineExceptionByMessage) directly proves the fragile case this was meant to fix.
• NoModulesRegisteredException being internal sealed with no XML docs is consistent with this repo's existing convention for exceptions used purely as internal control-flow signals (e.g. ModuleSelectionException, ModuleFailedException, ModuleNotInitializedException), as opposed to the public, XML-documented exceptions meant for consumers to catch. Good to see it followed the established pattern rather than introducing a new public API surface unnecessarily.
• Both throw sites (ModuleRetriever.cs, PipelinePlanner.cs) were migrated, and no other code in the repo still does string-matching against this message — the fix is complete, not partial.
• Considered whether the three catch clauses in PipelineBuilder.BuildAndValidatePipelineAsync should be merged, or whether the "no modules" check should return a Result instead of throwing, or whether the duplicated throw across the two call sites should be shared. In each case the current shape holds up: the catch clauses build genuinely different ValidationResults (different category + message), the throw/catch mechanism is consistent with how ModuleNotRegisteredException/DependencyCollisionException/etc. are already handled in the same method rather than a new pattern, and the two throw sites are different call paths (validation-time retrieval vs. execution-time planning) where sharing would add coupling to save one line each.

Nice, tightly-scoped fix for #3771.

[thomhurst]

CI diagnosis: Ubuntu, Windows, and macOS all fail on the same unrelated mainline compile error in RunReportTests.cs:1483-1484 (PipelineOptions.PrintLogo / PrintResults no longer exist). Fast fail, analyzers, trim/AOT, CodeQL, and Claude review pass. PR #3864 contains the exact shared correction and is currently waiting on its last CodeQL job; no duplicate patch or rerun here.

[thomhurst]

The shared RunReportTests regression is fixed on current main by merged PR #3844. Refreshed this branch onto current main for clean CI and review.

[thomhurst]

@codex review

[thomhurst]

@claude review

[chatgpt-codex-connector]

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

Reviewed commit: be133fe8fc

ℹ️ 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 — the diff (ModuleRetriever.cs, PipelinePlanner.cs, NoModulesRegisteredException.cs, PipelineBuilder.cs, ValidationTests.cs) is byte-for-byte identical to what was reviewed in the previous pass; only the base commit moved.

No issues found. Confirming the prior findings still hold:

• The catch (PipelineException ex) when (ex.Message.Contains("No modules")) string-matching anti-pattern is now a dedicated NoModulesRegisteredException, caught by type in PipelineBuilder.BuildAndValidatePipelineAsync.
• Both throw sites (ModuleRetriever.cs, PipelinePlanner.cs) were migrated; a repo-wide check confirms no other code still does message-text matching against "No modules" (the one other literal, in ModuleConfigurationValidator.cs, is an unrelated pre-existing validator message, not exception matching).
• NoModulesRegisteredException being internal sealed with no XML docs matches this repo's convention for exceptions used purely as internal control-flow signals (e.g. ModuleSelectionException, ModuleFailedException).
• The new ValidateAsync_DoesNotClassifyUnrelatedPipelineExceptionByMessage test directly proves the fragile case this PR fixes (an unrelated PipelineException containing "No modules" is no longer misclassified).

Nice, tightly-scoped fix for #3771.

[thomhurst]

Refreshed onto current main after Ubuntu's core test host ran 15m22s and exited without a failing-test summary; all other jobs passed. Validation on bb2d9c8: ValidationTests 40/40 and ModularPipelines.slnx Release build 0 warnings/errors. A detached worktree preserved the existing divergent local branch untouched.

[claude]

Code review

No new review needed — the PR's actual diff (ModuleRetriever.cs, PipelinePlanner.cs, NoModulesRegisteredException.cs, PipelineBuilder.cs, ValidationTests.cs) is unchanged since the last review. The one intervening commit (bb2d9c8e2) only merged in unrelated upstream changes (Directory.Packages.props, docs/docs/_sidebar.md, FileSystemModuleCache.cs/tests) and did not touch this PR's code.

Standing by the previous review: no issues found, clean and well-scoped fix.