Repository navigation
refactor(runtime): extract Registration-specific code to domain plugin [OMN-1346] - #154
Conversation
…n [OMN-1346] Move Registration-specific code from the generic runtime layer to the Registration domain (nodes/node_registration_orchestrator/). Changes: - Move dispatchers to nodes/node_registration_orchestrator/dispatchers/ - Move introspection_event_router.py to Registration domain - Extract wire_registration_handlers() to domain wiring.py - Extract wire_registration_dispatchers() to domain wiring.py - Implement ProtocolDomainPlugin for domain-specific initialization - Add PluginRegistration implementing the plugin protocol - Refactor kernel.py to use plugin hooks instead of hardcoded init - Update container_wiring.py to delegate to domain wiring functions - Split domain plugin models per ONEX architecture conventions The runtime layer now contains only generic infrastructure code. Other domains (like Intelligence) can implement their own plugins.
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAdds a domain-plugin system and moves registration orchestration into a registration domain: PluginRegistration, wiring/dispatchers, router, route IDs, and kernel plugin-driven bootstrap; plus plugin models/protocols, tests, and minor DB/validation tweaks. Changes
Sequence Diagram(s)sequenceDiagram
participant Kernel
participant PluginRegistration
participant PostgreSQL as Postgres
participant EventBus
participant DispatchEngine as MessageDispatchEngine
Kernel->>PluginRegistration: should_activate(config)
alt plugin activates
Kernel->>PluginRegistration: initialize(config)
PluginRegistration->>Postgres: create_pool() / init_schema()
PluginRegistration->>PluginRegistration: _load_projector()
PluginRegistration-->>Kernel: initialize result
Kernel->>PluginRegistration: wire_handlers(config)
PluginRegistration->>DispatchEngine: register handlers (wiring)
Kernel->>PluginRegistration: wire_dispatchers(config)
PluginRegistration->>DispatchEngine: register dispatchers & routes
Kernel->>PluginRegistration: start_consumers(config)
PluginRegistration->>EventBus: start consumer(s)
PluginRegistration-->>Kernel: unsubscribe callbacks
end
Kernel->>Kernel: run
Kernel->>PluginRegistration: shutdown(config)
PluginRegistration->>EventBus: stop consumers (call unsubscribe)
PluginRegistration->>Postgres: close_pool()
PluginRegistration-->>Kernel: shutdown result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Comment |
Code Review: Registration Domain Plugin Extraction (OMN-1346)OverviewThis is an excellent refactoring that successfully implements the domain plugin architecture. The PR extracts Registration-specific code from the generic runtime kernel into a clean plugin pattern following dependency inversion principles. Overall Assessment: ✅ APPROVED with minor suggestions The code quality is high, follows ONEX patterns, and significantly improves the separation of concerns. Strengths1. Clean Architecture ✅
2. Strong Typing & Models ✅
3. Error Handling ✅
4. Documentation ✅
Security ReviewResource Management ✅
Error Exposure ✅
Performance ConsiderationsBootstrap Timing ✅
Recommendation: Consider adding timing thresholds with warnings: if duration > 5.0:
logger.warning("Slow plugin initialization: %.2fs", duration)Potential Issues & Suggestions1. Plugin Shutdown Order
|
| Pattern | Status | Notes |
|---|---|---|
No Any types |
✅ | Zero violations detected |
| Strong typing | ✅ | ` |
| Error hierarchy | ✅ | ProtocolConfigurationError, RuntimeHostError |
| Container injection | ✅ | ModelONEXContainer used throughout |
| Correlation IDs | ✅ | Tracked across all operations |
| Declarative nodes | ✅ | Plugin delegates to domain wiring |
| No backwards compat | ✅ | Breaking change, properly documented |
Test Coverage
From PR description:
- ✅ Runtime tests: 1950 passed
- ✅ Registration tests: 238 passed (100%)
- ✅ No Registration imports in core runtime (verified via grep)
Recommendation: Add integration test for plugin failure scenarios:
- Plugin initialization fails mid-bootstrap
- Plugin shutdown fails during cleanup
- Multiple plugins activated with interdependencies
Code Quality Metrics
- Lines changed: +2479 / -829 (net +1650)
- Files modified: 15
- New abstractions: 3 (ProtocolDomainPlugin, RegistryDomainPlugin, PluginRegistration)
- Documentation: Excellent (comprehensive docstrings)
- Complexity: Low (clear lifecycle, single responsibility)
Migration Impact
Breaking Changes ✅ Properly Documented
From runtime/__init__.py diff:
- Removed:
DispatcherNodeIntrospected,DispatcherRuntimeTick,DispatcherNodeRegistrationAcked - Moved to:
nodes.node_registration_orchestrator.dispatchers
Import Path Changes
Old:
from omnibase_infra.runtime.dispatchers import DispatcherNodeIntrospectedNew:
from omnibase_infra.nodes.node_registration_orchestrator.dispatchers import (
DispatcherNodeIntrospected,
)Note: Per ONEX policy, no migration guide required. Downstream consumers expected to update immediately.
Final Recommendations
Must Fix
None - code is production-ready as-is.
Should Consider
- Add timing thresholds for slow plugin initialization warnings
- Make projector contracts directory configurable via environment variable
- Refine schema init error handling to reduce log noise
Nice to Have
- Add integration tests for plugin failure scenarios
- Document plugin dependency ordering constraints
- Add example of creating a custom domain plugin
Conclusion
This PR demonstrates excellent software engineering:
- Clean architectural separation via plugin pattern
- Comprehensive error handling and resource cleanup
- Strong typing with zero
Anyviolations - Production-ready documentation
The refactoring successfully achieves OMN-1346 goals and unblocks future domain plugins (Intelligence, Observability, etc.).
Recommendation: APPROVE and MERGE
Reviewer: Claude Sonnet 4.5
Correlation ID:
Review Date: 2026-01-15
… [OMN-1346] - Add plugin shutdown order documentation with LIFO constraint - Make projector contracts path configurable via ONEX_PROJECTOR_CONTRACTS_DIR - Handle schema idempotency errors at DEBUG level (not WARNING) - Add WiringResult TypedDict for precise return typing
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/omnibase_infra/runtime/kernel.py`:
- Around line 684-690: The logging extra currently uses a truthiness check on
wire_result which calls its __bool__ (e.g., ModelDomainPluginResult.success) and
can hide services_registered when wiring returned an instance with
success=False; update the ternary to explicitly check for a non-None result
(e.g., use "wire_result.services_registered if wire_result is not None else []")
or, if you want empty list on failure, use "wire_result.services_registered if
getattr(wire_result, 'services_registered', None) is not None and
wire_result.success else []" so that the code inspects the actual presence of
the ModelDomainPluginResult instance (wire_result) and/or its success flag
before accessing services_registered in the logging extra.
- Around line 904-922: The current check conflates None, failed, and skipped
consumer_result states; update the plugin consumer startup handling around
plugin.start_consumers/consumer_result so it explicitly branches: first handle
consumer_result is None (log warning with "unknown" correlation_id), then handle
consumer_result.success == False (log warning including
consumer_result.error_message), then handle
consumer_result.unsubscribe_callbacks truthy (extend
plugin_unsubscribe_callbacks and emit the debug log referencing plugin.plugin_id
and correlation_id), and finally handle the success-but-no-callbacks case (emit
an info or debug that plugin.plugin_id started zero consumers/skipped). Ensure
references to consumer_result.error_message, consumer_result.success, and
unsubscribe_callbacks are only accessed in the branches where they are defined.
🧹 Nitpick comments (1)
src/omnibase_infra/nodes/node_registration_orchestrator/plugin.py (1)
656-663:isinstancecheck for KafkaEventBus.Per coding guidelines, protocol resolution should use duck typing via protocols, not
isinstancechecks. However, this is checking a concrete class to determine if Kafka-specific consumer subscription is appropriate.Consider checking for a capability (e.g.,
hasattr(config.event_bus, 'subscribe')with async signature) rather than type, though the current approach is pragmatic sinceInMemoryEventBusmay have different subscription semantics.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Lite
📒 Files selected for processing (15)
src/omnibase_infra/nodes/node_registration_orchestrator/__init__.pysrc/omnibase_infra/nodes/node_registration_orchestrator/dispatchers/__init__.pysrc/omnibase_infra/nodes/node_registration_orchestrator/dispatchers/dispatcher_node_introspected.pysrc/omnibase_infra/nodes/node_registration_orchestrator/dispatchers/dispatcher_node_registration_acked.pysrc/omnibase_infra/nodes/node_registration_orchestrator/dispatchers/dispatcher_runtime_tick.pysrc/omnibase_infra/nodes/node_registration_orchestrator/introspection_event_router.pysrc/omnibase_infra/nodes/node_registration_orchestrator/plugin.pysrc/omnibase_infra/nodes/node_registration_orchestrator/wiring.pysrc/omnibase_infra/runtime/__init__.pysrc/omnibase_infra/runtime/container_wiring.pysrc/omnibase_infra/runtime/kernel.pysrc/omnibase_infra/runtime/models/__init__.pysrc/omnibase_infra/runtime/models/model_domain_plugin_config.pysrc/omnibase_infra/runtime/models/model_domain_plugin_result.pysrc/omnibase_infra/runtime/protocol_domain_plugin.py
🧰 Additional context used
📓 Path-based instructions (4)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Never useAnytype - useobjectfor generic payloads in function parameters and return types
UseX | None(PEP 604) instead ofOptional[X]for nullable types
All services must useModelONEXContainerfor dependency injection via__init__(self, container: ModelONEXContainer)
Use@allow_anydecorator with documented reason as exemption mechanism forAnytype violations
UseJsonTypefromomnibase_core.typesas the canonical type alias for JSON-compatible values
UseModelEventEnvelope[object]for generic dispatcher interfaces andobjectfor generic payloads
Infrastructure error handling must useOnexErrorbase class and never expose passwords, API keys, PII, or connection strings with credentials in error messages
UseInfraConnectionError,InfraTimeoutError,InfraAuthenticationError,InfraUnavailableErrorfor transport failures with properModelInfraErrorContext
Correlation IDs must be propagated from incoming requests, auto-generated withuuid4()if missing, and included in all error contexts
External service adapters must implementMixinAsyncCircuitBreakerwith appropriate threshold and reset_timeout configuration
Protocol resolution must use duck typing via protocols, never useisinstancechecks
Files:
src/omnibase_infra/runtime/protocol_domain_plugin.pysrc/omnibase_infra/nodes/node_registration_orchestrator/dispatchers/dispatcher_runtime_tick.pysrc/omnibase_infra/runtime/models/model_domain_plugin_config.pysrc/omnibase_infra/nodes/node_registration_orchestrator/dispatchers/dispatcher_node_registration_acked.pysrc/omnibase_infra/nodes/node_registration_orchestrator/introspection_event_router.pysrc/omnibase_infra/nodes/node_registration_orchestrator/dispatchers/dispatcher_node_introspected.pysrc/omnibase_infra/nodes/node_registration_orchestrator/__init__.pysrc/omnibase_infra/nodes/node_registration_orchestrator/wiring.pysrc/omnibase_infra/runtime/models/__init__.pysrc/omnibase_infra/runtime/kernel.pysrc/omnibase_infra/nodes/node_registration_orchestrator/plugin.pysrc/omnibase_infra/nodes/node_registration_orchestrator/dispatchers/__init__.pysrc/omnibase_infra/runtime/__init__.pysrc/omnibase_infra/runtime/container_wiring.pysrc/omnibase_infra/runtime/models/model_domain_plugin_result.py
**/protocol_*.py
📄 CodeRabbit inference engine (CLAUDE.md)
File naming convention: Protocols must use
protocol_<name>.pyorprotocols.pywith class nameProtocol<Name>
Files:
src/omnibase_infra/runtime/protocol_domain_plugin.py
**/model_*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/model_*.py: File naming convention: Models must usemodel_<name>.pywith class nameModel<Name>
Each model file must contain exactly oneModel*class
Pydantic workaround forAnytype must include# NOTE:comment documenting the reason when technically required
UseSerializeAsAnytype wrapper for Pydantic fields containing complex nested models to preserve subclass fields during serialization
Files:
src/omnibase_infra/runtime/models/model_domain_plugin_config.pysrc/omnibase_infra/runtime/models/model_domain_plugin_result.py
**/model_*result*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Result models may override
__bool__to enable idiomatic conditional checks, with requiredWarningsection in docstring explaining non-standard behavior
Files:
src/omnibase_infra/runtime/models/model_domain_plugin_result.py
🧠 Learnings (23)
📚 Learning: 2025-11-24T17:24:41.687Z
Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: .cursor/rules/standards.mdc:0-0
Timestamp: 2025-11-24T17:24:41.687Z
Learning: Applies to **/protocols/protocol_*.py : Use Protocol for tool interfaces and plugin APIs based on method shape (structural typing), not Pydantic models with inheritance
Applied to files:
src/omnibase_infra/runtime/protocol_domain_plugin.py
📚 Learning: 2025-11-28T18:58:53.781Z
Learnt from: CR
Repo: OmniNode-ai/omninode_bridge PR: 0
File: .cursor/rules/canonical_patterns.mdc:0-0
Timestamp: 2025-11-28T18:58:53.781Z
Learning: Organize models under `src/omnibase_core/models/` by domain including: base, cli, common, config, core, contracts, discovery, health, infrastructure, logging, metadata, nodes, operations, results, security, service, tools, validation, and workflows
Applied to files:
src/omnibase_infra/runtime/models/model_domain_plugin_config.pysrc/omnibase_infra/nodes/node_registration_orchestrator/__init__.pysrc/omnibase_infra/runtime/models/__init__.py
📚 Learning: 2026-01-15T13:12:08.552Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-15T13:12:08.552Z
Learning: Applies to src/omnibase_core/nodes/**/*.py : Use ModelONEXContainer (not ModelContainer) in node constructors for dependency injection
Applied to files:
src/omnibase_infra/runtime/models/model_domain_plugin_config.pysrc/omnibase_infra/nodes/node_registration_orchestrator/introspection_event_router.pysrc/omnibase_infra/nodes/node_registration_orchestrator/__init__.py
📚 Learning: 2026-01-15T13:12:08.553Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-15T13:12:08.553Z
Learning: Applies to src/omnibase_core/models/**/*.py : Every Pydantic model MUST have an explicit model_config ConfigDict; empty ConfigDict is forbidden. Immutable models must use ConfigDict(frozen=True, extra='forbid', from_attributes=True)
Applied to files:
src/omnibase_infra/runtime/models/model_domain_plugin_config.py
📚 Learning: 2025-11-30T21:55:10.298Z
Learnt from: CR
Repo: OmniNode-ai/omninode_bridge PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-30T21:55:10.298Z
Learning: Applies to src/omninode_bridge/nodes/**/*.py : Import mixins from omnibase_core.mixins.* and use Mixin* naming pattern (e.g., MixinHealthCheck, MixinMetrics, MixinEventBus) - never use local custom mixins unless experimental and documented
Applied to files:
src/omnibase_infra/nodes/node_registration_orchestrator/introspection_event_router.pysrc/omnibase_infra/nodes/node_registration_orchestrator/__init__.pysrc/omnibase_infra/runtime/kernel.pysrc/omnibase_infra/nodes/node_registration_orchestrator/dispatchers/__init__.pysrc/omnibase_infra/runtime/__init__.py
📚 Learning: 2025-12-06T22:21:32.649Z
Learnt from: CR
Repo: OmniNode-ai/omniagent PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T22:21:32.649Z
Learning: Applies to nodes/**/*.py : Use event bus mixins from `omnibase_core` for Kafka publishing instead of direct Kafka clients
Applied to files:
src/omnibase_infra/nodes/node_registration_orchestrator/introspection_event_router.pysrc/omnibase_infra/nodes/node_registration_orchestrator/__init__.pysrc/omnibase_infra/runtime/kernel.py
📚 Learning: 2026-01-15T13:12:08.553Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-15T13:12:08.553Z
Learning: Applies to src/omnibase_core/nodes/node_orchestrator.py : ORCHESTRATOR nodes CANNOT return typed results; they can only emit events and intents. Only COMPUTE nodes return typed results. This enforces separation between coordination and transformation.
Applied to files:
src/omnibase_infra/nodes/node_registration_orchestrator/introspection_event_router.pysrc/omnibase_infra/nodes/node_registration_orchestrator/__init__.py
📚 Learning: 2025-11-30T21:55:10.298Z
Learnt from: CR
Repo: OmniNode-ai/omninode_bridge PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-30T21:55:10.298Z
Learning: Applies to src/omninode_bridge/nodes/**/*.py : All nodes in omninode_bridge MUST use omnibase_core standards (ModelServiceEffect, ModelServiceCompute for effect/compute nodes; NodeOrchestrator, NodeReducer with mixins for orchestrator/reducer nodes)
Applied to files:
src/omnibase_infra/nodes/node_registration_orchestrator/introspection_event_router.pysrc/omnibase_infra/nodes/node_registration_orchestrator/__init__.pysrc/omnibase_infra/nodes/node_registration_orchestrator/dispatchers/__init__.py
📚 Learning: 2025-11-24T17:23:49.777Z
Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: .cursor/rules/node_standards.mdc:0-0
Timestamp: 2025-11-24T17:23:49.777Z
Learning: All ONEX nodes must use the node_kafka_event_bus as a secondary reference only for complex backend and event bus logic and advanced configuration patterns
Applied to files:
src/omnibase_infra/nodes/node_registration_orchestrator/introspection_event_router.pysrc/omnibase_infra/runtime/kernel.py
📚 Learning: 2025-11-30T21:55:10.298Z
Learnt from: CR
Repo: OmniNode-ai/omninode_bridge PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-30T21:55:10.298Z
Learning: Applies to src/omninode_bridge/events/**/*.py : Kafka event publishing MUST use OnexEnvelopeV1 format with 13 topics for event streaming at all workflow lifecycle stages
Applied to files:
src/omnibase_infra/nodes/node_registration_orchestrator/introspection_event_router.py
📚 Learning: 2026-01-11T17:31:33.550Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_infra PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T17:31:33.550Z
Learning: Node Introspection Security - use Python reflection for service discovery, but prefix internal/sensitive methods with `_`, use generic parameter names, and configure Kafka topic ACLs for introspection topics
Applied to files:
src/omnibase_infra/nodes/node_registration_orchestrator/introspection_event_router.py
📚 Learning: 2026-01-11T17:31:33.550Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_infra PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T17:31:33.550Z
Learning: Applies to **/handler_*.py : Handlers must NOT have direct event bus access - only orchestrators may have bus parameters and publish events
Applied to files:
src/omnibase_infra/nodes/node_registration_orchestrator/introspection_event_router.pysrc/omnibase_infra/runtime/kernel.py
📚 Learning: 2026-01-11T17:31:33.550Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_infra PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T17:31:33.550Z
Learning: Applies to **/node.py : All nodes must extend base classes from `omnibase_core.nodes` (NodeOrchestrator, NodeEffect, NodeCompute, NodeReducer) with container injection and no custom logic in node.py
Applied to files:
src/omnibase_infra/nodes/node_registration_orchestrator/__init__.py
📚 Learning: 2025-12-06T22:21:32.649Z
Learnt from: CR
Repo: OmniNode-ai/omniagent PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T22:21:32.649Z
Learning: Applies to nodes/**/*.py : Use `omnibase_infra` handlers for OmniIntelligence queries via HttpRestAdapter envelope pattern
Applied to files:
src/omnibase_infra/nodes/node_registration_orchestrator/__init__.pysrc/omnibase_infra/nodes/node_registration_orchestrator/wiring.pysrc/omnibase_infra/runtime/__init__.py
📚 Learning: 2026-01-11T17:31:33.550Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_infra PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T17:31:33.550Z
Learning: Applies to **/*.py : Use `ModelEventEnvelope[object]` for generic dispatcher interfaces and `object` for generic payloads
Applied to files:
src/omnibase_infra/nodes/node_registration_orchestrator/__init__.py
📚 Learning: 2025-11-24T16:32:55.606Z
Learnt from: CR
Repo: OmniNode-ai/omninode_bridge PR: 0
File: .cursor/rules/node_standards.mdc:0-0
Timestamp: 2025-11-24T16:32:55.606Z
Learning: Node communication must use event-driven patterns through `ModelEventEnvelope` from `omnibase_core.models.events.model_event_envelope`
Applied to files:
src/omnibase_infra/nodes/node_registration_orchestrator/__init__.py
📚 Learning: 2025-11-24T16:32:55.606Z
Learnt from: CR
Repo: OmniNode-ai/omninode_bridge PR: 0
File: .cursor/rules/node_standards.mdc:0-0
Timestamp: 2025-11-24T16:32:55.606Z
Learning: Node implementations must use mixin-based composition from `omnibase_core.mixins` (e.g., `MixinHealthCheck`, `MixinNodeExecutor`) to add capabilities
Applied to files:
src/omnibase_infra/nodes/node_registration_orchestrator/__init__.py
📚 Learning: 2026-01-15T13:12:08.553Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-15T13:12:08.553Z
Learning: Applies to src/omnibase_core/nodes/node_orchestrator.py : Implement NodeOrchestrator with ModelAction Pattern for lease-based single-writer semantics and workflow-driven actions
Applied to files:
src/omnibase_infra/nodes/node_registration_orchestrator/__init__.py
📚 Learning: 2025-11-24T16:33:32.747Z
Learnt from: CR
Repo: OmniNode-ai/omninode_bridge PR: 0
File: .cursor/rules/standards.mdc:0-0
Timestamp: 2025-11-24T16:33:32.747Z
Learning: Applies to **/*.py : Import models from shared core paths using `omnibase.model.core.model_*` pattern
Applied to files:
src/omnibase_infra/runtime/models/__init__.py
📚 Learning: 2026-01-15T13:12:08.552Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-15T13:12:08.552Z
Learning: Applies to src/omnibase_core/**/*.py : Use protocol names (e.g., 'ProtocolEventBus', 'ProtocolLogger') for service resolution, not concrete class names
Applied to files:
src/omnibase_infra/runtime/kernel.py
📚 Learning: 2026-01-15T13:12:08.553Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-15T13:12:08.553Z
Learning: Applies to src/omnibase_core/models/**/*.py : Use ModelEventEnvelope for inter-service event communication with structured event metadata and routing
Applied to files:
src/omnibase_infra/runtime/kernel.py
📚 Learning: 2026-01-06T17:57:00.689Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_spi PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T17:57:00.689Z
Learning: Applies to src/omnibase_spi/**/*.py : SPI MUST NOT import from omnibase_infra (no imports, even transitively)
Applied to files:
src/omnibase_infra/runtime/__init__.py
📚 Learning: 2025-12-06T22:21:32.649Z
Learnt from: CR
Repo: OmniNode-ai/omniagent PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T22:21:32.649Z
Learning: Applies to nodes/effect/**/*.py : Use handler envelopes from `omnibase_infra` for all I/O operations (HTTP, database, Kafka) instead of custom clients
Applied to files:
src/omnibase_infra/runtime/__init__.py
🧬 Code graph analysis (9)
src/omnibase_infra/runtime/protocol_domain_plugin.py (3)
src/omnibase_infra/runtime/models/model_domain_plugin_config.py (1)
ModelDomainPluginConfig(51-90)src/omnibase_infra/runtime/models/model_domain_plugin_result.py (1)
ModelDomainPluginResult(50-241)src/omnibase_infra/nodes/node_registration_orchestrator/plugin.py (8)
plugin_id(124-126)display_name(129-131)should_activate(148-168)initialize(170-258)wire_handlers(475-540)wire_dispatchers(542-626)start_consumers(628-717)shutdown(719-772)
src/omnibase_infra/runtime/models/model_domain_plugin_config.py (3)
src/omnibase_infra/event_bus/inmemory_event_bus.py (1)
InMemoryEventBus(64-709)src/omnibase_infra/event_bus/kafka_event_bus.py (1)
KafkaEventBus(208-2318)src/omnibase_infra/runtime/message_dispatch_engine.py (1)
MessageDispatchEngine(308-1787)
src/omnibase_infra/nodes/node_registration_orchestrator/introspection_event_router.py (1)
src/omnibase_infra/nodes/node_registration_orchestrator/dispatchers/dispatcher_node_introspected.py (1)
DispatcherNodeIntrospected(96-366)
src/omnibase_infra/runtime/models/__init__.py (2)
src/omnibase_infra/runtime/models/model_domain_plugin_config.py (1)
ModelDomainPluginConfig(51-90)src/omnibase_infra/runtime/models/model_domain_plugin_result.py (1)
ModelDomainPluginResult(50-241)
src/omnibase_infra/runtime/kernel.py (6)
src/omnibase_infra/runtime/container_wiring.py (1)
wire_infrastructure_services(198-352)src/omnibase_infra/runtime/registry/registry_protocol_binding.py (1)
ProtocolBindingRegistry(130-438)src/omnibase_infra/runtime/models/model_domain_plugin_result.py (1)
ModelDomainPluginResult(50-241)src/omnibase_infra/runtime/protocol_domain_plugin.py (12)
ProtocolDomainPlugin(131-329)RegistryDomainPlugin(332-417)register(368-390)get_all(403-409)should_activate(199-219)plugin_id(175-186)display_name(189-197)initialize(221-249)wire_handlers(251-278)wire_dispatchers(280-298)start_consumers(300-312)clear(411-413)src/omnibase_infra/nodes/node_registration_orchestrator/plugin.py (10)
PluginRegistration(91-785)should_activate(148-168)plugin_id(124-126)display_name(129-131)initialize(170-258)wire_handlers(475-540)wire_dispatchers(542-626)start_consumers(628-717)get_status_line(774-785)shutdown(719-772)src/omnibase_infra/nodes/node_registration_orchestrator/protocols.py (1)
correlation_id(148-158)
src/omnibase_infra/nodes/node_registration_orchestrator/plugin.py (7)
src/omnibase_infra/nodes/node_registration_orchestrator/dispatchers/dispatcher_node_introspected.py (1)
DispatcherNodeIntrospected(96-366)src/omnibase_infra/runtime/models/model_domain_plugin_config.py (1)
ModelDomainPluginConfig(51-90)src/omnibase_infra/runtime/models/model_domain_plugin_result.py (4)
ModelDomainPluginResult(50-241)failed(176-210)skipped(213-241)succeeded(133-173)src/omnibase_infra/runtime/protocol_domain_plugin.py (9)
ProtocolDomainPlugin(131-329)plugin_id(175-186)display_name(189-197)should_activate(199-219)initialize(221-249)wire_handlers(251-278)wire_dispatchers(280-298)start_consumers(300-312)shutdown(314-329)src/omnibase_infra/nodes/node_registration_orchestrator/wiring.py (1)
wire_registration_handlers(355-505)src/omnibase_infra/runtime/container_wiring.py (1)
wire_registration_handlers(760-838)src/omnibase_infra/nodes/node_registration_orchestrator/introspection_event_router.py (2)
IntrospectionEventRouter(85-432)handle_message(195-432)
src/omnibase_infra/nodes/node_registration_orchestrator/dispatchers/__init__.py (3)
src/omnibase_infra/nodes/node_registration_orchestrator/dispatchers/dispatcher_node_introspected.py (1)
DispatcherNodeIntrospected(96-366)src/omnibase_infra/nodes/node_registration_orchestrator/dispatchers/dispatcher_node_registration_acked.py (1)
DispatcherNodeRegistrationAcked(96-366)src/omnibase_infra/nodes/node_registration_orchestrator/dispatchers/dispatcher_runtime_tick.py (1)
DispatcherRuntimeTick(95-363)
src/omnibase_infra/runtime/container_wiring.py (1)
src/omnibase_infra/nodes/node_registration_orchestrator/wiring.py (6)
wire_registration_handlers(355-505)get_projection_reader_from_container(513-532)get_handler_node_introspected_from_container(535-554)get_handler_runtime_tick_from_container(557-576)get_handler_node_registration_acked_from_container(579-600)wire_registration_dispatchers(153-347)
src/omnibase_infra/runtime/models/model_domain_plugin_result.py (2)
src/omnibase_infra/nodes/node_registration_orchestrator/plugin.py (1)
plugin_id(124-126)src/omnibase_infra/runtime/protocol_domain_plugin.py (1)
plugin_id(175-186)
🔇 Additional comments (43)
src/omnibase_infra/nodes/node_registration_orchestrator/dispatchers/dispatcher_node_registration_acked.py (2)
53-54: LGTM!Cross-reference to OMN-1346 properly documents the refactoring context.
122-124: LGTM!Import path in docstring example correctly updated to reflect the new module location.
src/omnibase_infra/runtime/__init__.py (2)
134-138: LGTM!Clear migration guidance for developers looking for the relocated Registration dispatchers. The comment correctly identifies the new import locations.
269-270: LGTM!Documentation in
__all__section provides inline context about the moved items.src/omnibase_infra/nodes/node_registration_orchestrator/introspection_event_router.py (1)
49-51: LGTM!TYPE_CHECKING import correctly updated to reference the dispatcher from within the same domain package. This improves cohesion now that both the router and dispatcher reside in
node_registration_orchestrator.src/omnibase_infra/nodes/node_registration_orchestrator/dispatchers/dispatcher_runtime_tick.py (2)
54-55: LGTM!Cross-reference to OMN-1346 properly documents the refactoring context.
121-123: LGTM!Import path in docstring example correctly updated to reflect the new module location.
src/omnibase_infra/runtime/models/__init__.py (3)
11-12: LGTM!Docstring entries clearly describe the purpose of the new domain plugin models.
46-51: LGTM!Imports for the new domain plugin models follow the established pattern and are properly organized.
102-103: LGTM!New model exports added to
__all__in alphabetical order, consistent with the rest of the list.src/omnibase_infra/nodes/node_registration_orchestrator/dispatchers/dispatcher_node_introspected.py (1)
53-53: LGTM!The cross-reference addition (Line 53) and updated import path in the docstring example (Lines 122-124) correctly reflect the relocation of this dispatcher to the Registration domain. Documentation is consistent with the PR's architectural changes.
Also applies to: 122-124
src/omnibase_infra/runtime/protocol_domain_plugin.py (3)
130-172: LGTM! Well-designed protocol with duck typing.The
@runtime_checkabledecorator enablesisinstancechecks while maintaining structural typing. The lifecycle order is clearly documented, and the protocol methods align with the kernel bootstrap sequence described in the PR objectives. Based on learnings, this follows the recommended pattern of using Protocol for plugin APIs.
332-417: LGTM! Clean registry implementation.The registry correctly:
- Documents non-thread-safe behavior with clear usage guidance
- Returns a defensive copy from
get_all()to prevent external mutation- Raises
ValueErrorwith clear message for duplicate registrations- Provides
clear()method for test isolation
420-425: LGTM!The
__all__correctly includes both the locally-defined protocol/registry and the re-exported models from themodelssubpackage. This provides a convenient single import location for plugin-related types.src/omnibase_infra/runtime/models/model_domain_plugin_config.py (1)
50-91: LGTM! Appropriate use of dataclass for internal configuration.The
ModelDomainPluginConfigcorrectly:
- Uses
@dataclassfor a simple configuration container (Pydantic would add unnecessary overhead for internal wiring)- Follows PEP 604 union syntax (
InMemoryEventBus | KafkaEventBus,MessageDispatchEngine | None)- Uses
TYPE_CHECKINGguard to avoid circular imports while maintaining type safety- Documents mutability and thread-safety considerations
The naming follows the
model_<name>.pyconvention per coding guidelines.src/omnibase_infra/nodes/node_registration_orchestrator/dispatchers/__init__.py (1)
39-53: LGTM!The import paths correctly reference the local dispatcher modules within the
node_registration_orchestrator.dispatcherspackage. The__all__export list is complete with all three dispatchers, and the documentation updates reflect the new module structure.src/omnibase_infra/runtime/container_wiring.py (3)
760-838: LGTM! Clean delegation to Registration domain.The
wire_registration_handlersfunction correctly:
- Maintains backward-compatible public signature
- Uses deferred import to avoid circular dependencies
- Delegates all parameters to the Registration domain implementation
- Documents the delegation pattern in the docstring
This follows the domain-driven design principle established in the PR objectives.
841-951: LGTM!The getter functions (
get_projection_reader_from_container,get_handler_node_introspected_from_container,get_handler_runtime_tick_from_container,get_handler_node_registration_acked_from_container) all follow a consistent delegation pattern with appropriate documentation noting the OMN-1346 reference.
954-1027: LGTM!The
wire_registration_dispatchersfunction correctly delegates to the Registration domain while maintaining comprehensive documentation including prerequisites, engine frozen behavior, and usage examples.src/omnibase_infra/runtime/models/model_domain_plugin_result.py (4)
49-93: Well-structured result model with proper mutable default handling.The dataclass correctly uses
field(default_factory=list)for mutable defaults (resources_created,services_registered,unsubscribe_callbacks), avoiding the common Python gotcha of shared mutable defaults.
94-130: Compliant__bool__override with required Warning section.The
__bool__override follows coding guidelines for result models by including a comprehensiveWarningsection in the docstring explaining the non-standard behavior. The examples clearly demonstrate both the idiomatic usage and alternatives for explicit checks.
132-173: Factory methodsucceeded()is appropriately minimal.The docstring correctly notes that results requiring additional fields (like
resources_created) should use the constructor directly. This prevents API misuse where callers might expectsucceeded()to accept all fields.
175-242:failed()andskipped()factory methods correctly implement plugin lifecycle semantics.
failed()correctly defaultsmessageto"Plugin {plugin_id} failed"when not providedskipped()setssuccess=True, treating plugin skipping as a non-failure outcome (not an error)- The kernel's conditional checks (
if not result:for failures,if result:for success) correctly leverage the__bool__implementation that returnsself.successsrc/omnibase_infra/nodes/node_registration_orchestrator/__init__.py (2)
48-87: Clean import organization for relocated domain components.The imports are well-organized into logical groups (dispatchers, event router, plugin, wiring) with clear OMN-1346 comments indicating the refactoring context. The deferred grouping pattern keeps related imports together.
89-120:__all__exports align with imports.All newly imported symbols are properly exported in
__all__. The categorization comments in__all__match the import groupings, aiding discoverability.src/omnibase_infra/runtime/kernel.py (5)
423-426: Plugin infrastructure initialization is well-placed.The plugin registry and tracking lists are initialized before the try block, ensuring they're available for cleanup in the finally block regardless of where failures occur.
610-626: Deferred import and explicit plugin registration.Good design choice to use deferred import for
PluginRegistrationand explicit registration rather than magic discovery. This keeps the kernel's dependencies explicit and testable.
645-665: Partial activation continues after initialization failure.The loop correctly skips plugins that fail initialization (
continueon line 654). This allows other plugins to activate even if one fails, which is appropriate for graceful degradation.
1042-1077: Well-documented LIFO shutdown order with self-containment constraint.The shutdown documentation clearly explains the LIFO rationale and the critical constraint that plugins must be self-contained during shutdown. This prevents subtle ordering bugs.
1165-1184: Cleanup config uses minimal container/event_bus is intentional and safe.The
cleanup_configcreates fresh minimal instances (ModelONEXContainer(),InMemoryEventBus()) for shutdown. SincePluginRegistration.shutdown()only accessesconfig.correlation_idfrom the config (for logging), passing minimal instances is safe and appropriate for cleanup phase.src/omnibase_infra/nodes/node_registration_orchestrator/plugin.py (6)
116-146: Clean plugin initialization with typed optional resources.The constructor initializes all resource references to
None, and properties provide typed access. TheTYPE_CHECKINGimports keep runtime imports minimal while maintaining type safety.
194-219: DSN construction and pool creation.The DSN is constructed from environment variables and used directly in
asyncpg.create_pool(). The host is logged but the DSN itself (containing password) is not logged, which is correct. The pool size defaults (min=2, max=10) are reasonable for a registration service.
359-417: Schema initialization with proper idempotent handling.The duplicate table/object errors are correctly caught and logged at DEBUG level, treating them as expected idempotent behavior. The import of
asyncpg.exceptionsat runtime is appropriate since it's only needed in the error handling path.
457-473: Cleanup on failure properly resets all resource references.The
_cleanup_on_failuremethod closes the pool if created and resets all resource references toNone. This prevents resource leaks on partial initialization failures.
719-772: Shutdown correctly handles partial cleanup.The shutdown method:
- Closes the pool with error handling
- Clears all resource references
- Aggregates errors and returns failure result if any occurred
This ensures cleanup completes even if individual operations fail.
788-789: Protocol compliance verification at module load.The assignment
_: ProtocolDomainPlugin = PluginRegistration()verifies at import time thatPluginRegistrationsatisfies the protocol. This is a useful pattern for catching protocol mismatches early.src/omnibase_infra/nodes/node_registration_orchestrator/wiring.py (7)
47-56:WiringResultTypedDict provides precise typing.Using a TypedDict for the return value eliminates the need for type narrowing in callers and makes the API contract explicit. Good pattern for internal wiring results.
80-102: Route ID constants with comprehensive docstrings.Each route constant includes topic pattern, message type, and category documentation. This aids discoverability and ensures consistency when routes are referenced elsewhere.
105-151: Validation helper duplicated intentionally to avoid circular imports.The
_validate_service_registryfunction is a local copy as noted in the docstring. The detailed hints in the error messages help diagnose common configuration issues.
232-341: Dispatcher wiring with clear error propagation.The function:
- Validates prerequisites before imports (fail-fast)
- Creates adapters for all three handlers
- Registers dispatchers and routes with clear structure
- Provides actionable error message on failure
The exception handler correctly suggests calling
wire_registration_handlersfirst.
355-505: Handler wiring registers services with metadata.Services are registered with descriptive metadata including version, capabilities (has_projector, has_consul_handler), and configuration (liveness_interval_seconds). This aids debugging and introspection.
513-600: Getter functions provide consistent error messages.Each getter function validates the service registry and provides a consistent error message pattern suggesting
wire_registration_handlersbe called first. Thecast()calls are appropriate sinceresolve_servicereturns a generic type.
603-618:__all__exports all public symbols.All public constants, functions, and types are properly exported. The categorization comments help navigation.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
PR Review: Registration Plugin Architecture ExtractionOverall AssessmentAPPROVED with recommendations - This is a well-executed refactoring that successfully extracts Registration-specific code from the generic runtime layer into a domain plugin architecture. The implementation demonstrates strong adherence to SOLID principles and the codebase's declarative patterns. Strengths1. Excellent Separation of ConcernsThe plugin pattern properly implements dependency inversion - the kernel depends on 2. Strong Type Safety
3. Robust Error Handling
Issues Found🔴 CRITICAL: Missing Test Coverage for Plugin SystemImpact: HIGH - Zero test coverage for:
Recommendation: Add comprehensive test coverage for
|
CI Fixes: - Fix event bus test failures by adding `if activated_plugins:` guard - Update union threshold 121→122 for domain plugin extraction PR Review Fixes: - Plugin shutdown: Add protocol documentation and 10 new tests - Projector discovery: Replace fragile parent counting with package root - Schema idempotency: Fix docstring to match IF NOT EXISTS implementation - Kernel None checks: Use explicit `is not None` at lines 690, 922, 1078 - KafkaEventBus isinstance: Replace with hasattr + ProtocolEventBusLike
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/omnibase_infra/runtime/kernel.py`:
- Around line 1065-1096: The shutdown loop may leave activated_plugins unchanged
if an exception occurs, causing the later finally block to re-run shutdowns and
double-shutdown plugins; change the loop in the main shutdown sequence to remove
each plugin as it is processed (e.g., use a while activated_plugins: plugin =
activated_plugins.pop() to process in LIFO order or explicitly remove plugin
from activated_plugins after successful/failed shutdown handling) so that
activated_plugins.clear() in the normal path is no longer required and the
finally block will only see unprocessed plugins; ensure the same removal
approach is applied to the backup/final shutdown loop that references
activated_plugins to avoid duplicate shutdown calls.
- Around line 646-676: The code currently does falsy checks like "if not
init_result" then accesses init_result.error_message which raises AttributeError
when the result is None; update the three checks for init_result, wire_result,
and dispatcher_result to use explicit None checks and safe logging: replace "if
not X" with "if X is None" and when logging include either a fallback message or
branch to log X.error_message only if X is not None (e.g., if wire_result is
None: logger.warning('Plugin %s handler wiring failed: no result
(correlation_id=%s)', plugin.plugin_id, correlation_id) else:
logger.warning('Plugin %s handler wiring failed: %s (correlation_id=%s)',
plugin.plugin_id, wire_result.error_message, correlation_id)); apply the same
pattern for initialize, wire_handlers, and wire_dispatchers (init_result,
wire_result, dispatcher_result).
🧹 Nitpick comments (1)
src/omnibase_infra/runtime/kernel.py (1)
919-933: Logic conflatesNoneresult with failed result.The current implementation has confusing control flow. When
consumer_result.success=False, the object is truthy-falsy (evaluates toFalsein boolean context due to__bool__), sonot consumer_resultisTrue. However,consumer_result is not Noneon line 925 would then beTrue, making the ternary always returnconsumer_result.error_message.The branch never handles an actual
Noneresult clearly. Consider restructuring for clarity:♻️ Suggested restructure
- elif not consumer_result: - # Use explicit None check for type narrowing - consumer_result may be - # a failed result (success=False) which is falsy but still has valid - # error_message. We only use "unknown" when consumer_result is None. - error_msg = ( - consumer_result.error_message - if consumer_result is not None - else "unknown" - ) + elif consumer_result is None: + logger.warning( + "Plugin %s consumer startup returned None (correlation_id=%s)", + plugin.plugin_id, + correlation_id, + ) + elif not consumer_result.success: logger.warning( "Plugin %s consumer startup failed: %s (correlation_id=%s)", plugin.plugin_id, - error_msg, + consumer_result.error_message or "unknown", correlation_id, )
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Lite
📒 Files selected for processing (9)
src/omnibase_infra/idempotency/store_postgres.pysrc/omnibase_infra/nodes/node_registration_orchestrator/introspection_event_router.pysrc/omnibase_infra/nodes/node_registration_orchestrator/plugin.pysrc/omnibase_infra/runtime/container_wiring.pysrc/omnibase_infra/runtime/kernel.pysrc/omnibase_infra/runtime/protocol_domain_plugin.pysrc/omnibase_infra/validation/infra_validators.pytests/unit/runtime/test_domain_plugin_shutdown.pytests/unit/validation/test_validator_defaults.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Never useAnytype - useobjectfor generic payloads in function parameters and return types
UseX | None(PEP 604) instead ofOptional[X]for nullable types
All services must useModelONEXContainerfor dependency injection via__init__(self, container: ModelONEXContainer)
Use@allow_anydecorator with documented reason as exemption mechanism forAnytype violations
UseJsonTypefromomnibase_core.typesas the canonical type alias for JSON-compatible values
UseModelEventEnvelope[object]for generic dispatcher interfaces andobjectfor generic payloads
Infrastructure error handling must useOnexErrorbase class and never expose passwords, API keys, PII, or connection strings with credentials in error messages
UseInfraConnectionError,InfraTimeoutError,InfraAuthenticationError,InfraUnavailableErrorfor transport failures with properModelInfraErrorContext
Correlation IDs must be propagated from incoming requests, auto-generated withuuid4()if missing, and included in all error contexts
External service adapters must implementMixinAsyncCircuitBreakerwith appropriate threshold and reset_timeout configuration
Protocol resolution must use duck typing via protocols, never useisinstancechecks
Files:
src/omnibase_infra/validation/infra_validators.pysrc/omnibase_infra/idempotency/store_postgres.pytests/unit/validation/test_validator_defaults.pysrc/omnibase_infra/nodes/node_registration_orchestrator/introspection_event_router.pysrc/omnibase_infra/runtime/kernel.pysrc/omnibase_infra/nodes/node_registration_orchestrator/plugin.pysrc/omnibase_infra/runtime/container_wiring.pysrc/omnibase_infra/runtime/protocol_domain_plugin.pytests/unit/runtime/test_domain_plugin_shutdown.py
**/protocol_*.py
📄 CodeRabbit inference engine (CLAUDE.md)
File naming convention: Protocols must use
protocol_<name>.pyorprotocols.pywith class nameProtocol<Name>
Files:
src/omnibase_infra/runtime/protocol_domain_plugin.py
🧠 Learnings (15)
📚 Learning: 2025-12-06T22:21:32.649Z
Learnt from: CR
Repo: OmniNode-ai/omniagent PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T22:21:32.649Z
Learning: Applies to nodes/**/*.py : Use event bus mixins from `omnibase_core` for Kafka publishing instead of direct Kafka clients
Applied to files:
src/omnibase_infra/nodes/node_registration_orchestrator/introspection_event_router.pysrc/omnibase_infra/runtime/kernel.py
📚 Learning: 2026-01-11T17:31:33.550Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_infra PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T17:31:33.550Z
Learning: Node Introspection Security - use Python reflection for service discovery, but prefix internal/sensitive methods with `_`, use generic parameter names, and configure Kafka topic ACLs for introspection topics
Applied to files:
src/omnibase_infra/nodes/node_registration_orchestrator/introspection_event_router.py
📚 Learning: 2026-01-15T13:12:08.552Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-15T13:12:08.552Z
Learning: Applies to src/omnibase_core/**/*.py : Use protocol names (e.g., 'ProtocolEventBus', 'ProtocolLogger') for service resolution, not concrete class names
Applied to files:
src/omnibase_infra/nodes/node_registration_orchestrator/introspection_event_router.pysrc/omnibase_infra/runtime/kernel.py
📚 Learning: 2025-11-30T21:55:10.298Z
Learnt from: CR
Repo: OmniNode-ai/omninode_bridge PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-30T21:55:10.298Z
Learning: Applies to src/omninode_bridge/nodes/**/*.py : Import mixins from omnibase_core.mixins.* and use Mixin* naming pattern (e.g., MixinHealthCheck, MixinMetrics, MixinEventBus) - never use local custom mixins unless experimental and documented
Applied to files:
src/omnibase_infra/nodes/node_registration_orchestrator/introspection_event_router.pysrc/omnibase_infra/runtime/kernel.py
📚 Learning: 2025-12-07T17:50:13.678Z
Learnt from: CR
Repo: OmniNode-ai/omniintelligence PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-07T17:50:13.678Z
Learning: Implement Kafka event-driven architecture with proper topic naming using prefix dev.archon-intelligence. and proper event flow pattern with Effect nodes consuming events, processing, and publishing results with Dead Letter Queue routing
Applied to files:
src/omnibase_infra/nodes/node_registration_orchestrator/introspection_event_router.py
📚 Learning: 2026-01-15T13:12:08.553Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-15T13:12:08.553Z
Learning: Applies to src/omnibase_core/models/**/*.py : Use ModelEventEnvelope for inter-service event communication with structured event metadata and routing
Applied to files:
src/omnibase_infra/nodes/node_registration_orchestrator/introspection_event_router.pysrc/omnibase_infra/runtime/kernel.py
📚 Learning: 2025-11-29T17:13:38.776Z
Learnt from: CR
Repo: OmniNode-ai/omniarchon PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-29T17:13:38.776Z
Learning: Use event-driven architecture with Kafka topics for asynchronous processing: enrichment, code analysis, manifest processing, and entity embedding pipelines.
Applied to files:
src/omnibase_infra/nodes/node_registration_orchestrator/introspection_event_router.py
📚 Learning: 2025-11-30T21:55:10.298Z
Learnt from: CR
Repo: OmniNode-ai/omninode_bridge PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-30T21:55:10.298Z
Learning: Applies to src/omninode_bridge/events/**/*.py : Kafka event publishing MUST use OnexEnvelopeV1 format with 13 topics for event streaming at all workflow lifecycle stages
Applied to files:
src/omnibase_infra/nodes/node_registration_orchestrator/introspection_event_router.py
📚 Learning: 2025-12-03T16:55:49.755Z
Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T16:55:49.755Z
Learning: Applies to agents/**/*.py : Use routing_event_client from agents/lib/routing_event_client.py for agent routing via Kafka with route_via_events() function
Applied to files:
src/omnibase_infra/nodes/node_registration_orchestrator/introspection_event_router.py
📚 Learning: 2026-01-11T17:31:33.550Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_infra PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T17:31:33.550Z
Learning: Applies to **/*.py : Use `ModelEventEnvelope[object]` for generic dispatcher interfaces and `object` for generic payloads
Applied to files:
src/omnibase_infra/nodes/node_registration_orchestrator/introspection_event_router.py
📚 Learning: 2025-11-24T17:23:49.777Z
Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: .cursor/rules/node_standards.mdc:0-0
Timestamp: 2025-11-24T17:23:49.777Z
Learning: All ONEX nodes must use the node_kafka_event_bus as a secondary reference only for complex backend and event bus logic and advanced configuration patterns
Applied to files:
src/omnibase_infra/nodes/node_registration_orchestrator/introspection_event_router.pysrc/omnibase_infra/runtime/kernel.py
📚 Learning: 2026-01-11T17:31:33.550Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_infra PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T17:31:33.550Z
Learning: Applies to **/handler_*.py : Handlers must NOT have direct event bus access - only orchestrators may have bus parameters and publish events
Applied to files:
src/omnibase_infra/runtime/kernel.py
📚 Learning: 2026-01-15T13:12:08.552Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-15T13:12:08.552Z
Learning: Applies to src/omnibase_core/nodes/**/*.py : Use ModelONEXContainer (not ModelContainer) in node constructors for dependency injection
Applied to files:
src/omnibase_infra/runtime/container_wiring.py
📚 Learning: 2026-01-15T13:12:08.553Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-15T13:12:08.553Z
Learning: Applies to src/omnibase_core/nodes/**/*.py : Distinguish between ModelContainer[T] (value wrapper) and ModelONEXContainer (dependency injection); ModelContainer must NEVER be used in node __init__ constructors
Applied to files:
src/omnibase_infra/runtime/container_wiring.py
📚 Learning: 2025-11-24T17:24:41.687Z
Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: .cursor/rules/standards.mdc:0-0
Timestamp: 2025-11-24T17:24:41.687Z
Learning: Applies to **/protocols/protocol_*.py : Use Protocol for tool interfaces and plugin APIs based on method shape (structural typing), not Pydantic models with inheritance
Applied to files:
src/omnibase_infra/runtime/protocol_domain_plugin.py
🧬 Code graph analysis (2)
src/omnibase_infra/nodes/node_registration_orchestrator/introspection_event_router.py (2)
src/omnibase_infra/nodes/node_registration_orchestrator/dispatchers/dispatcher_node_introspected.py (1)
DispatcherNodeIntrospected(96-366)src/omnibase_infra/runtime/runtime_host_process.py (1)
output_topic(545-551)
src/omnibase_infra/runtime/protocol_domain_plugin.py (4)
src/omnibase_infra/runtime/models/model_domain_plugin_config.py (1)
ModelDomainPluginConfig(51-90)src/omnibase_infra/runtime/models/model_domain_plugin_result.py (1)
ModelDomainPluginResult(50-241)src/omnibase_infra/nodes/node_registration_orchestrator/plugin.py (8)
plugin_id(158-160)display_name(163-165)should_activate(182-202)initialize(204-292)wire_handlers(511-576)wire_dispatchers(578-662)start_consumers(664-754)shutdown(756-809)tests/unit/runtime/test_domain_plugin_shutdown.py (8)
plugin_id(47-49)display_name(52-54)should_activate(56-58)initialize(60-65)wire_handlers(67-72)wire_dispatchers(74-79)start_consumers(81-86)shutdown(88-99)
🔇 Additional comments (28)
src/omnibase_infra/validation/infra_validators.py (1)
384-397: LGTM! Well-documented threshold bump.The threshold history comment properly documents the ticket reference (OMN-1346), date, and rationale (+1 non-optional union from extract registration domain plugin). The increment from 121 to 122 is consistent with the documented history pattern.
tests/unit/validation/test_validator_defaults.py (1)
64-70: LGTM! Test expectation properly synchronized.The threshold history comment and assertion are correctly updated to match the new
INFRA_MAX_UNIONSvalue of 122. The assertion message accurately reflects the expected value and the exclusion ofX | Nonepatterns.src/omnibase_infra/idempotency/store_postgres.py (2)
27-30: LGTM! Docstring accurately documents idempotent index creation.The schema documentation now correctly reflects the
IF NOT EXISTSpattern for all indexes, and the new partial index oncorrelation_idis appropriately documented with theWHERE correlation_id IS NOT NULLclause. This aligns with the implementation in_ensure_table_exists.
345-368: LGTM! Idempotent index creation with appropriate partial index optimization.The changes provide two improvements:
Idempotent initialization: Using
CREATE INDEX IF NOT EXISTSensures safe re-initialization without errors, supporting the broader domain-plugin architecture's emphasis on deterministic resource setup.Storage optimization: The partial index on
correlation_id(lines 358-362) only indexes non-null values, which is appropriate sincecorrelation_idis optional. This saves storage and improves insert performance for records without correlation IDs while still enabling efficient lookups for distributed tracing queries.src/omnibase_infra/runtime/protocol_domain_plugin.py (3)
130-172: LGTM! Well-designed protocol with comprehensive documentation.The
@runtime_checkabledecorator enablesisinstancechecks against the protocol while maintaining duck typing semantics. The protocol follows structural typing best practices as recommended in the learnings, and the lifecycle documentation is thorough with clear ordering guarantees.
381-466: Registry implementation is clean and follows explicit registration pattern.The
RegistryDomainPlugincorrectly documents thread-safety limitations and provides the expected registry operations. The duplicate registration check with a clear error message is helpful for debugging.One minor note: the
clear()method is documented as "useful for testing" - consider if this should be protected or have additional safeguards in production code, though for the current use case it appears appropriate.
469-474: Verify all exports include all expected types.The exports include
ModelDomainPluginConfigandModelDomainPluginResultwhich are imported fromomnibase_infra.runtime.modelsbut re-exported here. This is a reasonable convenience pattern, but ensure consumers are aware these types originate from the models module.src/omnibase_infra/nodes/node_registration_orchestrator/introspection_event_router.py (3)
50-54: Good alignment with duck typing guidelines.The import changes correctly move
DispatcherNodeIntrospectedto its new domain location and introduceProtocolEventBusLikefor type hints. This follows the coding guideline: "Protocol resolution must use duck typing via protocols, never useisinstancechecks."
119-134: Type annotation update is correct.The
event_busparameter and attribute are now typed asProtocolEventBusLike, enabling any compliant event bus implementation to be used. The docstrings are updated consistently.
341-345:publish_envelopemethod is correctly available onProtocolEventBusLike.The method signature at lines 74-78 of
protocol_event_bus_like.pymatches the usage:async def publish_envelope(self, envelope: object, topic: str) -> None:. The call at lines 341-345 passes the correct parameters, and the protocol documentation confirms this is the preferred method when available.src/omnibase_infra/nodes/node_registration_orchestrator/plugin.py (6)
106-122: Good approach for robust path resolution.Using the package root via
omnibase_infra.__file__instead of counting parent directories is more robust against internal restructuring. The environment variable override provides necessary flexibility for different deployment layouts.
228-253: Consider masking password in DSN construction for logs.The DSN is constructed with
POSTGRES_PASSWORDand while it's not logged directly here, ensure this pattern doesn't inadvertently expose credentials elsewhere. The logging at line 245-252 correctly only logs host/port/database, not the password.
395-453: Well-handled idempotent schema initialization.The schema initialization correctly handles
DuplicateTableErrorandDuplicateObjectErrorat DEBUG level, treating them as expected for idempotent operations. This follows best practices for database migrations.
694-700: Duck typing check aligns with coding guidelines.Using
hasattr(config.event_bus, "subscribe")instead ofisinstancecheck correctly follows the coding guideline: "Protocol resolution must use duck typing via protocols, never useisinstancechecks."
756-809: Shutdown implementation follows self-contained constraint.The shutdown method correctly:
- Handles pool closure errors gracefully without re-raising
- Clears all resource references to
Noneafter cleanup- Returns failed result with error details instead of raising
This aligns with the documented constraint in
ProtocolDomainPlugin.shutdown.
825-826: Good practice: Protocol compliance verification at module load.The assignment
_: ProtocolDomainPlugin = PluginRegistration()verifies structural compatibility with the protocol at import time, catching interface mismatches early.src/omnibase_infra/runtime/container_wiring.py (3)
842-871: Delegation wrappers maintain backward compatibility.The
get_*_from_containerfunctions now serve as thin wrappers that delegate to the Registration domain. This maintains the existing public API while keeping domain-specific code in its proper location.
955-1028: Dispatcher wiring delegation is consistent.The
wire_registration_dispatchersfunction follows the same delegation pattern. The comprehensive docstring is preserved, providing good context for consumers.
761-839: Clean delegation pattern for domain separation.The function now delegates entirely to the Registration domain wiring module while preserving the same public API. The docstring correctly documents this architectural decision with reference to OMN-1346.
The return type changed from
dict[str, list[str] | str]toWiringResult(a TypedDict withservices: list[str]andstatus: strfields). TypedDict maintains backward compatibility with dictionary key access, so all existing callers that usesummary["services"]andsummary["status"]patterns continue to work without modification. The import is in place and the delegation pattern successfully separates concerns.tests/unit/runtime/test_domain_plugin_shutdown.py (4)
31-100: Well-designed mock plugin for testing.The
MockPluginclass correctly implements allProtocolDomainPluginmethods and includes useful testing features:
- Shared shutdown order tracking via
_shutdown_orderlist- Configurable error injection via
_shutdown_error- Call tracking via
_shutdown_calledflagThis enables comprehensive testing of plugin lifecycle behavior.
102-166: Good test coverage for LIFO shutdown order and error resilience.The tests verify:
test_shutdown_order_is_lifo: Confirms reversed iteration produces expected LIFO ordertest_shutdown_continues_after_plugin_error: Validates that errors in one plugin don't block othersThese tests directly validate the documented shutdown constraints in
ProtocolDomainPlugin.shutdown.
229-245: Documentation verification test is valuable.Testing that the protocol docstring contains expected keywords (
LIFO,self-contained,MUST NOT depend) ensures documentation stays synchronized with implementation. This is a good practice for maintaining protocol contracts.
294-320: Integration test validates minimal cleanup config pattern.This test verifies that the kernel's finally-block cleanup pattern works correctly - creating a minimal
ModelDomainPluginConfigfor shutdown even in error scenarios. This is important for ensuring graceful degradation.src/omnibase_infra/runtime/kernel.py (5)
77-85: LGTM!The new imports for the domain plugin architecture are well-organized. Using protocol types (
ProtocolDomainPlugin,RegistryDomainPlugin) and model types (ModelDomainPluginConfig,ModelDomainPluginResult) aligns with the coding guidelines for protocol resolution via duck typing.
423-427: LGTM!Plugin infrastructure variables are properly typed and initialized before the try block, ensuring they're available for cleanup in the finally block.
593-626: LGTM!The plugin setup is well-documented with clear rationale. The deferred import prevents circular dependencies, and explicit plugin registration (rather than magic discovery) makes the code more maintainable and debuggable.
939-948: LGTM!The duck-typing approach with
hasattrfor the optionalget_status_linemethod aligns with the coding guidelines for protocol resolution withoutisinstancechecks.
1053-1096: Excellent shutdown documentation and implementation.The LIFO shutdown order with clear constraints is well-documented. The explicit
Nonecheck pattern on lines 1078-1082 correctly handles the case whereshutdown_resultcould beNoneor a failed result.However, for consistency with the suggested refactor in consumer startup, consider the same pattern here:
💡 Optional: Consistent pattern suggestion
- else: - # Use explicit None check for type narrowing - shutdown_result may - # be a failed result (success=False) which is falsy but still has - # valid error_message. We only use "unknown" when it's None. - error_msg = ( - shutdown_result.error_message - if shutdown_result is not None - else "unknown" - ) + elif shutdown_result is None: + logger.warning( + "Plugin %s shutdown returned None (correlation_id=%s)", + plugin.plugin_id, + correlation_id, + ) + else: # shutdown_result.success is False logger.warning( "Plugin %s shutdown failed: %s (correlation_id=%s)", plugin.plugin_id, - error_msg, + shutdown_result.error_message or "unknown", correlation_id, )
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
…fety fixes [OMN-1346] - Resolve merge conflict in kernel.py by accepting domain plugin architecture - Fix double-shutdown of plugins using pop() pattern to prevent re-processing in finally block if exception occurs mid-shutdown - Fix potential AttributeError when accessing .error_message on None results by using explicit None checks with safe fallback for init_result, wire_result, and dispatcher_result - Verify MAJOR issues already addressed: - Schema Initialization Idempotency: catches DuplicateTableError/DuplicateObjectError - Projector Discovery Path: uses package root via _get_default_projector_contracts_dir()
Code Review: Registration Domain Plugin Extraction [OMN-1346]OverviewThis PR successfully extracts Registration-specific code from the generic runtime layer into a domain plugin architecture. The refactoring demonstrates strong architectural discipline with excellent separation of concerns. ✅ Strengths
|
… [OMN-1346] Fix mypy error by passing graceful_mode through ModelProjectorPluginLoaderConfig instead of as a direct keyword argument to ProjectorPluginLoader.__init__.
Pull Request Review: Registration Domain Plugin Extraction (OMN-1346)SummaryThis PR successfully implements a plugin architecture to extract Registration-specific code from the generic runtime layer. The refactoring follows solid architectural principles and demonstrates excellent attention to ONEX conventions. ✅ Strengths1. Excellent Architectural Design
2. Strong Type Safety
3. Robust Error Handling
4. ONEX Compliance
5. Excellent Documentation
6. Test Coverage
🔍 Observations & Minor Issues1. Explicit None Checks (Not a Bug, Just Verbose)Lines 650-660, 667-677, 684-696 in if init_result is None or not init_result.success:
error_msg = (
init_result.error_message if init_result is not None else "unknown"
)Observation: The pattern is correct but verbose. Since Recommendation: Consider adding a helper method to def get_error_message_or_default(self, default: str = "unknown") -> str:
return self.error_message if self.error_message else default2. Contract Directory Path Calculation
def _get_default_projector_contracts_dir() -> Path:
import omnibase_infra
package_root = Path(omnibase_infra.__file__).parent
return package_root / "projectors" / "contracts"Issue: Uses Recommendation: Consider using from importlib.resources import files
return files("omnibase_infra").joinpath("projectors/contracts")3. Schema Idempotency Error Handling
duplicate_errors = (
asyncpg.exceptions.DuplicateTableError,
asyncpg.exceptions.DuplicateObjectError,
)
if isinstance(schema_error, duplicate_errors):
logger.debug("Schema already initialized (idempotent, correlation_id=%s)", ...)Observation: This violates the "duck typing, never isinstance" rule from CLAUDE.md. However, this is an exception-handling scenario where checking exception types is standard practice. Recommendation: Document why 4. Windows Signal Handling
def windows_handler(signum: int, frame: object) -> None:
sig = signal.Signals(signum)
logger.info("Received %s...", sig.name, correlation_id)
loop.call_soon_threadsafe(shutdown_event.set)Issue: The Fix: logger.info(
"Received %s, initiating graceful shutdown... (correlation_id=%s)",
sig.name,
correlation_id,
)🛡️ Security Considerations1. Credential Sanitization
2. Plugin Discovery
📊 Performance Considerations1. Bootstrap Timing
2. Resource Pooling
🧪 Test Coverage AnalysisTest Files Analyzed:
Coverage Assessment: Excellent
🎯 RecommendationsHigh Priority
Medium Priority
Low Priority
🚀 Final VerdictApprove with Minor CommentsThis is high-quality work that demonstrates:
The identified issues are minor and don't block merging. The refactoring successfully achieves the goal of extracting Registration code from the runtime layer while maintaining backward compatibility (in terms of functionality, not API - per ONEX policy). Recommended Action: ✅ Approve (with optional follow-up fixes for minor issues) 📝 Checklist
Reviewed by: Claude Sonnet 4.5 |
…ss [OMN-1346] - Add get_error_message_or_default() helper to ModelDomainPluginResult - Update contract directory path to use importlib.resources for robustness - Document isinstance usage for exception handling (standard practice) - Add structured logging extra dict to Windows signal handler
PR #154 Review: Registration Domain Plugin ArchitectureExecutive SummaryThis PR successfully extracts Registration-specific code from the kernel into a domain plugin architecture. The implementation follows ONEX architecture principles with strong type safety, proper error handling, and clean separation of concerns. The code quality is excellent overall. Overall Assessment: APPROVED ✅ 1. ONEX Architecture Compliance ✅Strong Typing & Type Safety (EXCELLENT)
Container-Based Dependency Injection (EXCELLENT)
Protocol Usage (GOOD with minor note)
plugin.py:375 - Type narrowing after runtime discovery: # Suggestion: Add comment
# Type narrowing after runtime discovery - isinstance acceptable here
if isinstance(discovered, ProjectorShell):
self._projector = discoveredplugin.py:466 - Exception handling - Already documented properly ✅ Error Handling (EXCELLENT)
2. Code QualityPlugin Architecture Design (EXCELLENT)
Resource Management (EXCELLENT)
Potential Issue - Concurrent Shutdown Protectionkernel.py:1091-1123 - Plugin shutdown uses Recommendation for plugin.py: class PluginRegistration:
def __init__(self) -> None:
self._pool: asyncpg.Pool | None = None
self._shutdown_in_progress = False # Add this
async def shutdown(self, config: ModelDomainPluginConfig) -> ModelDomainPluginResult:
if self._shutdown_in_progress:
return ModelDomainPluginResult.skipped(
plugin_id=self.plugin_id,
reason="Shutdown already in progress"
)
self._shutdown_in_progress = True
try:
# ... existing shutdown logic ...
finally:
self._shutdown_in_progress = False3. Security (EXCELLENT)Password Handling ✅plugin.py:250-255 - PostgreSQL DSN construction is secure:
Plugin Isolation (GOOD)
Recommendation for wiring.py:355: """
Note: Services are registered with scope="global" and may conflict if
multiple plugins register the same interface. Use domain-specific
interface types to ensure isolation.
"""4. Input ValidationGood Coverage
Minor Issue - Pool Creation Validationplugin.py:258-262 - Should validate pool creation succeeded: self._pool = await asyncpg.create_pool(...)
if self._pool is None: # Add this check
raise RuntimeError("PostgreSQL pool creation returned None")5. Documentation (EXCELLENT)Strengths
Minor Fix Neededprotocol_domain_plugin.py:53 - Docstring refers to wrong class name:
6. Test CoverageGood Coverage ✅
Recommended Additional Tests (can be post-merge)
7. Performance (EXCELLENT)
SummaryCode Quality Score: 9.5/10 - Excellent work! Strengths
Recommended Actions Before Merge
Breaking ChangesPer CLAUDE.md policy, this PR correctly introduces breaking changes without backwards compatibility. Downstream consumers will need to update to the new plugin architecture. Verdict: APPROVED ✅ - Ready to merge after addressing the two high-priority items above. The plugin architecture is clean, well-documented, and follows ONEX principles rigorously. |
…OMN-1346] - Add _shutdown_in_progress flag to prevent concurrent shutdown calls - Extract shutdown logic to _do_shutdown() with try/finally guard - Add pool creation validation after asyncpg.create_pool() - Document global scope service registration in wiring.py docstring
PR Review: Domain Plugin Architecture for Registration ExtractionSummaryThis PR successfully implements a plugin-based architecture to extract Registration-specific code from the generic runtime layer into the Registration domain. The implementation follows ONEX architectural principles and demonstrates strong adherence to SOLID principles. Overall Assessment: APPROVE - High-quality refactoring with excellent separation of concerns, comprehensive testing, and solid error handling. Strengths1. Excellent Architectural Design
2. Strong ONEX Compliance
3. Comprehensive Error Handling
4. Excellent Test Coverage
5. Robust Path Resolution
6. Strong Documentation
Minor Recommendations (Non-blocking)
Security & Performance
Breaking ChangesProperly handled per CLAUDE.md policy (no backwards compatibility maintained):
VerdictAPPROVE - This is an exemplary refactoring that:
The minor issues raised are non-blocking and can be addressed in follow-up work if desired. Great work on this architectural improvement! Reviewed by Claude Code following ONEX infrastructure guidelines from CLAUDE.md |
1 participant
Summary
Moves Registration-specific code from the generic runtime layer to the Registration domain (
nodes/node_registration_orchestrator/), implementing a plugin architecture for domain-specific initialization.ProtocolDomainPluginfor domain-specific initialization hooksPluginRegistrationimplementing the plugin protocolChanges
runtime/dispatchers/nodes/node_registration_orchestrator/dispatchers/runtime/introspection_event_router.pynodes/node_registration_orchestrator/introspection_event_router.pycontainer_wiring.py(embedded)nodes/node_registration_orchestrator/wiring.pykernel.py(hardcoded)PluginRegistrationvia plugin hooksNew Architecture
Test plan
Related
Summary by CodeRabbit
Refactor
New Features
Bug Fixes / Ops
Tests
✏️ Tip: You can customize this high-level summary in your review settings.