Strengthen engine descriptors and typed navigation - #129
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ 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".
There was a problem hiding this comment.
Pull request overview
This PR introduces a strongly-typed, transport-agnostic engine architecture for the Bravellian platform. It establishes a manifest-driven discovery system that decouples business logic engines (UI and webhook) from their transport adapters, enabling modules to declare capabilities, security requirements, and navigation hints through typed descriptors. The changes implement generic factories for type-safe engine resolution and add enums for navigation targets and signature algorithms.
Key Changes
- Introduced typed engine descriptor interfaces (
IModuleEngineDescriptor,ModuleEngineDescriptor<TContract>) with factory-based resolution - Added strongly-typed navigation tokens (
ModuleNavigationToken,NavigationTargetKind) and security metadata (ModuleSignatureAlgorithm,ModuleEngineSecurity) - Implemented discovery service and registry for deterministic engine lookup by module, kind, feature area, or webhook provider/event pairs
Reviewed changes
Copilot reviewed 35 out of 35 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
EngineRefactoringTests.cs |
Comprehensive test suite validating UI/webhook engine execution, discovery filtering, and adapter behavior |
UiEngineAdapter.cs |
Adapter mapping UI engine contracts to transport-ready responses with navigation tokens |
UiAdapterResponse.cs |
Response record containing view model, navigation targets, and emitted events |
FullStackModuleServiceCollectionExtensions.cs |
Registers ModuleEngineDiscoveryService for full-stack modules |
WebhookRequest.cs |
Request record passed to webhook engines with provider, event type, and payload |
WebhookOutcomeType.cs |
Enum defining webhook processing outcomes (Acknowledge, Retry, EnqueueEvent) |
WebhookOutcome.cs |
Outcome record with factory methods for webhook engine results |
UiEngineResult.cs |
Result record for UI engines containing view model, navigation, and events |
NavigationTargetKind.cs |
Enum for navigation target types (Route, Dialog, Component, External) |
ModuleSignatureAlgorithm.cs |
Enum for webhook signature algorithms (None, HmacSha256, HmacSha512, RsaSha256) |
ModuleRegistry.cs |
Updated to register engine descriptors during module initialization and validate module key consistency |
ModuleNavigationToken.cs |
Typed navigation token record for engine-to-adapter communication |
ModuleEngineWebhookMetadata.cs |
Webhook event metadata for provider/event type routing |
ModuleEngineSecurity.cs |
Security metadata record for signature validation and idempotency |
ModuleEngineSchema.cs |
Schema hints record for engine input/output types |
ModuleEngineRegistry.cs |
Internal registry for storing and querying engine descriptors by various criteria |
ModuleEngineNavigationHints.cs |
Navigation hints record containing well-known navigation tokens |
ModuleEngineManifest.cs |
Comprehensive manifest record describing engine capabilities, schemas, security, and compatibility |
ModuleEngineDiscoveryService.cs |
Public service for listing, filtering, and resolving engine descriptors and instances |
ModuleEngineDescriptor.cs |
Strongly-typed descriptor record wrapping manifest with factory for engine resolution |
ModuleEngineCompatibility.cs |
Compatibility metadata record for version requirements |
ModuleEngineCapabilities.cs |
Capabilities record declaring supported actions and events |
ModuleEngineAdapterHints.cs |
Adapter hints record for transport-level requirements |
IWebhookEngine.cs |
Generic webhook engine interface for handling typed payloads |
IUiEngine.cs |
Generic UI engine interface for executing commands and producing view models |
IModuleEngineDescriptor.cs |
Base interface for engine descriptors with module key, manifest, and factory |
IEngineModule.cs |
Marker interface for modules exposing engine descriptors |
EngineKind.cs |
Enum distinguishing UI and webhook engines |
BackgroundModuleServiceCollectionExtensions.cs |
Registers ModuleEngineDiscoveryService for background modules |
WebhookEngineAdapter.cs |
Adapter validating signatures, enforcing idempotency, and dispatching to webhook engines |
WebhookAdapterResponse.cs |
Response record mapping webhook outcomes to transport responses |
WebhookAdapterRequest.cs |
Request envelope for webhook adapters with headers, signature, and payload |
IWebhookSignatureValidator.cs |
Interface for webhook signature validation implementations |
ApiModuleServiceCollectionExtensions.cs |
Registers ModuleEngineDiscoveryService for API modules |
engine-overview.md |
Documentation of engine architecture, building blocks, adapter roles, and versioning strategy |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
@copilot Can you resolve the comments on this PR? |
|
@SamuelMcAravey I've opened a new pull request, #130, to work on those changes. Once the pull request is ready, I'll request review from you. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 35 out of 35 changed files in this pull request and generated 14 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@copilot Can you address the comments on this PR? |
|
@SamuelMcAravey I've opened a new pull request, #131, to work on those changes. Once the pull request is ready, I'll request review from you. |
…lidation, improve diagnostics (#131) Co-authored-by: SamuelMcAravey <11021165+SamuelMcAravey@users.noreply.github.com> Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
…port-agnostic-modules-1ziys6
|
@codex open a new pull request to apply changes based on the comments in this thread |
|
@copilot open a new pull request to apply changes based on the comments in this thread |
|
@SamuelMcAravey I've opened a new pull request, #132, to work on those changes. Once the pull request is ready, I'll request review from you. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 35 out of 35 changed files in this pull request and generated 13 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Summary
Testing
|
…132) Co-authored-by: SamuelMcAravey <11021165+SamuelMcAravey@users.noreply.github.com> Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…on-for-transport-agnostic-modules-1ziys6' into codex/2025-12-21-refactor-solution-for-transport-agnostic-modules-1ziys6
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 35 out of 35 changed files in this pull request and generated 15 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 38 out of 38 changed files in this pull request and generated 10 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public object ResolveEngine(IModuleEngineDescriptor descriptor, IServiceProvider serviceProvider) | ||
| { | ||
| if (serviceProvider is null) | ||
| { | ||
| throw new ArgumentNullException(nameof(serviceProvider)); | ||
| } | ||
|
|
||
| var instance = descriptor.Create(serviceProvider); | ||
|
|
||
| if (instance is null) | ||
| { | ||
| throw new InvalidOperationException( | ||
| $"The factory for module engine '{descriptor.ModuleKey}/{descriptor.Manifest.Id}' returned null."); | ||
| } | ||
|
|
||
| return instance; | ||
| } |
There was a problem hiding this comment.
Missing validation for the descriptor parameter. The overloaded ResolveEngine method validates serviceProvider for null but doesn't validate the descriptor parameter itself. If descriptor is null, the code will throw a NullReferenceException on line 79 when accessing descriptor.Create. Add a null check for the descriptor parameter with an appropriate ArgumentNullException.
| foreach (var service in requiredServices) | ||
| { | ||
| if (string.IsNullOrWhiteSpace(service)) | ||
| { | ||
| throw new InvalidOperationException( | ||
| $"Engine '{descriptor.ModuleKey}/{descriptor.Manifest.Id}' declares an empty required service identifier."); | ||
| } | ||
| } | ||
|
|
||
| var validator = services.GetService<IRequiredServiceValidator>(); | ||
| if (validator is null) | ||
| { | ||
| throw new InvalidOperationException( | ||
| $"Engine '{descriptor.ModuleKey}/{descriptor.Manifest.Id}' declares required services but no {nameof(IRequiredServiceValidator)} is registered."); | ||
| } | ||
|
|
||
| var missing = validator.GetMissingServices(requiredServices.ToArray()) ?? Array.Empty<string>(); | ||
| if (missing.Count > 0) | ||
| { | ||
| throw new InvalidOperationException( | ||
| $"Engine '{descriptor.ModuleKey}/{descriptor.Manifest.Id}' is missing required services: {string.Join(", ", missing)}."); | ||
| } | ||
| } |
There was a problem hiding this comment.
The ValidateRequiredServices method has code duplication with the UiEngineAdapter's ValidateRequiredServices method. Lines 129-150 in WebhookEngineAdapter are nearly identical to lines 78-99 in UiEngineAdapter. The only difference is the WebhookEngineAdapter has additional logic to aggregate required services from webhook metadata. Consider extracting the common validation logic into a shared helper method to improve maintainability and reduce duplication.
| /// </summary> | ||
| internal static class ModuleEngineRegistry | ||
| { | ||
| private static readonly ConcurrentDictionary<string, List<IModuleEngineDescriptor>> Engines = new(StringComparer.OrdinalIgnoreCase); |
There was a problem hiding this comment.
The ConcurrentDictionary is being used with an external lock, which negates the benefits of using a concurrent collection. Since all access to the Engines dictionary is protected by RegistryLock, consider using a regular Dictionary instead. ConcurrentDictionary adds overhead that isn't needed when external synchronization is already in place.
| private static void ValidateWebhookMetadataUniqueness(IModuleEngineDescriptor descriptor) | ||
| { | ||
| var metadata = descriptor.Manifest.WebhookMetadata; | ||
| if (metadata is null) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| var seen = new HashSet<(string Provider, string EventType)>(new WebhookMetadataKeyComparer()); | ||
|
|
||
| foreach (var entry in metadata) | ||
| { | ||
| if (!seen.Add((entry.Provider, entry.EventType))) | ||
| { | ||
| throw new InvalidOperationException( | ||
| $"Engine '{descriptor.ModuleKey}/{descriptor.Manifest.Id}' declares duplicate webhook metadata for provider '{entry.Provider}' and event '{entry.EventType}'."); | ||
| } | ||
|
|
||
| foreach (var existingList in Engines.Values) | ||
| { | ||
| foreach (var existing in existingList) | ||
| { | ||
| if (existing.Manifest.WebhookMetadata is null) | ||
| { | ||
| continue; | ||
| } | ||
|
|
||
| foreach (var existingEntry in existing.Manifest.WebhookMetadata) | ||
| { | ||
| if (string.Equals(existingEntry.Provider, entry.Provider, StringComparison.OrdinalIgnoreCase) | ||
| && string.Equals(existingEntry.EventType, entry.EventType, StringComparison.OrdinalIgnoreCase)) | ||
| { | ||
| throw new InvalidOperationException( | ||
| $"Webhook provider '{entry.Provider}' and event '{entry.EventType}' are already handled by engine '{existing.ModuleKey}/{existing.Manifest.Id}'."); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The validation logic iterates through Engines.Values while inside a lock that's meant to protect Engines. However, ValidateWebhookMetadataUniqueness is called within the Register method before the new descriptor is added to the list. This means the cross-engine validation checks existing engines but not the newly registered ones in the same batch. If multiple descriptors with the same webhook metadata are passed in the same descriptors enumerable, only the first will be caught by the local HashSet check, but duplicates later in the same batch won't be detected until they're individually validated.
| public TContract ResolveEngine<TContract>(ModuleEngineDescriptor<TContract> descriptor, IServiceProvider serviceProvider) | ||
| where TContract : notnull | ||
| { | ||
| if (serviceProvider is null) | ||
| { | ||
| throw new ArgumentNullException(nameof(serviceProvider)); | ||
| } | ||
|
|
||
| var instance = descriptor.Factory(serviceProvider); | ||
|
|
||
| if (instance is null) | ||
| { | ||
| throw new System.InvalidOperationException( | ||
| $"The factory for module engine '{descriptor.ModuleKey}/{descriptor.Manifest.Id}' returned null."); | ||
| } | ||
|
|
||
| return instance; | ||
| } |
There was a problem hiding this comment.
Missing validation for the descriptor parameter. The method validates serviceProvider for null but doesn't validate the descriptor parameter itself. If descriptor is null, the code will throw a NullReferenceException on line 59 when accessing descriptor.Factory. Add a null check for the descriptor parameter with an appropriate ArgumentNullException.
| foreach (var service in requiredServices) | ||
| { | ||
| if (string.IsNullOrWhiteSpace(service)) | ||
| { | ||
| throw new InvalidOperationException( | ||
| $"Engine '{descriptor.ModuleKey}/{descriptor.Manifest.Id}' declares an empty required service identifier."); | ||
| } | ||
| } |
There was a problem hiding this comment.
This foreach loop implicitly filters its target sequence - consider filtering the sequence explicitly using '.Where(...)'.
| foreach (var meta in metadataCollection) | ||
| { | ||
| if (string.Equals(meta.Provider, provider, StringComparison.OrdinalIgnoreCase) | ||
| && string.Equals(meta.EventType, eventType, StringComparison.OrdinalIgnoreCase)) | ||
| { | ||
| return descriptor; | ||
| } | ||
| } |
There was a problem hiding this comment.
This foreach loop implicitly filters its target sequence - consider filtering the sequence explicitly using '.Where(...)'.
| foreach (var descriptor in list) | ||
| { | ||
| if (string.Equals(descriptor.Manifest.Id, engineId, StringComparison.OrdinalIgnoreCase)) | ||
| { | ||
| return descriptor; | ||
| } | ||
| } |
There was a problem hiding this comment.
This foreach loop implicitly filters its target sequence - consider filtering the sequence explicitly using '.Where(...)'.
| foreach (var service in requiredServices) | ||
| { | ||
| if (string.IsNullOrWhiteSpace(service)) | ||
| { | ||
| throw new InvalidOperationException( | ||
| $"Engine '{descriptor.ModuleKey}/{descriptor.Manifest.Id}' declares an empty required service identifier."); | ||
| } | ||
| } |
There was a problem hiding this comment.
This foreach loop implicitly filters its target sequence - consider filtering the sequence explicitly using '.Where(...)'.
Summary
Testing
Codex Task