Hide module engine plumbing from public API

[thomhurst]

Closes #3793

Summary

• reduce public IModule to ResultType and Configuration
• move result awaiting/application onto internal IInternalModule
• replace ModuleRunType checks with ModuleConfiguration.AlwaysRun and remove the dead enum
• add public API surface regression coverage

Validation

• ModularPipelines.slnx Release build (0 warnings, 0 errors)
• ModularPipelines.Testing.slnx Release build (0 warnings, 0 errors)
• module API tests (2 passed)
• AlwaysRun handler tests (8 passed)
• distributed completion-source tests (2 passed)

[thomhurst]

@codex review

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

[claude]

Review

Solid, well-scoped refactor — IModule now exposes only ResultType/Configuration, the engine plumbing (ResultTask, TrySetDistributedResult) moved to an internal IInternalModule, and the dead ModuleRunType enum was replaced by a plain Configuration.AlwaysRun check everywhere it was used. I diffed every remaining reference to ResultTask/TrySetDistributedResult/ModuleRunType across the whole repo (not just the touched files) and confirmed the migration is complete — no stragglers in the untouched tool packages. The new ModuleApiSurfaceTests is a good regression guard for the public surface itself.

One architectural concern worth addressing before merge:

The public registration API still advertises a contract this change breaks

IModule is not just an internal marker — it's a real, reachable public extension point:

• PipelineBuilderExtensions.AddModule<TModule>() is constrained to where TModule : class, IModule (not Module<T>), src/ModularPipelines/Extensions/PipelineBuilderExtensions.cs:29.
• Assembly auto-discovery (ServiceCollectionExtensions.AddModulesFromAssembly) and ModuleAutoRegistrar.IsValidModuleType both register any concrete, non-abstract type that is IsAssignableTo(typeof(IModule)) — again, not Module<T> specifically.

Before this PR, a type that implemented IModule directly (skipping Module<T>) had to provide real ResultTask/TrySetDistributedResult members itself, which the engine could call polymorphically through the IModule interface — awkward, but functional. After this PR, those members live only on internal interface IInternalModule, and every engine touchpoint (ModuleCacheResultRepository.cs:23, DistributedModuleExecutor.cs:49, WorkerModuleExecutor.cs:84, ModuleCompletionSourceApplicator.cs:72) reaches them via a hard (IInternalModule) module cast. Since IInternalModule is internal, no type outside this repo's InternalsVisibleTo list can ever implement it — so any consumer who (legally, per the public generic constraints) implements IModule directly instead of deriving from Module<T>/SyncModule<T> will now get an InvalidCastException the instant the engine touches their module, rather than a clear error. It's a real regression in that specific (if rare) path: previously-possible custom IModule implementations are now guaranteed to crash, and there's nothing in the public API or docs (docs/docs/how-to/defining-modules.md only documents Module<T>/SyncModule<T>) that signals this.

Two ways to close the gap, either is better than leaving a silent InvalidCastException landmine:

1. Tighten the public contract to match reality. Change the generic constraints on AddModule<TModule>(), DependsOnAttribute, IModuleRegistrationContext, etc. from IModule to Module<T>/a shared non-generic ModuleBase that itself implements IInternalModule. This makes the compiler enforce what's already true at runtime, instead of the type system promising something the engine can't deliver.
2. If arbitrary IModule implementations must stay supported, validate at registration time (AddModulesFromAssembly/ModuleAutoRegistrar/AddModule<TModule>()) that the type also satisfies IInternalModule, and throw a descriptive InvalidOperationException ("modules must derive from Module") instead of deferring to a cryptic cast failure deep inside distributed/caching code.

Given Module<T>/SyncModule<T> are the only base types the docs ever mention, option 1 is probably the cleaner fix and is a natural extension of what this PR is already trying to do (make the real contract explicit).

Minor

(IInternalModule) x casts are duplicated across 5 files (ModuleCacheResultRepository, DistributedModuleExecutor, WorkerModuleExecutor, ModuleCompletionSourceApplicator, ModuleTester). Not blocking, but a small internal static IInternalModule AsInternal(this IModule module) extension would remove the repeated cast syntax and give you one place to add the guard from option 2 above if you go that route.

No prior reviewer comments to reconcile against (the earlier @codex review request hit a usage-limit error with no actual feedback).

[thomhurst]

Addressed the architectural review concern in c8472cf: direct IModule implementations now fail immediately with descriptive Module/SyncModule guidance across generic, instance, factory, runtime-type, assembly-scan, and auto-registration paths. Internal execution access also uses one guarded contract instead of raw casts. Validation: ModuleApiSurfaceTests 3/3, PipelineBuilderRegistrationTests 14/14, ModuleCompletionSourceApplicatorTests 2/2; core and ModularPipelines.Testing Release builds both clean with 0 warnings/errors.

[claude]

Review

