refactor: Use interface abstraction for IModuleResultRepository enabled check

[thomhurst]

Summary

Fixes #1526

• Add IsEnabled property to IModuleResultRepository interface to indicate whether the repository is configured for actual storage
• Set IsEnabled => false in NoOpModuleResultRepository (the default no-op implementation)
• Set IsEnabled => true in test implementations
• Update ModuleExecutionPipeline.cs to use _resultRepository.IsEnabled instead of concrete type checks against NoOpModuleResultRepository

This change improves testability and follows the Dependency Inversion Principle by removing direct dependencies on concrete types.

Test plan

• Verify build compiles successfully
• Verify existing tests pass (ResultsRepositoryTests, ModuleHistoryTests)
• Verify no regressions in module history/skip functionality

🤖 Generated with Claude Code

[thomhurst]

Summary

Refactors module result repository type checking from runtime type comparison to a polymorphic IsEnabled property.

Critical Issues

None found ✅

Suggestions

Documentation Update Needed
The documentation at docs/docs/how-to/storing-and-retrieving-results.md should be updated to include the IsEnabled property in the example implementation:

public class MyModuleRepository : IModuleResultRepository
{
    public bool IsEnabled => true;
    
    // ... rest of implementation
}

This helps users understand that the interface now requires this property when implementing custom repositories.

Verdict

✅ APPROVE - No critical issues

The refactoring improves the design by:

• Replacing fragile runtime type checking (GetType() == typeof(NoOpModuleResultRepository)) with polymorphic behavior
• Making the enable/disable intent explicit through the interface
• Following better OOP principles

All test repositories correctly implement the new property. The only minor improvement would be updating the documentation example.

[Copilot]

Pull request overview

This PR refactors the module result repository pattern to use interface-based abstraction instead of concrete type checks, addressing issue #1526. The change improves testability and follows the Dependency Inversion Principle by removing direct dependencies on the NoOpModuleResultRepository concrete type.

Key changes:

• Added IsEnabled property to the IModuleResultRepository interface to indicate whether the repository performs actual storage operations
• Replaced type checks (GetType() == typeof(NoOpModuleResultRepository)) with property-based checks (IsEnabled) in ModuleExecutionPipeline
• Updated all implementations to provide appropriate IsEnabled values

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
src/ModularPipelines/Engine/IModuleResultRepository.cs	Added IsEnabled property with comprehensive XML documentation to the interface
src/ModularPipelines/Engine/NoOpModuleResultRepository.cs	Implemented IsEnabled => false for the no-op repository implementation
src/ModularPipelines/Engine/ModuleExecutionPipeline.cs	Replaced concrete type checks with IsEnabled property checks in two locations (historical result retrieval and result saving)
test/ModularPipelines.UnitTests/ResultsRepositoryTests.cs	Implemented IsEnabled => true in test repository mock
test/ModularPipelines.UnitTests/ModuleHistoryTests.cs	Implemented IsEnabled => true in both test repository mocks

---

Review Summary: This is a clean, well-documented refactoring that successfully eliminates the code smell identified in issue #1526. The implementation is consistent across all affected files, maintains backward compatibility (adds a new property without breaking existing implementations), and follows SOLID principles. The XML documentation for the new property is clear and comprehensive. No issues found.