Remove the redundant module registration handle

[thomhurst]

Summary

• return PipelineBuilder directly from all AddModule overloads
• remove ModuleRegistration and its duplicate integration overload
• simplify analyzer/source-generator registration recognition and update API tests

Validation

• ModularPipelines.slnx Release build
• ModularPipelines.GitHub.csproj Release build
• PipelineBuilderRegistrationTests (16/16)
• ModuleRegistration export test (1/1)
• source-generator chained builder test (1/1)
• targeted dotnet format

Closes #3765

[thomhurst]

Merged current main and resolved its ModuleRegistration documentation edit by retaining this PR's intentional type deletion. Focused tests cannot currently rebuild on main because RunReportTests still initializes removed PipelineOptions.PrintLogo/PrintResults properties; pre-merge #3765 validation remains green.

[claude]

Code review

Reviewed the diff, the linked issue (#3765), and cross-checked the codebase for leftover references.

This is a clean, well-scoped simplification — no issues found.

Highlights:

• Deleting ModuleRegistration<TModule> and returning PipelineBuilder directly from all AddModule overloads is exactly the fix the linked issue recommended (option b). The old handle added a parallel object graph that had to be hand-mirrored for every PipelineBuilder API (~25 forwarders) and forced integration packages like ModularPipelines.GitHub to duplicate every extension method for both receiver types — a real "every new builder API now costs three declarations" tax that's now gone.
• TModule on the old type wasn't doing any real work — it only typed further chained calls and an implicit conversion back to PipelineBuilder; it never enabled per-module configuration (e.g. no .WithRetry()/.DependsOn<T>()). So this isn't a loss of type safety, just removal of a type parameter that was decorative.
• Removing the WriteDistributedWorkflow<TModule>(this ModuleRegistration<TModule> ...) overload in GitHubExtensions.cs is safe and complete — it was a pure passthrough (registration.Builder.WriteDistributedWorkflow(options)), and since AddModule now returns PipelineBuilder natively, the remaining non-generic WriteDistributedWorkflow(this PipelineBuilder ...) overload covers the same chaining scenario without needing a second signature.
• The analyzer (ModuleAuthoringAnalysis.IsModuleRegistrationMethod/IsDirectServiceRegistrationMethod) and source generator (ModuleMetadataGenerator.IsModuleRegistrationMethod) special-casing for the old type is cleanly removed alongside it, so there's no dead recognition logic left to fall out of sync with the API surface going forward.
• Confirmed no dangling references remain anywhere in src/, test/, or docs/ (ModuleRegistration< has zero matches outside this diff; the unrelated ModuleRegistrationAnalyzer/ModuleRegistrationContext types are a different, still-used concept and untouched).
• Updated tests (PipelineBuilderRegistrationTests, FlexibleDependencyApiExportTests, ModuleMetadataGeneratorTests) match the new shape and assert the type is gone from the compiled assembly rather than just unused, which is a stronger guarantee than what was there before.

No architectural concerns — if anything, this reduces the codebase's abstraction count and removes a maintenance trap, in line with the project's stated preference for avoiding duplication and premature abstraction.

[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

[thomhurst]

Correction: GitHub could not refresh this branch because rebase conflicts with current main. The preceding refresh/review comments were posted by a chained command after that failure; no new head or CI cycle exists yet. Resolving the conflict explicitly now.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ad9f1c1b20

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

Keep recognizing root AddModule calls

With AddModule now returning PipelineBuilder, chained calls in real consumers bind to the public ModularPipelines.PipelineBuilderExtensions type, but this predicate only accepts PipelineBuilderExtensionsFullName, which is ModularPipelines.Extensions.PipelineBuilderExtensions. That means a normal chain such as builder.AddModule<Start>().AddModule<GenericModule<string>>() no longer produces closed-generic module metadata or module event metadata; before this change the second call bound to ModuleRegistration<T>.AddModule and was accepted by the removed branch.

Useful? React with 👍 / 👎.

[thomhurst]

Fixed in 5a6eb66. The generator now recognizes the actual public ModularPipelines.PipelineBuilderExtensions type. All registration fixtures were corrected to mirror production, including the chained builder regression. Exact chained test 1/1, full ModuleMetadataGeneratorTests 24/24, core Release build 0 warnings/errors.

[thomhurst]

Rebased onto current main and resolved the sole modify/delete conflict by keeping ModuleRegistration deleted, matching this PR's API-removal intent. Validation: core Release build 0 warnings/errors; PipelineBuilderRegistrationTests 16/16; FlexibleDependencyApiExportTests 13/13; ModuleMetadataGeneratorTests 24/24; GitHub solution Release build 0 warnings/errors; scoped whitespace and diff checks clean. Current head: 3823c2a.

[thomhurst]

@codex review

[thomhurst]

@claude review

[chatgpt-codex-connector]

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

Reviewed commit: 3823c2a205

ℹ️ 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-checked after the rebase onto current `main` (head `3823c2a2`). The rebase only resolved a modify/delete conflict by keeping this PR's deletion of `ModuleRegistration` — the PR's own diff is otherwise unchanged from the version already reviewed.

No issues found. Confirmed:

• No leftover references to the deleted `ModuleRegistration` type anywhere in the codebase (all remaining ModuleRegistration* hits are unrelated types like ModuleRegistrationAnalyzer, IModuleRegistrationEventReceiver, GeneratedModuleRegistration, ModuleRegistrationContext).
• Analyzer and source-generator updates (ModuleAuthoringAnalysis.cs, ModuleMetadataGenerator.cs) consistently drop the now-dead IsModuleRegistrationType special-casing.
• AddModule overloads in PipelineBuilderExtensions.cs now return PipelineBuilder directly — no type-safety loss since ModuleRegistration<TModule> only ever forwarded to PipelineBuilder.
• Tests were updated to match (PipelineBuilderRegistrationTests, FlexibleDependencyApiExportTests, ModuleMetadataGeneratorTests).

Clean, well-scoped simplification. Still good to merge.

[thomhurst]

Refreshed onto current main after Ubuntu's core suite ran 15m40s and exited 2/134 without a failing-test summary. Validation on b106643: PipelineBuilderRegistrationTests 16/16, FlexibleDependencyApiExportTests 13/13, targeted ModuleMetadataGenerator test 1/1. The guarded core build reached the repository's 2 GB process-tree limit (2,103 MB), so I did not raise limits or retry; CI will perform that expensive validation.

[claude]

Code review

Re-checked at the current head (b106643c4). Since my last review (head 3823c2a2), the only activity has been rebasing onto current main — the PR's actual diff is byte-for-byte the same simplification already reviewed: ModuleRegistration<TModule> removed, AddModule overloads return PipelineBuilder directly, and the analyzer/source-generator/test updates match.

No new issues. Still a clean, well-scoped simplification that removes a decorative generic wrapper and the maintenance tax it imposed on integration packages — good to merge.

[thomhurst]

Review fix pushed in 5a6eb660ac.

@claude review
@codex review

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 5a6eb660ac

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

Recognize the event-metadata registration fixture

Fresh evidence after the fix is that ModuleEventMetadataGenerator.GetRegisteredModuleCandidate still delegates through this predicate, while the event-metadata test infrastructure declares AddModule on ModularPipelines.Extensions.PipelineBuilderExtensions. With this equality those registered closed-generic modules no longer produce event metadata, so the registered-generic event tests get no generated tree; either update that fixture to the public namespace or continue accepting the legacy containing type here.

Useful? React with 👍 / 👎.

[claude]

Code review

Re-checked at the current head (5a6eb660a, "fix: recognize public module registrations"). Since my last review, one substantive change landed in response to the codex-flagged regression on ModuleMetadataGenerator.cs:285.

The fix is correct and complete.

• The regression was real: PipelineBuilderExtensionsFullName was set to "ModularPipelines.Extensions.PipelineBuilderExtensions", but the actual production type lives at namespace ModularPipelines { public static class PipelineBuilderExtensions } (src/ModularPipelines/Extensions/PipelineBuilderExtensions.cs:13). I confirmed this directly against the file. With the old (wrong) namespace and ModuleRegistration<T> deleted, chained calls like builder.AddModule<Start>().AddModule<Generic<T>>() would silently stop producing closed-generic module/event metadata for the second call — a real functional regression, not just a cosmetic one.
• The fix updates the constant to the correct "ModularPipelines.PipelineBuilderExtensions" and reworks all seven affected fixtures in ModuleMetadataGeneratorTests.cs to declare PipelineBuilderExtensions inside namespace ModularPipelines (matching production) instead of the fictitious ModularPipelines.Extensions namespace, including consumers switching to using ModularPipelines;. This makes the test fixtures representative of the real shape instead of a namespace that never existed in the shipped API.
• I checked the analyzer side too (ModuleAuthoringAnalysis.IsModuleRegistrationMethod) — it already accepted both "ModularPipelines.PipelineBuilderExtensions" and "ModularPipelines.Extensions.PipelineBuilderExtensions", so it wasn't affected by this bug; only the source generator's single-namespace constant was wrong.
• I built ModularPipelines.Tests.slnf (core + ModularPipelines.UnitTests) in Release from a worktree at this head: 0 errors, 79 pre-existing nullability warnings unrelated to this change.
• Confirmed (again) no leftover references to the deleted ModuleRegistration<TModule> type anywhere in src/, test/, or docs/ — remaining ModuleRegistration* hits are unrelated, still-used types (ModuleRegistrationAnalyzer, IModuleRegistrationEventReceiver, ModuleRegistrationContext, GeneratedModuleRegistration).

No new issues. This is a clean, well-scoped simplification that now also correctly preserves chained-registration metadata generation. Good to merge.