Follow-up on the earlier review comment: the architectural gap I flagged — direct IModule implementations bypassing Module<T>/SyncModule<T> would previously hit a silent InvalidCastException deep in engine code — is fixed in c8472cf, and the fix is done well.

What changed and why it closes the gap:

• ModuleExecutionContract (new, internal) centralizes both directions of the contract: Validate(Type) throws a descriptive InvalidOperationException ("must derive from Module or SyncModule...") and AsInternal(this IModule) replaces the five duplicated (IInternalModule) casts with one guarded extension — this also resolves the "Minor" note from the earlier review about duplicated cast syntax.
• Validate is now called at every registration entry point: ServiceCollectionExtensions.AddModule<TModule>() (generic/instance/factory/type overloads), AddModulesFromAssembly (validates every discovered type, generated-metadata or reflection-scanned, before registering any of them), PipelineBuilderExtensions.AddModules(Type[]), and ModuleAutoRegistrar.IsValidModuleType (auto-registered transitive dependencies). That's every path that can put an IModule into the container, so the check can't be bypassed by picking a different registration API.
• This is option 2 from my original comment (validate at registration, fail fast with guidance) rather than option 1 (tightening generic constraints to Module<T>). Given AddModule<TModule>() etc. are public API with where TModule : class, IModule constraints that third parties may already compile against, changing those constraints would be a more disruptive breaking change than this PR already makes — validating at the boundary is the pragmatic choice here and gets you the same outcome (fail before the engine ever touches the module) without the wider API churn.
• ModuleApiSurfaceTests.DirectIModuleImplementationsFailAtRegistrationWithGuidance exercises all four registration overloads plus the direct AsInternal() execution path with a DirectModule : IModule test double, and asserts the same descriptive message everywhere. That's good coverage — it pins down the contract at the exact boundary where it matters instead of just testing one call site.

One small gap, not blocking: AddModulesFromAssembly/AddModulesFromAssemblyContainingType (assembly-scan registration) go through the same Validate loop in the source but aren't hit by the new test — only the four paths in DirectIModuleImplementationsFailAtRegistrationWithGuidance are. Worth a follow-up test asserting a DirectModule in a scanned assembly also throws with the same message, since that's the path most likely to regress silently (e.g., if someone swaps the loop order relative to DependencyGraphValidator.ValidateNoCycles).

Everything else from the original review still holds — the IModule → ResultType/Configuration narrowing, the ModuleRunType → Configuration.AlwaysRun migration, and the rest of the plumbing move are unchanged and were already verified clean. Nothing new to flag there.

Note: CodeFactor is currently reporting 7 issues on this PR (link in the checks tab) — worth a quick look before merge, though that's a separate lint pass and out of scope for this architectural review.

[thomhurst]

Cleared CodeFactor’s seven SA1508 findings in d2f5c76; these were stray blank lines left in three direct-IModule test fakes after removing engine members. Targeted formatter passes; change is whitespace-only.

[claude]

Review (follow-up)

Re-checked the PR after the latest commit (d2f5c76). Since my last review, that commit only removes stray blank lines in three test files (SA1508 fixes) — whitespace-only, confirmed via the commit diff, no code paths touched. The architectural gap I flagged earlier (direct IModule implementations bypassing Module<T>/SyncModule<T>) remains fixed as described, and the outstanding CodeFactor findings from my previous comment are now cleared.

Re-verified the full diff end-to-end (all 3 commits) with fresh eyes:

• IModule → ResultType/Configuration narrowing and the IInternalModule/ModuleExecutionContract.AsInternal() split are applied consistently — every former (IModule)/(IInternalModule) cast site now goes through the single AsInternal() extension, and ModuleExecutionContract.Validate is wired into all four AddModule overloads, AddModulesFromAssembly, PipelineBuilderExtensions.ValidateModuleType, and ModuleAutoRegistrar.IsValidModuleType. I couldn't find a registration path that bypasses it.
• ModuleRunType removal is a clean, fully-swept breaking change (Configuration.AlwaysRun replaces every call site); the only remaining references are in versioned migration docs describing the previous v2→v3 migration, which is correct historical content, not live guidance.
• SyncModule<T> : Module<T> still flows through IInternalModule transitively, so no gap there.

One architectural point worth a quick gut-check (not blocking): ModuleAutoRegistrar.IsValidModuleType and the AddModulesFromAssembly loop now call ModuleExecutionContract.Validate on every concrete IModule-assignable type discovered by a broad scan (assembly.GetTypes() in the reflection-fallback path, or transitively-referenced [DependsOn<T>] types in the auto-registrar). Previously this scan silently filtered out anything that wasn't a "real" module candidate; now it throws for any incidental direct-IModule type it encounters in the scanned assembly, even one never meant to be registered or run (e.g. a test double or mock sitting in the same assembly that also happens to implement IModule). That's arguably fine — the PR's stance is that direct IModule implementation is unsupported, period — but it does change AddModulesFromAssembly from "best-effort discovery, skip what doesn't fit" to "all-or-nothing, fail the whole scan on one unrelated type." Worth being deliberate about which behavior you want for the bulk-scan APIs specifically, since that's a different blast radius than the explicit single-type AddModule<T>() calls. Not something I'd hold up merge for — flagging for awareness.

The non-blocking test-coverage suggestion from my last comment (assembly-scan path not exercised by DirectIModuleImplementationsFailAtRegistrationWithGuidance) is still open but remains a nice-to-have, not a gap in the shipped guarantee.

Nothing else to add — the rest of the plumbing move (public API narrowing, ModuleConfiguration.AlwaysRun migration) is unchanged since my last pass and was already verified clean.

[thomhurst]

Addressed the final review gut-check in f0def1c: bulk assembly scans are deliberately all-or-nothing for unsupported direct IModule implementations, the public scan APIs now document that exception contract, and the API regression exercises the scan path. Post-main validation: focused test 1/1; core and Testing Release solutions both 0 warnings/errors.

[claude]

Review (re-check after latest commits)

Re-reviewed the full current state of the PR (through feb086e, the merge of main). This is a solid, well-scoped refactor: IModule now only exposes ResultType/Configuration, engine plumbing lives on internal IInternalModule, and ModuleRunType is gone in favor of Configuration.AlwaysRun everywhere.

Both previously-flagged issues are resolved:

1. Silent InvalidCastException for direct IModule implementations (flagged in the first review pass) — fixed via ModuleExecutionContract (src/ModularPipelines/Modules/ModuleExecutionContract.cs). Validate(Type) now runs at every registration entry point I could find: all four AddModule overloads and the factory/instance variants in ServiceCollectionExtensions.cs, AddModulesFromAssembly's reflection-fallback loop, and ModuleAutoRegistrar.IsValidModuleType. Every former raw (IModule)/(IInternalModule) cast is replaced by the single AsInternal() extension (ModuleTester.cs, ModuleCacheResultRepository.cs, DistributedModuleExecutor.cs, WorkerModuleExecutor.cs, ModuleCompletionSourceApplicator.cs), so there's exactly one place that owns the guard now instead of five duplicated casts. Good fix — turns a deferred runtime crash into an immediate, descriptive InvalidOperationException at the point of registration.
2. Bulk assembly-scan blast radius (the "all-or-nothing" gut-check from the second review pass) — now explicitly documented via <exception> XML doc tags on AddModulesFromAssembly/AddModulesFromAssemblyContainingType in both PipelineBuilderExtensions.cs and ModuleRegistration.cs, and exercised directly by the new ModuleApiSurfaceTests.DirectIModuleImplementationsFailAtRegistrationWithGuidance test, which asserts the guard fires from the generic, instance, factory, runtime-type, and assembly-scan registration paths, plus AsInternal() itself — six call sites in one test. That closes the "nice-to-have" test-coverage gap noted previously.

I also swept the whole repo (not just the touched files) for stray ResultTask/TrySetDistributedResult/ModuleRunType references outside this diff and found none — the migration is complete, and the only remaining ModuleRunType mentions are in versioned migration docs describing prior releases, which is correct as historical content.

Nothing further to add — this is ready from my side. Nice iteration across the review rounds.

[thomhurst]

Refreshed current main after #3846 changed mergeability. Merge was conflict-free. Validation: ModuleApiSurfaceTests 3/3, PipelineBuilderRegistrationTests 14/14, ModuleCompletionSourceApplicatorTests 2/2; core and ModularPipelines.Testing Release builds both 0 warnings/errors. Current head 0fc64f7.

[thomhurst]

@claude review

[claude]

Review (re-check after merge refresh)

Re-checked at current head 0fc64f7d03. The only commit since my last full review (through feb086e2) is 0fc64f7d03, a merge of origin/main into this branch. I diffed feb086e2 → 0fc64f7d03 restricted to the 32 files this PR actually touches and confirmed it is empty — the merge pulled in unrelated upstream changes (generator/test-fixture files elsewhere in the repo) but did not modify any line of this PR's own diff. That matches the author's note that the merge was conflict-free.

Since the PR's actual code is unchanged since my last pass, my prior assessment stands:

• IModule → ResultType/Configuration narrowing and the IInternalModule/ModuleExecutionContract split are consistent, with AsInternal() replacing every former raw cast.
• ModuleExecutionContract.Validate is wired into all registration entry points (AddModule overloads, AddModulesFromAssembly, PipelineBuilderExtensions, ModuleAutoRegistrar), so direct IModule implementations fail fast with a descriptive message instead of a deferred InvalidCastException.
• ModuleRunType removal is fully swept; only historical migration docs still reference it, which is correct.
• CodeFactor now reports no issues (the SA1508 findings from an earlier pass are cleared).

No new issues to flag. This is ready from my side.

[thomhurst]

Required pipeline failed from core test timing/process instability; rerunning failed jobs once on the unchanged head.