Repository navigation
refactor(registry): load handler routing from contract.yaml [OMN-1316] - #153
Conversation
Replace programmatic handler construction with contract-driven loading using the Handler Plugin Loader pattern. This eliminates the dual source of truth between contract.yaml and hardcoded Python imports. Changes: - Extract handler routing loader to shared utility at runtime/contract_loaders/handler_routing_loader.py - Update node.py to use shared utility via thin wrapper - Refactor registry to dynamically load handlers from contract.yaml - Add comprehensive test suite (35 new tests) The registry now: - Loads handler class paths from contract.yaml handler_routing section - Uses importlib for dynamic class loading - Keeps DI/constructor params for dependency injection - Validates protocol compliance before registration
📝 WalkthroughWalkthroughCentralizes handler-routing contract parsing into a new runtime contract loader; node orchestrator delegates subcontract loading to it; registry now dynamically imports, instantiates, and validates handlers from contract.yaml using an allowlist and dependency map, replacing hard-coded handler wiring. Changes
Sequence Diagram(s)sequenceDiagram
participant Registry as RegistryOrchestrator
participant SharedLoader as ContractLoader
participant ContractFile as "contract.yaml"
participant Importer as DynamicImporter
participant HandlerClass as HandlerClass
participant HandlerInst as HandlerInstance
Registry->>SharedLoader: load_handler_class_info_from_contract(path)
SharedLoader->>ContractFile: read & parse handler_routing
SharedLoader-->>Registry: return list of {handler_module, handler_class, routing_key}
loop for each handler entry
Registry->>Importer: import module.class (namespace allowlist)
Importer-->>Registry: class object or error
Registry->>HandlerClass: instantiate(class, dependencies...)
HandlerClass-->>HandlerInst: created
Registry->>Registry: validate protocol & register handler
end
Registry-->>Registry: registration complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🧹 Recent nitpick comments
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Lite 📒 Files selected for processing (2)
🧰 Additional context used📓 Path-based instructions (3)**/*.py📄 CodeRabbit inference engine (CLAUDE.md)
Files:
**/registry_*.py📄 CodeRabbit inference engine (CLAUDE.md)
Files:
**/handler_*.py📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🧠 Learnings (33)📓 Common learnings📚 Learning: 2026-01-11T17:31:33.550ZApplied to files:
📚 Learning: 2026-01-15T13:12:08.553ZApplied to files:
📚 Learning: 2026-01-15T13:12:08.553ZApplied to files:
📚 Learning: 2025-12-06T22:21:32.649ZApplied to files:
📚 Learning: 2026-01-11T17:31:33.550ZApplied to files:
📚 Learning: 2026-01-11T17:31:33.550ZApplied to files:
📚 Learning: 2025-12-06T22:21:32.649ZApplied to files:
📚 Learning: 2025-11-24T17:23:49.777ZApplied to files:
📚 Learning: 2026-01-11T17:31:33.550ZApplied to files:
📚 Learning: 2025-12-06T22:21:32.649ZApplied to files:
📚 Learning: 2025-11-24T17:24:41.687ZApplied to files:
📚 Learning: 2025-11-24T17:22:32.195ZApplied to files:
📚 Learning: 2025-11-24T17:22:32.195ZApplied to files:
📚 Learning: 2026-01-11T17:31:33.550ZApplied to files:
📚 Learning: 2025-11-24T17:24:41.687ZApplied to files:
📚 Learning: 2026-01-15T13:12:08.552ZApplied to files:
📚 Learning: 2025-11-24T16:33:32.747ZApplied to files:
📚 Learning: 2025-11-24T17:22:32.195ZApplied to files:
📚 Learning: 2026-01-06T17:57:00.689ZApplied to files:
📚 Learning: 2026-01-11T17:31:33.550ZApplied to files:
📚 Learning: 2025-12-03T16:55:49.755ZApplied to files:
📚 Learning: 2025-12-03T16:55:49.755ZApplied to files:
📚 Learning: 2025-11-29T17:13:38.776ZApplied to files:
📚 Learning: 2026-01-11T17:31:33.550ZApplied to files:
📚 Learning: 2025-11-24T17:23:49.777ZApplied to files:
📚 Learning: 2025-11-24T17:24:41.687ZApplied to files:
📚 Learning: 2025-12-06T22:21:32.649ZApplied to files:
📚 Learning: 2026-01-11T17:31:33.550ZApplied to files:
📚 Learning: 2025-11-28T18:58:53.781ZApplied to files:
📚 Learning: 2025-12-07T17:50:13.678ZApplied to files:
📚 Learning: 2025-11-24T16:32:55.606ZApplied to files:
📚 Learning: 2025-11-24T17:24:41.687ZApplied to files:
🧬 Code graph analysis (2)src/omnibase_infra/nodes/node_registration_orchestrator/registry/registry_infra_node_registration_orchestrator.py (4)
src/omnibase_infra/runtime/contract_loaders/handler_routing_loader.py (5)
🔇 Additional comments (13)
✏️ Tip: You can disable this entire section by setting Comment |
PR Review: Contract-Driven Handler Loading [OMN-1316]SummaryThis PR successfully refactors handler registry loading to use contract-driven plugin loading, eliminating the dual source of truth between ✅ StrengthsArchitecture & Design
Code Quality
Testing
🔍 Issues & Concerns1. CRITICAL: Security - Arbitrary Code Execution via Dynamic ImportLocation: module = importlib.import_module(module_path)Risk: The registry loads handler modules directly from Attack Vector: handler:
name: "MaliciousHandler"
module: "os" # Or any module with side effects on importRecommendation:
ALLOWED_NAMESPACES = [
"omnibase_infra.handlers.",
"omnibase_infra.nodes.",
]
def _load_handler_class(class_name: str, module_path: str, allowed_namespaces: list[str]) -> type:
if not any(module_path.startswith(ns) for ns in allowed_namespaces):
raise ProtocolConfigurationError(
f"Handler module {module_path} not in allowed namespaces"
)
# ... rest of implementationSeverity: High - This is production code that loads from configuration files 2. Code Duplication: YAML Loading LogicLocations:
Issue: Both functions parse Impact:
Recommendation: Extract common YAML loading logic: def _load_contract_yaml(contract_path: Path) -> dict:
"""Load and validate contract.yaml with standardized error handling."""
# Common loading + error handling logic
passThen both functions use this shared helper. 3. Brittle: Hardcoded Dependency MapLocation: handler_dependencies: dict[str, dict[str, object]] = {
"HandlerNodeIntrospected": {
"projection_reader": projection_reader,
"projector": projector,
"consul_handler": consul_handler,
},
# ... 3 more handlers
}Issues:
Better Approach (from CLAUDE.md): handler_routing:
handlers:
- event_model: { name: "ModelNodeIntrospectionEvent" }
handler:
name: "HandlerNodeIntrospected"
dependencies:
- projection_reader
- projector
- consul_handlerThen construct the dependency map from contract data at runtime. 4. Inconsistent Transport Type in Error ContextLocation: ctx = ModelInfraErrorContext(
transport_type=EnumInfraTransportType.DATABASE, # ← Wrong\!
operation="load_handler_routing_contract",
)Issue: Loading a YAML file is not a database operation. This should be Impact: Misleading error categorization, incorrect monitoring/alerting Fix: Change all instances in 5. Missing Protocol Validation for Loaded HandlersLocation: Registry validates handlers AFTER instantiation (line 464), but the Risk: You could load a class that's not a handler, waste resources instantiating it, then fail. Recommendation: Add early validation in handler_class: type = getattr(module, class_name)
# Quick check: class should have handle method
if not callable(getattr(handler_class, "handle", None)):
raise ProtocolConfigurationError(
f"{class_name} does not appear to be a handler (missing handle method)"
)
return handler_class6. Ambiguous Error MessageLocation: raise ProtocolConfigurationError(
f"No dependency configuration found for handler {handler_class_name}. "
"Update handler_dependencies map with required constructor arguments.",
)Issue: This error tells developers to "update handler_dependencies map" but doesn't say WHERE that map is (it's in the same file, but not obvious to someone debugging at 2am). Recommendation: Include file location: f"No dependency configuration found for handler {handler_class_name}. "
f"Update handler_dependencies map in {__file__} with required constructor arguments."7. Test Coverage Gap: Dynamic Import FailuresObservation: Tests cover
Recommendation: Add integration tests that:
8. Performance: Repeated Contract ParsingIssue: If you call Impact: Minor - YAML parsing is fast, but this violates single-responsibility (registry shouldn't own contract loading) Suggestion: Consider memoizing 🎨 Style & ConventionMinor Issues
📋 Testing NotesWhat's Tested Well
What Could Use More Tests
🔐 Security Checklist (Per CLAUDE.md)
Action Required: Implement namespace allowlisting before production deployment. 🎯 Recommendations Priority
✨ Overall AssessmentScore: 7.5/10 Verdict: Approve with required security fixes before merge. This is a solid refactoring that achieves the core goal (eliminate dual source of truth for handler routing). The code is well-structured, properly tested, and follows ONEX patterns. However, the namespace allowlisting security gap is a blocker for production use according to CLAUDE.md's Handler Plugin Loader Security guidelines. The hardcoded dependency map is a design concern (creates a new dual source of truth), but it's acceptable as a transitional step - the contract already has the structure to support dependency declaration. Recommended next steps:
Great work on the comprehensive tests and clear documentation! 🚀 |
![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/nodes/node_registration_orchestrator/registry/registry_infra_node_registration_orchestrator.py`:
- Around line 163-249: Replace the duplicated YAML parsing in
_load_handler_routing_from_contract by reusing the shared loader: import
load_handler_routing_subcontract from handler_routing_loader.py and call it to
obtain handler keys, then either (preferred) extend
load_handler_routing_subcontract to also return the handler "module" field so
this registry can get fully qualified module paths, or (alternative) call the
shared loader for names and load the raw contract.yaml only to extract module
fields (keeping all YAML parsing centralized). Update
_load_handler_routing_from_contract to remove its own yaml.safe_load logic and
instead map the shared loader's output (plus module info if you extended the
shared loader) to the expected list of {"handler_class": ..., "handler_module":
...}.
In `@src/omnibase_infra/runtime/contract_loaders/handler_routing_loader.py`:
- Around line 143-147: Replace EnumInfraTransportType.DATABASE with
EnumInfraTransportType.FILESYSTEM in every ModelInfraErrorContext created inside
the load_handler_routing_contract flow; specifically update the transport_type
argument for the ModelInfraErrorContext instances tied to
operation="load_handler_routing_contract" (the ones around the contract_path,
parsing, validation, and any filesystem read error handlers) so the error
context correctly reflects local filesystem transport. Ensure all four
ModelInfraErrorContext calls in this function use
EnumInfraTransportType.FILESYSTEM.
🧹 Nitpick comments (5)
src/omnibase_infra/runtime/contract_loaders/handler_routing_loader.py (2)
234-239: Consider validatingrouting_strategyvalue before constructing the model.The
ModelRoutingSubcontract.routing_strategyfield is typed asLiteral["payload_type_match"], so Pydantic will raise a validation error if an invalid value is passed. However, the current code passes the raw string from YAML directly. If the contract contains an unsupported strategy (e.g.,"round_robin"), the PydanticValidationErrormay be less informative than a customProtocolConfigurationErrorwith context.This is optional—Pydantic validation will still catch invalid values, but a custom error would provide better context.
139-141: Consider usingFileRegistryfor contract loading consistency.Based on learnings,
FileRegistryfromomnibase_coreis the canonical pattern for loading YAML contracts, providing structured error codes likeFILE_NOT_FOUND,CONFIGURATION_PARSE_ERROR, etc. The current implementation provides equivalent functionality but diverges from the established pattern.This is optional since the current error handling is robust and the learning applies primarily to
omnibase_corefiles.src/omnibase_infra/nodes/node_registration_orchestrator/registry/registry_infra_node_registration_orchestrator.py (2)
385-401: Handler dependencies map requires code changes for new handlers.The
handler_dependenciesmap hardcodes constructor arguments for each handler. Adding a new handler tocontract.yamlrequires updating this map, which partially undermines the "contract-driven" goal.Consider documenting this coupling in the docstring or exploring ways to make dependency wiring more declarative (e.g., defining dependencies in the contract or using inspection). This is optional given the current architecture.
442-446: Filter logic clarification for optional dependencies.The condition
if v is not None or k == "projection_reader"ensuresprojection_readeris always passed even ifNone. However,projection_readeris a required parameter tocreate_registry(notOptional), so it should never beNoneat this point. The defensive check is harmless but the comment could clarify this invariant.tests/unit/runtime/contract_loaders/test_handler_routing_loader.py (1)
132-139: Weak assertion for underscore handling test.The assertion
assert "_" in result or "-" in resultdoesn't definitively verify the expected behavior. If the function's behavior with underscores is implementation-dependent, consider either:
- Pinning down the expected behavior and testing for it explicitly
- Documenting why the behavior is intentionally undefined
♻️ Suggested improvement
def test_underscore_preserved(self) -> None: """Test that underscores are preserved (not converted).""" from omnibase_infra.runtime.contract_loaders import convert_class_to_handler_key - # Underscores are not typical in class names but should be preserved - result = convert_class_to_handler_key("My_Handler") - assert "_" in result or "-" in result # Implementation dependent + # Underscores are not typical in class names; verify actual behavior + result = convert_class_to_handler_key("My_Handler") + # Pin down expected behavior - adjust based on actual implementation + assert result == "my_-handler" or result == "my-_handler", ( + f"Unexpected underscore handling: {result}" + )
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Lite
📒 Files selected for processing (7)
src/omnibase_infra/nodes/node_registration_orchestrator/node.pysrc/omnibase_infra/nodes/node_registration_orchestrator/registry/registry_infra_node_registration_orchestrator.pysrc/omnibase_infra/runtime/contract_loaders/__init__.pysrc/omnibase_infra/runtime/contract_loaders/handler_routing_loader.pytests/unit/runtime/contract_loaders/__init__.pytests/unit/runtime/contract_loaders/conftest.pytests/unit/runtime/contract_loaders/test_handler_routing_loader.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:
tests/unit/runtime/contract_loaders/__init__.pysrc/omnibase_infra/nodes/node_registration_orchestrator/node.pytests/unit/runtime/contract_loaders/test_handler_routing_loader.pysrc/omnibase_infra/runtime/contract_loaders/handler_routing_loader.pytests/unit/runtime/contract_loaders/conftest.pysrc/omnibase_infra/runtime/contract_loaders/__init__.pysrc/omnibase_infra/nodes/node_registration_orchestrator/registry/registry_infra_node_registration_orchestrator.py
**/node.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/node.py: Node files must be namednode.pywith class nameNode<Name><Type>and must be declarative with no custom logic
All nodes must extend base classes fromomnibase_core.nodes(NodeOrchestrator, NodeEffect, NodeCompute, NodeReducer) with container injection and no custom logic in node.py
Files:
src/omnibase_infra/nodes/node_registration_orchestrator/node.py
**/handler_*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Handlers must NOT have direct event bus access - only orchestrators may have bus parameters and publish events
Files:
src/omnibase_infra/runtime/contract_loaders/handler_routing_loader.py
**/registry_*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Registry files must follow naming convention: node-specific registries use
registry_infra_<node_name>.py→RegistryInfra<NodeName>, standalone registries useregistry_<purpose>.py→Registry<Purpose>
Files:
src/omnibase_infra/nodes/node_registration_orchestrator/registry/registry_infra_node_registration_orchestrator.py
🧠 Learnings (18)
📓 Common learnings
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_contract.yaml : Handlers must be discovered and loaded dynamically via YAML contracts using plugin pattern, not hardcoded registries
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: Applies to **/node_*/v[0-9]*_[0-9]*_[0-9]*/contracts/contract_*.yaml : All ONEX node contract definitions must use the new subcontract architecture pattern, breaking down complex contracts into separate contract_actions.yaml, contract_models.yaml, contract_validation.yaml, and optional contract_cli.yaml and contract_capabilities.yaml files for separation of concerns, maintainability, reusability, modularity, and future tool-as-a-service readiness
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_contract.yaml : Handler contract files must declare handler routing with `routing_strategy`, event models, handler classes, and handler modules
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/**/*.py : Use FileRegistry for loading YAML contracts: registry = FileRegistry(); contract = registry.load(Path(...)); handle FileRegistry error codes: FILE_NOT_FOUND, FILE_READ_ERROR, CONFIGURATION_PARSE_ERROR, CONTRACT_VALIDATION_ERROR, DUPLICATE_REGISTRATION, DIRECTORY_NOT_FOUND
📚 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: Applies to **/node_*/v[0-9]*_[0-9]*_[0-9]*/models/model_contract_*.py : All ONEX node auto-generated Pydantic models must be organized in a `models/` directory with files for state.py, model_contract_actions.py, model_contract_models.py, model_contract_validation.py, model_contract_cli.py (optional), model_contract_capabilities.py (optional), and error_codes.py, generated from the corresponding contract definitions
Applied to files:
tests/unit/runtime/contract_loaders/__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: Applies to **/node_*/v[0-9]*_[0-9]*_[0-9]*/node_tests/**/*.py : All ONEX node tests must be organized in a `node_tests/` directory using scenario-driven testing patterns with fixture-injected tests
Applied to files:
tests/unit/runtime/contract_loaders/__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: Applies to **/node_*/v[0-9]*_[0-9]*_[0-9]*/contracts/contract_*.yaml : All ONEX node contract definitions must use the new subcontract architecture pattern, breaking down complex contracts into separate contract_actions.yaml, contract_models.yaml, contract_validation.yaml, and optional contract_cli.yaml and contract_capabilities.yaml files for separation of concerns, maintainability, reusability, modularity, and future tool-as-a-service readiness
Applied to files:
src/omnibase_infra/nodes/node_registration_orchestrator/node.pytests/unit/runtime/contract_loaders/conftest.pysrc/omnibase_infra/nodes/node_registration_orchestrator/registry/registry_infra_node_registration_orchestrator.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: Workflow coordination must be defined in YAML contracts loaded by `NodeAgentOrchestrator`, not as separate Python classes
Applied to files:
src/omnibase_infra/nodes/node_registration_orchestrator/node.pysrc/omnibase_infra/nodes/node_registration_orchestrator/registry/registry_infra_node_registration_orchestrator.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/**/contract.yaml : All contract YAML files for ONEX v2.0 nodes MUST define subcontract references, input/output models, and FSM configurations. Use YAML 1.2 syntax.
Applied to files:
src/omnibase_infra/nodes/node_registration_orchestrator/node.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_contract.yaml : Handler contract files must declare handler routing with `routing_strategy`, event models, handler classes, and handler modules
Applied to files:
src/omnibase_infra/nodes/node_registration_orchestrator/node.pytests/unit/runtime/contract_loaders/test_handler_routing_loader.pysrc/omnibase_infra/runtime/contract_loaders/handler_routing_loader.pysrc/omnibase_infra/runtime/contract_loaders/__init__.pysrc/omnibase_infra/nodes/node_registration_orchestrator/registry/registry_infra_node_registration_orchestrator.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 : Use NodeEffect, NodeCompute, NodeReducer, and NodeOrchestrator base classes with declarative YAML contracts; import from omnibase_core.nodes
Applied to files:
src/omnibase_infra/nodes/node_registration_orchestrator/node.pysrc/omnibase_infra/nodes/node_registration_orchestrator/registry/registry_infra_node_registration_orchestrator.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_contract.yaml : Handlers must be discovered and loaded dynamically via YAML contracts using plugin pattern, not hardcoded registries
Applied to files:
src/omnibase_infra/nodes/node_registration_orchestrator/node.pysrc/omnibase_infra/runtime/contract_loaders/handler_routing_loader.pysrc/omnibase_infra/nodes/node_registration_orchestrator/registry/registry_infra_node_registration_orchestrator.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/reducer/**/*.py : FSM behavior must be defined in YAML contracts, not as multiple Python reducer classes; use one NodeReducer class that loads different FSM contracts
Applied to files:
src/omnibase_infra/nodes/node_registration_orchestrator/node.pysrc/omnibase_infra/nodes/node_registration_orchestrator/registry/registry_infra_node_registration_orchestrator.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/**/*.py : Use FileRegistry for loading YAML contracts: registry = FileRegistry(); contract = registry.load(Path(...)); handle FileRegistry error codes: FILE_NOT_FOUND, FILE_READ_ERROR, CONFIGURATION_PARSE_ERROR, CONTRACT_VALIDATION_ERROR, DUPLICATE_REGISTRATION, DIRECTORY_NOT_FOUND
Applied to files:
src/omnibase_infra/nodes/node_registration_orchestrator/node.pysrc/omnibase_infra/runtime/contract_loaders/handler_routing_loader.pytests/unit/runtime/contract_loaders/conftest.pysrc/omnibase_infra/runtime/contract_loaders/__init__.pysrc/omnibase_infra/nodes/node_registration_orchestrator/registry/registry_infra_node_registration_orchestrator.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: Applies to **/node_*/v[0-9]*_[0-9]*_[0-9]*/contracts/contract_*.yaml : All ONEX node subcontracts must be organized in a `contracts/` subdirectory within the versioned implementation directory with separate files for contract_actions.yaml, contract_models.yaml, contract_validation.yaml, contract_cli.yaml (optional), and contract_capabilities.yaml (optional)
Applied to files:
src/omnibase_infra/nodes/node_registration_orchestrator/node.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: Applies to **/node_*/v[0-9]*_[0-9]*_[0-9]*/contract.yaml : All ONEX node contract definitions must follow the linked document architecture pattern with contract.yaml linking to node_config.yaml and deployment_config.yaml as associated documents
Applied to files:
src/omnibase_infra/nodes/node_registration_orchestrator/node.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/node.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: When both `handler_contract.yaml` and `contract.yaml` exist in the same directory, the loader must raise an error (AMBIGUOUS_CONTRACT_CONFIGURATION)
Applied to files:
src/omnibase_infra/runtime/contract_loaders/handler_routing_loader.pytests/unit/runtime/contract_loaders/conftest.pysrc/omnibase_infra/runtime/contract_loaders/__init__.py
📚 Learning: 2025-11-24T16:33:51.604Z
Learnt from: CR
Repo: OmniNode-ai/omninode_bridge PR: 0
File: .cursor/rules/testing.mdc:0-0
Timestamp: 2025-11-24T16:33:51.604Z
Learning: Applies to tests/**/conftest.py : Test fixtures must be defined in `conftest.py` and should provide reusable sample data, UUIDs, semantic versions, and model data
Applied to files:
tests/unit/runtime/contract_loaders/conftest.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/registry/registry_infra_node_registration_orchestrator.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 **/registry_*.py : Registry files must follow naming convention: node-specific registries use `registry_infra_<node_name>.py` → `RegistryInfra<NodeName>`, standalone registries use `registry_<purpose>.py` → `Registry<Purpose>`
Applied to files:
src/omnibase_infra/nodes/node_registration_orchestrator/registry/registry_infra_node_registration_orchestrator.py
🧬 Code graph analysis (4)
src/omnibase_infra/nodes/node_registration_orchestrator/node.py (2)
src/omnibase_infra/models/routing/model_routing_subcontract.py (1)
ModelRoutingSubcontract(21-67)src/omnibase_infra/runtime/contract_loaders/handler_routing_loader.py (1)
load_handler_routing_subcontract(103-239)
src/omnibase_infra/runtime/contract_loaders/handler_routing_loader.py (5)
src/omnibase_infra/enums/enum_infra_transport_type.py (1)
EnumInfraTransportType(32-60)src/omnibase_infra/models/errors/model_infra_error_context.py (1)
ModelInfraErrorContext(17-96)src/omnibase_infra/errors/error_infra.py (1)
ProtocolConfigurationError(154-189)src/omnibase_infra/models/routing/model_routing_entry.py (1)
ModelRoutingEntry(14-49)src/omnibase_infra/models/routing/model_routing_subcontract.py (1)
ModelRoutingSubcontract(21-67)
src/omnibase_infra/runtime/contract_loaders/__init__.py (1)
src/omnibase_infra/runtime/contract_loaders/handler_routing_loader.py (2)
convert_class_to_handler_key(77-100)load_handler_routing_subcontract(103-239)
src/omnibase_infra/nodes/node_registration_orchestrator/registry/registry_infra_node_registration_orchestrator.py (8)
src/omnibase_infra/models/errors/model_infra_error_context.py (1)
ModelInfraErrorContext(17-96)src/omnibase_infra/enums/enum_infra_transport_type.py (1)
EnumInfraTransportType(32-60)src/omnibase_infra/errors/error_infra.py (1)
ProtocolConfigurationError(154-189)src/omnibase_infra/runtime/projector_plugin_loader.py (1)
contract(123-125)src/omnibase_infra/runtime/projector_shell.py (1)
contract(229-231)tests/integration/projectors/conftest.py (2)
contract(287-330)projector(201-230)tests/integration/registration/e2e/conftest.py (1)
projection_reader(363-378)tests/unit/handlers/test_service_discovery_protocol_compliance.py (1)
consul_handler(79-89)
🔇 Additional comments (17)
src/omnibase_infra/runtime/contract_loaders/handler_routing_loader.py (1)
77-100: LGTM! Well-documented CamelCase to kebab-case conversion.The regex-based conversion handles edge cases like consecutive uppercase letters (
MyHTTPHandler→my-http-handler) correctly. The docstring examples are helpful.tests/unit/runtime/contract_loaders/__init__.py (1)
1-12: LGTM!Clean test package initializer with appropriate documentation.
src/omnibase_infra/runtime/contract_loaders/__init__.py (1)
1-36: LGTM!Clean package initialization with appropriate re-exports and well-documented usage examples.
src/omnibase_infra/nodes/node_registration_orchestrator/node.py (2)
70-88: LGTM! Clean refactor to shared utility.The thin wrapper pattern preserves the existing function signature while delegating to the shared loader. This maintains backward compatibility and follows the DRY principle effectively.
61-62: Good import organization.The imports from the new shared contract loader module are correctly added, enabling the delegation pattern.
src/omnibase_infra/nodes/node_registration_orchestrator/registry/registry_infra_node_registration_orchestrator.py (2)
112-160: Well-implemented dynamic handler class loading.Good separation of
ModuleNotFoundErrorandImportErrorfor clearer error messages. The use ofEnumInfraTransportType.RUNTIMEis semantically correct for runtime module loading.
403-484: Good dynamic handler loading implementation with proper validation.The implementation correctly:
- Handles the heartbeat handler special case based on projector availability
- Validates protocol compliance via duck typing (per ONEX conventions)
- Provides detailed error messages for instantiation failures
- Logs handler registration at DEBUG level
The fail-fast pattern at lines 363-377 combined with the skip logic at lines 410-420 provides clear production vs. testing behavior.
tests/unit/runtime/contract_loaders/conftest.py (4)
1-14: LGTM!Clean header, appropriate imports, and good module documentation referencing the ticket number for traceability.
20-127: LGTM!Excellent coverage of test scenarios with well-documented YAML constants. The samples appropriately cover valid configurations, edge cases (empty handlers, incomplete entries), and error conditions (invalid YAML, missing sections). Based on learnings, the handler contract structure with
routing_strategy, event models, and handler declarations aligns with expected contract patterns.
135-238: LGTM!Well-structured fixtures with clear docstrings, proper use of
tmp_pathfor test isolation, and explicit return type annotations. Thenonexistent_contract_pathfixture correctly returns a path that won't exist without creating the parent directory.
245-267: LGTM!Comprehensive
__all__exports with clear categorization. Note that pytest auto-discovers fixtures fromconftest.py, so including them in__all__is primarily for documentation purposes, which is fine.tests/unit/runtime/contract_loaders/test_handler_routing_loader.py (6)
1-34: LGTM!Excellent module documentation with clear test categorization, usage examples, and ticket reference. The imports are minimal and appropriate, using relative imports for conftest constants.
146-258: LGTM!Comprehensive happy path tests covering valid contracts, minimal configurations, default values, and handler key conversion. The tests properly verify the
ModelRoutingSubcontractstructure and routing key mappings.
265-373: LGTM!Thorough error handling tests that verify appropriate
ProtocolConfigurationErrorexceptions are raised with informative messages. Good verification of error context for debugging purposes.
380-544: LGTM!Excellent edge case coverage including graceful handling of incomplete entries, default values, and both absolute and relative path support. Good use of
caplogto verify warning logs for skipped entries andmonkeypatch.chdirfor relative path testing.
551-599: LGTM!Solid integration tests that verify the loader works with the real orchestrator contract. The handler key validation assertions (lines 586-598) effectively enforce naming conventions: kebab-case, lowercase, and hyphen-prefixed. The flexible assertion
>= 4appropriately accommodates future handler additions.
605-611: LGTM!The
__all__exports document the test classes, though pytest auto-discovers test classes regardless. This is consistent with the pattern used inconftest.py.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
…ading [OMN-1316] Address PR #153 review feedback: Security (CRITICAL): - Add ALLOWED_NAMESPACES constant for trusted module namespaces - Validate module paths before dynamic import in _load_handler_class - Reject untrusted namespaces with NAMESPACE_NOT_ALLOWED error code Code Quality (MAJOR): - Extract load_handler_class_info_from_contract() to shared loader - Remove duplicate _load_handler_routing_from_contract from registry - Change transport type from DATABASE to FILESYSTEM for file operations - Improve error messages to be actionable with fix guidance Validation (NITPICK): - Add VALID_ROUTING_STRATEGIES constant with validation - Clarify filter logic comments for optional dependencies - Strengthen weak assertion in underscore handling test
PR Review: Handler Routing Loader Refactoring [OMN-1316]SummaryThis PR successfully extracts handler routing loading logic into a shared utility, implementing the Handler Plugin Loader pattern. The refactoring eliminates code duplication and establishes contract-driven handler loading as a first-class pattern. Overall, this is high-quality work with excellent test coverage (625 lines of tests for 406 lines of production code). ✅ StrengthsArchitecture & Design
Code Quality
Testing
🔴 Critical Issues1. Transport Type Mismatch in Registry
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/omnibase_infra/runtime/contract_loaders/handler_routing_loader.py`:
- Around line 76-83: VALID_ROUTING_STRATEGIES includes unimplemented
"first_match" and "all_match"; change the constant VALID_ROUTING_STRATEGIES to
only include "payload_type_match" so it matches the implemented behavior and the
type on ModelRoutingSubcontract.routing_strategy
(Literal["payload_type_match"]), and update any loader validation that
references VALID_ROUTING_STRATEGIES to rely solely on that single allowed value.
🧹 Nitpick comments (3)
src/omnibase_infra/nodes/node_registration_orchestrator/registry/registry_infra_node_registration_orchestrator.py (1)
328-346: Handler dependency map creates partial coupling to contract.yaml.While the handler classes are now loaded dynamically from
contract.yaml, thehandler_dependenciesmap still requires code changes when adding new handlers. This is acceptable given the PR's stated approach to "preserve DI/constructor parameters" without making contracts a secondary DI container. However, consider adding a comment noting that new handlers require an entry here.💡 Suggested documentation improvement
# Map of handler dependencies by handler class name # Each handler class has specific dependencies based on its constructor + # NOTE: When adding a new handler to contract.yaml, also add an entry here + # with the handler's required constructor arguments. handler_dependencies: dict[str, dict[str, object]] = {tests/unit/runtime/contract_loaders/test_handler_routing_loader.py (1)
132-153: Underscore test documents surprising but correct behavior.The test correctly documents that
convert_class_to_handler_key("My_Handler")produces"my_-handler"due to how the regex operates on case boundaries. While technically correct, this produces suboptimal output. Since underscores in Python class names are atypical (as the comment notes), this is acceptable, but if underscore-named classes ever appear in contracts, you may want to sanitize the output.💡 Optional: Clean up double punctuation in output
In
convert_class_to_handler_key, you could add a post-processing step:# After the regex transformations: return result.replace("_-", "-").replace("-_", "-")src/omnibase_infra/runtime/contract_loaders/handler_routing_loader.py (1)
270-398: Consider reducing duplication between loader functions.
load_handler_class_info_from_contractduplicates the YAML loading and validation logic fromload_handler_routing_subcontract. While this is acceptable for clarity, a shared internal helper could reduce maintenance burden if the contract format evolves.Additionally, the return type
list[dict[str, str]]could benefit from aTypedDictfor better IDE support and type safety.💡 Optional: Use TypedDict for return type
from typing import TypedDict class HandlerClassInfo(TypedDict): handler_class: str handler_module: str def load_handler_class_info_from_contract( contract_path: Path, ) -> list[HandlerClassInfo]: ...
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Lite
📒 Files selected for processing (4)
src/omnibase_infra/nodes/node_registration_orchestrator/registry/registry_infra_node_registration_orchestrator.pysrc/omnibase_infra/runtime/contract_loaders/__init__.pysrc/omnibase_infra/runtime/contract_loaders/handler_routing_loader.pytests/unit/runtime/contract_loaders/test_handler_routing_loader.py
🧰 Additional context used
📓 Path-based instructions (3)
**/*.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/contract_loaders/__init__.pysrc/omnibase_infra/nodes/node_registration_orchestrator/registry/registry_infra_node_registration_orchestrator.pysrc/omnibase_infra/runtime/contract_loaders/handler_routing_loader.pytests/unit/runtime/contract_loaders/test_handler_routing_loader.py
**/registry_*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Registry files must follow naming convention: node-specific registries use
registry_infra_<node_name>.py→RegistryInfra<NodeName>, standalone registries useregistry_<purpose>.py→Registry<Purpose>
Files:
src/omnibase_infra/nodes/node_registration_orchestrator/registry/registry_infra_node_registration_orchestrator.py
**/handler_*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Handlers must NOT have direct event bus access - only orchestrators may have bus parameters and publish events
Files:
src/omnibase_infra/runtime/contract_loaders/handler_routing_loader.py
🧠 Learnings (18)
📓 Common learnings
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_contract.yaml : Handlers must be discovered and loaded dynamically via YAML contracts using plugin pattern, not hardcoded registries
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: Applies to **/node_*/v[0-9]*_[0-9]*_[0-9]*/contracts/contract_*.yaml : All ONEX node contract definitions must use the new subcontract architecture pattern, breaking down complex contracts into separate contract_actions.yaml, contract_models.yaml, contract_validation.yaml, and optional contract_cli.yaml and contract_capabilities.yaml files for separation of concerns, maintainability, reusability, modularity, and future tool-as-a-service readiness
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/**/*.py : Use FileRegistry for loading YAML contracts: registry = FileRegistry(); contract = registry.load(Path(...)); handle FileRegistry error codes: FILE_NOT_FOUND, FILE_READ_ERROR, CONFIGURATION_PARSE_ERROR, CONTRACT_VALIDATION_ERROR, DUPLICATE_REGISTRATION, DIRECTORY_NOT_FOUND
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_contract.yaml : Handler contract files must declare handler routing with `routing_strategy`, event models, handler classes, and handler modules
Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: .cursor/rules/canonical_patterns.mdc:0-0
Timestamp: 2025-11-24T17:22:32.195Z
Learning: Applies to **/v[0-9]_[0-9]_[0-9]/contracts/ : Organize contract subcomponents into separate files (contract_actions.yaml, contract_models.yaml, contract_validation.yaml, etc.) and reference them from main contract.yaml
Learnt from: CR
Repo: OmniNode-ai/omniagent PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T22:21:32.649Z
Learning: Workflow coordination must be defined in YAML contracts loaded by `NodeAgentOrchestrator`, not as separate Python classes
Learnt from: CR
Repo: OmniNode-ai/omnibase_infra PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T17:31:33.550Z
Learning: When both `handler_contract.yaml` and `contract.yaml` exist in the same directory, the loader must raise an error (AMBIGUOUS_CONTRACT_CONFIGURATION)
📚 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/**/*.py : Use FileRegistry for loading YAML contracts: registry = FileRegistry(); contract = registry.load(Path(...)); handle FileRegistry error codes: FILE_NOT_FOUND, FILE_READ_ERROR, CONFIGURATION_PARSE_ERROR, CONTRACT_VALIDATION_ERROR, DUPLICATE_REGISTRATION, DIRECTORY_NOT_FOUND
Applied to files:
src/omnibase_infra/runtime/contract_loaders/__init__.pysrc/omnibase_infra/nodes/node_registration_orchestrator/registry/registry_infra_node_registration_orchestrator.pysrc/omnibase_infra/runtime/contract_loaders/handler_routing_loader.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_contract.yaml : Handler contract files must declare handler routing with `routing_strategy`, event models, handler classes, and handler modules
Applied to files:
src/omnibase_infra/runtime/contract_loaders/__init__.pysrc/omnibase_infra/nodes/node_registration_orchestrator/registry/registry_infra_node_registration_orchestrator.pysrc/omnibase_infra/runtime/contract_loaders/handler_routing_loader.pytests/unit/runtime/contract_loaders/test_handler_routing_loader.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: When both `handler_contract.yaml` and `contract.yaml` exist in the same directory, the loader must raise an error (AMBIGUOUS_CONTRACT_CONFIGURATION)
Applied to files:
src/omnibase_infra/runtime/contract_loaders/__init__.pysrc/omnibase_infra/nodes/node_registration_orchestrator/registry/registry_infra_node_registration_orchestrator.pysrc/omnibase_infra/runtime/contract_loaders/handler_routing_loader.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_contract.yaml : Handlers must be discovered and loaded dynamically via YAML contracts using plugin pattern, not hardcoded registries
Applied to files:
src/omnibase_infra/nodes/node_registration_orchestrator/registry/registry_infra_node_registration_orchestrator.pysrc/omnibase_infra/runtime/contract_loaders/handler_routing_loader.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/registry/registry_infra_node_registration_orchestrator.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 : Use NodeEffect, NodeCompute, NodeReducer, and NodeOrchestrator base classes with declarative YAML contracts; import from omnibase_core.nodes
Applied to files:
src/omnibase_infra/nodes/node_registration_orchestrator/registry/registry_infra_node_registration_orchestrator.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 **/registry_*.py : Registry files must follow naming convention: node-specific registries use `registry_infra_<node_name>.py` → `RegistryInfra<NodeName>`, standalone registries use `registry_<purpose>.py` → `Registry<Purpose>`
Applied to files:
src/omnibase_infra/nodes/node_registration_orchestrator/registry/registry_infra_node_registration_orchestrator.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/registry/registry_infra_node_registration_orchestrator.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: Workflow coordination must be defined in YAML contracts loaded by `NodeAgentOrchestrator`, not as separate Python classes
Applied to files:
src/omnibase_infra/nodes/node_registration_orchestrator/registry/registry_infra_node_registration_orchestrator.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: Applies to **/node_*/v[0-9]*_[0-9]*_[0-9]*/contracts/contract_*.yaml : All ONEX node contract definitions must use the new subcontract architecture pattern, breaking down complex contracts into separate contract_actions.yaml, contract_models.yaml, contract_validation.yaml, and optional contract_cli.yaml and contract_capabilities.yaml files for separation of concerns, maintainability, reusability, modularity, and future tool-as-a-service readiness
Applied to files:
src/omnibase_infra/nodes/node_registration_orchestrator/registry/registry_infra_node_registration_orchestrator.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/reducer/**/*.py : FSM behavior must be defined in YAML contracts, not as multiple Python reducer classes; use one NodeReducer class that loads different FSM contracts
Applied to files:
src/omnibase_infra/nodes/node_registration_orchestrator/registry/registry_infra_node_registration_orchestrator.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 **/v[0-9]_[0-9]_[0-9]/contract.yaml : The main contract.yaml file serves as the source of truth for node interfaces and should reference subcontracts using $ref patterns
Applied to files:
src/omnibase_infra/nodes/node_registration_orchestrator/registry/registry_infra_node_registration_orchestrator.py
📚 Learning: 2025-11-24T17:22:32.195Z
Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: .cursor/rules/canonical_patterns.mdc:0-0
Timestamp: 2025-11-24T17:22:32.195Z
Learning: Applies to **/v[0-9]_[0-9]_[0-9]/contracts/ : Organize contract subcomponents into separate files (contract_actions.yaml, contract_models.yaml, contract_validation.yaml, etc.) and reference them from main contract.yaml
Applied to files:
src/omnibase_infra/nodes/node_registration_orchestrator/registry/registry_infra_node_registration_orchestrator.py
📚 Learning: 2025-11-24T17:22:32.195Z
Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: .cursor/rules/canonical_patterns.mdc:0-0
Timestamp: 2025-11-24T17:22:32.195Z
Learning: Applies to **/v[0-9]_[0-9]_[0-9]/contract.yaml : Use shared schema references with project root paths in contract definitions (e.g., schemas/onex_field_model.schema.yaml, schemas/semver_model.schema.yaml)
Applied to files:
src/omnibase_infra/nodes/node_registration_orchestrator/registry/registry_infra_node_registration_orchestrator.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/protocols/handlers/**/*.py : Protocol naming convention: Handler protocols must follow `Protocol{Type}Handler` pattern
Applied to files:
src/omnibase_infra/nodes/node_registration_orchestrator/registry/registry_infra_node_registration_orchestrator.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/nodes/node_registration_orchestrator/registry/registry_infra_node_registration_orchestrator.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 `InfraConnectionError`, `InfraTimeoutError`, `InfraAuthenticationError`, `InfraUnavailableError` for transport failures with proper `ModelInfraErrorContext`
Applied to files:
src/omnibase_infra/runtime/contract_loaders/handler_routing_loader.py
🧬 Code graph analysis (3)
src/omnibase_infra/runtime/contract_loaders/__init__.py (1)
src/omnibase_infra/runtime/contract_loaders/handler_routing_loader.py (3)
convert_class_to_handler_key(86-109)load_handler_class_info_from_contract(270-398)load_handler_routing_subcontract(112-267)
src/omnibase_infra/runtime/contract_loaders/handler_routing_loader.py (5)
src/omnibase_infra/enums/enum_infra_transport_type.py (1)
EnumInfraTransportType(32-60)src/omnibase_infra/models/errors/model_infra_error_context.py (1)
ModelInfraErrorContext(17-96)src/omnibase_infra/errors/error_infra.py (1)
ProtocolConfigurationError(154-189)src/omnibase_infra/models/routing/model_routing_entry.py (1)
ModelRoutingEntry(14-49)src/omnibase_infra/models/routing/model_routing_subcontract.py (1)
ModelRoutingSubcontract(21-67)
tests/unit/runtime/contract_loaders/test_handler_routing_loader.py (4)
src/omnibase_infra/runtime/contract_loaders/handler_routing_loader.py (2)
convert_class_to_handler_key(86-109)load_handler_routing_subcontract(112-267)src/omnibase_infra/models/routing/model_routing_subcontract.py (1)
ModelRoutingSubcontract(21-67)src/omnibase_infra/errors/error_infra.py (1)
ProtocolConfigurationError(154-189)src/omnibase_infra/nodes/node_registration_orchestrator/node.py (1)
_create_handler_routing_subcontract(70-88)
🔇 Additional comments (11)
src/omnibase_infra/runtime/contract_loaders/__init__.py (1)
1-40: Well-structured package initializer with clean public API exposure.The module correctly re-exports the shared utilities from
handler_routing_loader, provides clear documentation with usage examples, and maintains a consistent__all__declaration. This aligns with the PR objective to exposeload_handler_routing_subcontractas a shared utility and follows the Handler Plugin Loader pattern from learnings.src/omnibase_infra/nodes/node_registration_orchestrator/registry/registry_infra_node_registration_orchestrator.py (4)
70-77: Good security practice with namespace allowlisting.The
ALLOWED_NAMESPACEStuple provides defense-in-depth against arbitrary code execution via maliciouscontract.yamlconfigurations. The documentation clearly references the security pattern and error code.
143-156: Namespace validation is secure but consider edge case.The namespace check using
startswith()is appropriate. However, be aware that paths likeomnibase_infra_malicious.evilwould pass validation since they start withomnibase_infra. The current implementation is safe because Python's import system would reject such module names if they don't exist, and the allowlist is restrictive enough. The error message correctly includes the error code for traceability.
399-412: Solid error handling for handler instantiation failures.Catching
TypeErrorspecifically handles constructor signature mismatches from**filtered_depsunpacking. The error message is actionable, directing developers to check thehandler_dependenciesmap.
393-397: Clarify why projection_reader requires special handling in filtered_deps.The filtering logic intentionally keeps
projection_readereven whenNone(line 396:if v is not None or k == "projection_reader"), while filtering otherNonevalues. This special case suggestsprojection_readercan beNone, but if the parameter type annotation doesn't include| None, this inconsistency could be confusing. Either the type annotation should be updated to reflect thatprojection_readeracceptsNone, or the reason for the special case should be documented with a clarifying comment in the code.tests/unit/runtime/contract_loaders/test_handler_routing_loader.py (3)
160-272: Comprehensive happy path test coverage.Tests cover the essential success scenarios: valid contracts, minimal contracts, default values, handler key conversion, and empty handler lists. The fixture-based approach keeps tests clean and maintainable.
394-558: Thorough edge case coverage with appropriate test utilities.The edge case tests properly use
caplogfor warning verification, inline YAML for specific scenarios, andmonkeypatch.chdirfor relative path testing. The skipped entry tests correctly verify graceful degradation behavior.
565-612: Valuable integration tests against real contract structure.Testing against the actual
node_registration_orchestratorcontract validates that the loader works with production data. The handler key format assertions (lowercase, kebab-case, "handler-" prefix) document and enforce naming conventions.src/omnibase_infra/runtime/contract_loaders/handler_routing_loader.py (3)
86-109: Correct CamelCase to kebab-case conversion.The two-step regex approach properly handles standard CamelCase, acronyms like
HTTP, and numeric boundaries. The docstring examples accurately reflect the output.
148-267: Well-implemented contract loader with comprehensive error handling.The function correctly:
- Uses
FILESYSTEMtransport type for all error contexts (addressing past review feedback)- Sanitizes YAML errors to avoid leaking file contents (line 173)
- Gracefully skips malformed entries with warnings
- Validates and defaults
routing_strategy
401-406: Consistent public API exports.The
__all__list matches the re-exports in__init__.py, maintaining a clear and consistent public API surface.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
- Remove unimplemented routing strategies (first_match, all_match) from VALID_ROUTING_STRATEGIES constant, keeping only payload_type_match - Add file size limit enforcement (10MB max) to prevent memory exhaustion - Reduce code duplication by extracting _load_and_validate_contract_yaml helper - Fix transport type: DATABASE → RUNTIME for handler config validation - Add TestValidRoutingStrategies with 12 tests for strategy validation - Add TestFileSizeEnforcement with 11 tests for security controls - Refactor underscore handling test to parametrized with 7 edge cases Note: Pre-commit hook failure is for unrelated file (projector_plugin_loader.py:241)
Code Review - PR #153: Contract-Driven Handler Loading (OMN-1316)SummaryThis PR successfully extracts handler routing loader logic to a shared utility, implementing the Handler Plugin Loader pattern. The refactoring eliminates code duplication and establishes a consistent contract-driven approach for orchestrator handler loading. ✅ Strengths1. Excellent Architecture & Design
2. Security-First Implementation
3. Robust Error Handling
4. Comprehensive Testing
5. Code Quality
🔍 Observations & Suggestions1. Handler Dependencies Map (Minor) - registry_infra_node_registration_orchestrator.py:328-346Current Implementation: handler_dependencies: dict[str, dict[str, object]] = {
"HandlerNodeIntrospected": {
"projection_reader": projection_reader,
"projector": projector,
"consul_handler": consul_handler,
},
# ... 3 more handlers
}Observation: The Suggestions:
Priority: Low (current approach is pragmatic and well-documented) 2. Dependency Filtering Logic - registry_infra_node_registration_orchestrator.py:384-397Current Implementation: filtered_deps = {
k: v
for k, v in deps.items()
if v is not None or k == "projection_reader"
}Observation: The special case for Suggestions:
Priority: Low (current implementation works, but could be more maintainable) 3. File Size Check Timing - handler_routing_loader.py:82-120Current Implementation: def _check_file_size(contract_path: Path, operation: str) -> None:
try:
file_size = contract_path.stat().st_size
except FileNotFoundError:
return # Let caller handleObservation: File size check happens before opening the file, which is good. However, Suggestions:
Priority: Very Low (current approach is reasonable, just a style consideration) 4. Protocol Validation - registry_infra_node_registration_orchestrator.py:80-120Excellent Implementation: The Suggestion: Consider extracting this to a shared utility ( Priority: Very Low (nice-to-have for future reuse) 5. Routing Strategy Validation - handler_routing_loader.py:227-231Current Implementation: VALID_ROUTING_STRATEGIES: frozenset[str] = frozenset({
"payload_type_match", # Only implemented strategy
})Observation: The code gracefully handles unknown strategies by logging a warning and falling back to Suggestion: Consider if fail-fast (raise error) would be better for production deployments to catch misconfigurations early. Current approach is more forgiving but might hide contract typos. Priority: Very Low (current approach is reasonable, discuss with team) 🛡️ Security Review✅ Security Controls Properly Implemented
|
| Metric | Value |
|---|---|
| Files Changed | 7 |
| Lines Added | +2,158 |
| Lines Deleted | -236 |
| Net Change | +1,922 |
| New Tests | 35 |
| Test Files | 3 |
Impact: Medium (refactoring improves maintainability, no functional changes to runtime behavior)
Excellent work on this refactoring! The code quality is very high and the architecture is sound. 🎉
…-1316] Addresses all PR review issues including critical, major, minor, and nitpicks: - Add error codes to all error messages (HANDLER_LOADER_010-023) - Document handler_dependencies maintenance requirements - Clarify filter logic for required vs optional dependencies - Add FileRegistry pattern rationale note - Enhance underscore handling test as characterization test - Add security note for file size check timing All 88 tests pass (64 contract loader + 24 orchestrator).
PR Review: Handler Routing Loader Extraction (OMN-1316)SummaryThis PR successfully extracts handler routing logic into a shared utility and implements contract-driven handler loading, eliminating the dual source of truth between ✅ Strengths1. Excellent Architecture & Pattern Adherence
2. Security-First ImplementationPer CLAUDE.md Handler Plugin Loader security patterns, the implementation includes: ✅ YAML Safe Loading: Uses Security Note: The namespace allowlisting is a critical defense-in-depth control. Consider documenting in the module docstring that this is REQUIRED for production deployments (not optional). 3. Exceptional Test Coverage
4. Error Handling ExcellenceAll error paths include:
5. Strong Type Safety
6. Documentation Quality
🔍 Issues & Recommendations1. CRITICAL: Maintenance Burden - Two-Location Handler RegistrationIssue: When adding a new handler to Current Code (registry line 347): handler_dependencies: dict[str, dict[str, object]] = {
"HandlerNodeIntrospected": {
"projection_reader": projection_reader,
"projector": projector,
"consul_handler": consul_handler,
},
# ... 3 more handlers
}Problem:
Recommendation: Consider one of these approaches: Option A: Contract-Driven Dependencies (Recommended) handler_routing:
handlers:
- event_model:
name: "ModelNodeIntrospectionEvent"
handler:
name: "HandlerNodeIntrospected"
module: "omnibase_infra.nodes..."
dependencies: # NEW
- projection_reader
- projector
- consul_handlerThen resolve dependencies dynamically in the registry: deps = {
dep_name: globals()[dep_name] # Or use a dependency resolver
for dep_name in handler_config.get("dependencies", [])
}Option B: Convention-Based Injection import inspect
sig = inspect.signature(handler_cls.__init__)
deps = {
param: available_deps.get(param)
for param in sig.parameters if param \!= "self"
}Option C: Fail-Fast Validation (Minimum Viable Fix) # In scripts/validate.py
def validate_handler_dependencies():
contract_handlers = load_handler_class_info_from_contract(path)
registry_deps = RegistryInfraNodeRegistrationOrchestrator._get_handler_deps()
missing = set(contract_handlers) - set(registry_deps.keys())
if missing:
raise ValidationError(f"Missing handler_dependencies: {missing}")Impact: Medium-High. This issue will surface during development (runtime error), but could be prevented entirely with contract-driven dependencies. 2. MINOR: Inconsistent Transport Type for Runtime OperationsIssue: The registry uses Current:
Recommendation: Standardize on Proposed Mapping: # File operations (contract loading)
transport_type=EnumInfraTransportType.FILESYSTEM
# Handler operations (import, instantiation, validation)
transport_type=EnumInfraTransportType.RUNTIMEImpact: Low. Error context is already clear from operation names. 3. MINOR: Optional Dependencies Filtering Logic Could Be SimplifiedIssue: The dependency filtering logic (registry lines 422-426) uses a complex conditional: filtered_deps = {
k: v
for k, v in deps.items()
if v is not None or k == "projection_reader"
}While the comment explains the "why" (lines 402-421), the condition Recommendation: Make the intent more explicit: # Always include projection_reader (required), exclude other None values
filtered_deps = {
k: v
for k, v in deps.items()
if k == "projection_reader" or v is not None
}Or use a whitelist approach: REQUIRED_DEPS = {"projection_reader"}
filtered_deps = {
k: v
for k, v in deps.items()
if k in REQUIRED_DEPS or v is not None
}Impact: Very Low. Current code works correctly and is well-commented. 4. MINOR: Missing Docstring for
|
| Aspect | Rating | Notes |
|---|---|---|
| Architecture | ⭐⭐⭐⭐⭐ | Excellent separation, shared utility pattern |
| Security | ⭐⭐⭐⭐⭐ | Defense-in-depth, namespace allowlisting, validation |
| Type Safety | ⭐⭐⭐⭐⭐ | Zero Any types, proper return types |
| Error Handling | ⭐⭐⭐⭐⭐ | Comprehensive codes, actionable messages |
| Documentation | ⭐⭐⭐⭐☆ | Good overall, minor gaps noted above |
| Test Coverage | ⭐⭐⭐⭐⭐ | 1,096 test lines, comprehensive scenarios |
| Maintenance | ⭐⭐⭐☆☆ | Two-location handler registration (Issue #1) |
✅ CLAUDE.md Compliance Checklist
| Rule | Status | Notes |
|---|---|---|
Zero Any types |
✅ | Enforced by CI |
| PEP 604 unions | ✅ | Uses X | None throughout |
| Container DI | ✅ | Uses ModelONEXContainer pattern |
| Protocol validation | ✅ | Duck typing via _validate_handler_protocol |
| OnexError hierarchy | ✅ | Uses ProtocolConfigurationError with context |
| No backwards compat | ✅ | Breaking changes acceptable |
| Declarative nodes | ✅ | Node is thin wrapper to shared loader |
| Security patterns | ✅ | Namespace allowlisting, file size limits |
| Error sanitization | ✅ | No raw YAML errors (line 191) |
🎯 Verdict
Recommendation: APPROVE with minor suggestions
This PR demonstrates excellent engineering practices:
- Strong architectural patterns
- Comprehensive security controls
- Exceptional test coverage
- Clear documentation
The only medium-priority issue (#1 - two-location handler registration) is a maintenance concern rather than a functional bug. It can be addressed in a follow-up PR without blocking this change.
Suggested Follow-Up Tickets:
- OMN-XXXX: Contract-driven handler dependency injection (eliminates
handler_dependenciesdict) - OMN-XXXX: CI contract validation job
- OMN-XXXX: Standardize transport type usage across error contexts
🙏 Kudos
Outstanding work on:
- Security-first design with defense-in-depth controls
- Comprehensive testing (35+ test cases, 1,096 lines)
- Clear error codes (HANDLER_LOADER_010-050)
- Detailed inline comments explaining "why" not just "what"
This PR sets a high standard for contract-driven infrastructure! 🚀
- Add comprehensive documentation for handler dependency map design trade-off - Add error codes to all ProtocolConfigurationError messages - Enhance error messages with actionable fix instructions - Add TODO for future FileRegistry refactoring - Document routing strategy validation order (before model construction) - Add tests verifying VALID_ROUTING_STRATEGIES is used in validation - Enhance underscore test with characterization test documentation - Add error handling tests for edge cases
Code Review: Handler Routing Loader Refactor (OMN-1316)SummaryThis PR successfully extracts handler routing loader functionality into a shared utility, eliminating code duplication and establishing a contract-driven handler loading pattern. The implementation is well-architected with comprehensive error handling, security controls, and excellent test coverage (1441 lines of tests for 464 lines of production code). ✅ Strengths1. Excellent Architecture & Design
2. Security Implementation (Aligns with CLAUDE.md)
3. Error Handling Excellence
4. Test Coverage
5. Documentation
6. Code Quality
🔍 Minor Issues & Suggestions1. Dependency Filtering Logic ClarityFile: registry_infra_node_registration_orchestrator.py:456-461 The comment could be clearer about why projection_reader=None is intentionally passed:
Suggestion: Clarify that None is passed so handlers can validate and raise clear errors. 2. Contract File Ambiguity CheckFile: handler_routing_loader.py:424-426 CLAUDE.md mentions FAIL-FAST behavior for ambiguous contract configurations (HANDLER_LOADER_040) when both handler_contract.yaml and contract.yaml exist in the same directory. Question: Is this ambiguity check implemented in the shared loader? Verify if this check is needed for this loader or if it only applies to a different loader pattern. 3. TODO Comment Needs TrackingFile: handler_routing_loader.py:139-145 The TODO mentions refactoring to use RegistryFileBased once available in omnibase_core. Suggestion: Create a tracking ticket and reference it in the comment to make this technical debt actionable. 📊 Code Metrics
🔒 Security Review✅ Implemented Controls
|
- Clarify projection_reader dependency comment to explain intentional None passing for handler-level validation and fail-fast behavior - Add OMN-1352 ticket reference to TODO comments for RegistryFileBased refactoring technical debt tracking - Add validation exemption for ProjectorPluginLoader.__init__ params
…tead-of-programmatic Resolved conflicts: - node.py: Keep refactored version using shared load_handler_routing_subcontract() - registry_infra_node_registration_orchestrator.py: Keep contract-driven handler loading - validation_exemptions.yaml: Merge exemption entries, keeping cleaner documentation
Code Review: Handler Routing Loader Refactor [OMN-1316]SummaryThis PR successfully extracts handler routing logic into a shared utility module, eliminating code duplication and establishing a contract-driven pattern for handler loading. The implementation is well-architected with strong security controls and comprehensive testing. ✅ Strengths1. Excellent Architecture & Design
2. Robust Error Handling
3. Security ControlsStrong adherence to CLAUDE.md security patterns:
4. Comprehensive Testing
5. Documentation Quality
🔍 Observations & Considerations1. Intentional Design Trade-offs (Well-Justified)The Pros:
Cons:
Verdict: ✅ Acceptable trade-off - The fail-fast error on missing dependency config ( 2. Handler Dependency Filtering LogicLines 442-456 in filtered_deps = {
k: v
for k, v in deps.items()
if v is not None or k == "projection_reader"
}Why
Suggestion: Consider adding a runtime assertion or debug log when 3. Security: Namespace AllowlistingThe ALLOWED_NAMESPACES: tuple[str, ...] = (
"omnibase_infra.",
"omnibase_core.",
)Considerations:
4. Contract File Size Limit10MB limit for contract files (line 79): MAX_CONTRACT_FILE_SIZE_BYTES: int = 10 * 1024 * 1024 # 10MBAnalysis:
🐛 Potential Issues (Minor)1. Missing Import in Registry Example DocstringLine 419 in handler_infos = load_handler_class_info_from_contract(contract_path)
for info in handler_infos:
handler_cls = importlib.import_module(info["handler_module"]) # ← This is wrong
handler = getattr(handler_cls, info["handler_class"])Issue: module = importlib.import_module(info["handler_module"])
handler_cls = getattr(module, info["handler_class"])Impact: Low (documentation only, doesn't affect runtime behavior) 2. Potential Race Condition in Handler RegistrationLines 360-400 in The handler instantiation loop doesn't appear to have thread-safety controls. If
Verdict: ✅ Thread-safe - No actual issue here. 3. Heartbeat Handler Special CaseLines 369-380 handle the special case where if handler_class_name == "HandlerNodeHeartbeat":
if projector is None:
logger.warning(
"HandlerNodeHeartbeat NOT registered: require_heartbeat_handler=False. "
"This creates a contract.yaml mismatch (4 handlers defined, only 3 registered). "
# ...
)
continueAnalysis:
🚀 Performance ConsiderationsContract Loading Performance
No performance concerns identified. 🔐 Security AssessmentThreat Model Coverage
Security posture: STRONG ✅ 📊 Test Coverage AnalysisTest File Structure
Test-to-code ratio: 3.9:1 (excellent coverage) Test Categories
Test coverage: EXCELLENT ✅ 📝 RecommendationsHigh Priority
Medium Priority
Low Priority (Future Enhancements)
✅ Final VerdictAPPROVE with minor documentation fix recommended. Alignment with CLAUDE.md
Code Quality Metrics
🎯 SummaryThis is a high-quality refactor that successfully:
The single documentation fix is minor and doesn't affect runtime behavior. The intentional design trade-offs (explicit dependency wiring, manual maintenance) are well-justified and properly documented. Recommendation: Merge after fixing the docstring example. Great work! 🚀 |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In
`@src/omnibase_infra/nodes/node_registration_orchestrator/registry/registry_infra_node_registration_orchestrator.py`:
- Around line 179-230: The ModelInfraErrorContext instances created in
_load_handler_class and create_registry lack correlation IDs; replace every
direct ModelInfraErrorContext(...) call (all 8 occurrences) with
ModelInfraErrorContext.with_correlation(...), preserving the existing keyword
args (transport_type=EnumInfraTransportType.RUNTIME,
operation="load_handler_class" or the appropriate operation string,
target_name=f"{module_path}.{class_name}" or the appropriate target) so the
resulting ctx passed into ProtocolConfigurationError (and raised "from e" cases)
includes an auto-generated correlation ID for distributed tracing.
- Around line 156-176: The return type annotation of _load_handler_class should
be changed from the unparameterized `type` to the parameterized `type[object]`
to avoid implicit Any typing; update the function signature `def
_load_handler_class(class_name: str, module_path: str) -> type:` to use `->
type[object]` and adjust any related type hints/usages within that function (and
add a typing import if needed) so strict typing rules are satisfied while
keeping the same behaviour.
In `@src/omnibase_infra/runtime/contract_loaders/handler_routing_loader.py`:
- Around line 103-121: Replace direct instantiations of ModelInfraErrorContext
with its with_correlation factory so a correlation_id is always included; e.g.,
where code currently does
ModelInfraErrorContext(transport_type=EnumInfraTransportType.FILESYSTEM,
operation=operation, target_name=str(contract_path)) (and the four other similar
sites), call
ModelInfraErrorContext.with_correlation(transport_type=EnumInfraTransportType.FILESYSTEM,
operation=operation, target_name=str(contract_path)) and pass that ctx into the
raised ProtocolConfigurationError (and any other error paths using ctx) so all
error contexts in this module include an auto-generated correlation_id.
- Around line 124-127: The return annotation of _load_and_validate_contract_yaml
is too broad (tuple[dict, dict]) and allows implicit Any; change it to
tuple[dict[str, JsonType], dict[str, JsonType]] to express JSON-compatible
key/value types, update the function signature accordingly, and add or import
JsonType (from your project's types or typing helpers) at the top of the module
so yaml.safe_load() results and the returned dicts are typed explicitly.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Lite
📒 Files selected for processing (5)
src/omnibase_infra/nodes/node_registration_orchestrator/registry/registry_infra_node_registration_orchestrator.pysrc/omnibase_infra/runtime/contract_loaders/handler_routing_loader.pysrc/omnibase_infra/runtime/handler_contract_source.pysrc/omnibase_infra/validation/validation_exemptions.yamltests/unit/runtime/contract_loaders/test_handler_routing_loader.py
🧰 Additional context used
📓 Path-based instructions (3)
**/*.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/handler_contract_source.pysrc/omnibase_infra/nodes/node_registration_orchestrator/registry/registry_infra_node_registration_orchestrator.pytests/unit/runtime/contract_loaders/test_handler_routing_loader.pysrc/omnibase_infra/runtime/contract_loaders/handler_routing_loader.py
**/handler_*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Handlers must NOT have direct event bus access - only orchestrators may have bus parameters and publish events
Files:
src/omnibase_infra/runtime/handler_contract_source.pysrc/omnibase_infra/runtime/contract_loaders/handler_routing_loader.py
**/registry_*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Registry files must follow naming convention: node-specific registries use
registry_infra_<node_name>.py→RegistryInfra<NodeName>, standalone registries useregistry_<purpose>.py→Registry<Purpose>
Files:
src/omnibase_infra/nodes/node_registration_orchestrator/registry/registry_infra_node_registration_orchestrator.py
🧠 Learnings (24)
📓 Common learnings
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_contract.yaml : Handlers must be discovered and loaded dynamically via YAML contracts using plugin pattern, not hardcoded registries
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: Applies to **/node_*/v[0-9]*_[0-9]*_[0-9]*/contracts/contract_*.yaml : All ONEX node contract definitions must use the new subcontract architecture pattern, breaking down complex contracts into separate contract_actions.yaml, contract_models.yaml, contract_validation.yaml, and optional contract_cli.yaml and contract_capabilities.yaml files for separation of concerns, maintainability, reusability, modularity, and future tool-as-a-service readiness
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_contract.yaml : Handler contract files must declare handler routing with `routing_strategy`, event models, handler classes, and handler modules
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/**/*.py : Use FileRegistry for loading YAML contracts: registry = FileRegistry(); contract = registry.load(Path(...)); handle FileRegistry error codes: FILE_NOT_FOUND, FILE_READ_ERROR, CONFIGURATION_PARSE_ERROR, CONTRACT_VALIDATION_ERROR, DUPLICATE_REGISTRATION, DIRECTORY_NOT_FOUND
Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: .cursor/rules/canonical_patterns.mdc:0-0
Timestamp: 2025-11-24T17:22:32.195Z
Learning: Applies to **/v[0-9]_[0-9]_[0-9]/contracts/ : Organize contract subcomponents into separate files (contract_actions.yaml, contract_models.yaml, contract_validation.yaml, etc.) and reference them from main contract.yaml
Learnt from: CR
Repo: OmniNode-ai/omniagent PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T22:21:32.649Z
Learning: Workflow coordination must be defined in YAML contracts loaded by `NodeAgentOrchestrator`, not as separate Python classes
Learnt from: CR
Repo: OmniNode-ai/omnibase_infra PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T17:31:33.550Z
Learning: When both `handler_contract.yaml` and `contract.yaml` exist in the same directory, the loader must raise an error (AMBIGUOUS_CONTRACT_CONFIGURATION)
📚 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/**/*.py : All TODOs must reference a Linear ticket in the format # TODO(OMN-XXXX): description; use # TODO(OMN-TBD): [NEEDS TICKET] as temporary marker during triage
Applied to files:
src/omnibase_infra/runtime/handler_contract_source.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/**/*.py : All type: ignore comments must include specific error codes (e.g., [arg-type], [return-value]) and an explanation comment on the line above with format: # NOTE(OMN-XXXX): reason. Safe because <invariant>.
Applied to files:
src/omnibase_infra/runtime/handler_contract_source.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 : Document exception handlers with standardized markers: # fallback-ok:, # catch-all-ok:, # cleanup-resilience-ok:, # boundary-ok:, # init-errors-ok:, # tool-resilience-ok:
Applied to files:
src/omnibase_infra/runtime/handler_contract_source.pysrc/omnibase_infra/validation/validation_exemptions.yaml
📚 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/**/*.py : Use FileRegistry for loading YAML contracts: registry = FileRegistry(); contract = registry.load(Path(...)); handle FileRegistry error codes: FILE_NOT_FOUND, FILE_READ_ERROR, CONFIGURATION_PARSE_ERROR, CONTRACT_VALIDATION_ERROR, DUPLICATE_REGISTRATION, DIRECTORY_NOT_FOUND
Applied to files:
src/omnibase_infra/runtime/handler_contract_source.pysrc/omnibase_infra/nodes/node_registration_orchestrator/registry/registry_infra_node_registration_orchestrator.pytests/unit/runtime/contract_loaders/test_handler_routing_loader.pysrc/omnibase_infra/runtime/contract_loaders/handler_routing_loader.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: Applies to **/contracts/*.yaml : Validate contracts using the contract linter tool: python -m omniintelligence.tools.contract_linter for YAML contract definitions
Applied to files:
src/omnibase_infra/runtime/handler_contract_source.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: Applies to **/*contract*.yaml : All ONEX nodes must have validated YAML contracts following the contract-driven development pattern with input_state and output_state schema definitions
Applied to files:
src/omnibase_infra/runtime/handler_contract_source.py
📚 Learning: 2025-11-24T17:22:32.195Z
Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: .cursor/rules/canonical_patterns.mdc:0-0
Timestamp: 2025-11-24T17:22:32.195Z
Learning: Applies to **/v[0-9]_[0-9]_[0-9]/{tools,registry}/**.py : Constructor-based dependency injection must be used for all tool and node classes; validate that required dependencies are not None, raising OnexError with specific error code if missing
Applied to files:
src/omnibase_infra/validation/validation_exemptions.yaml
📚 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 **/!(tool_)+(handler_|utils_|core_)*.py : Do not use prefix patterns `handler_`, `utils_`, or `core_` for file names; use `tool_` prefix instead for business logic files
Applied to files:
src/omnibase_infra/validation/validation_exemptions.yaml
📚 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_contract.yaml : Handlers must be discovered and loaded dynamically via YAML contracts using plugin pattern, not hardcoded registries
Applied to files:
src/omnibase_infra/nodes/node_registration_orchestrator/registry/registry_infra_node_registration_orchestrator.pysrc/omnibase_infra/runtime/contract_loaders/handler_routing_loader.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 : Use NodeEffect, NodeCompute, NodeReducer, and NodeOrchestrator base classes with declarative YAML contracts; import from omnibase_core.nodes
Applied to files:
src/omnibase_infra/nodes/node_registration_orchestrator/registry/registry_infra_node_registration_orchestrator.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/registry/registry_infra_node_registration_orchestrator.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: Workflow coordination must be defined in YAML contracts loaded by `NodeAgentOrchestrator`, not as separate Python classes
Applied to files:
src/omnibase_infra/nodes/node_registration_orchestrator/registry/registry_infra_node_registration_orchestrator.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: Applies to **/node_*/v[0-9]*_[0-9]*_[0-9]*/contracts/contract_*.yaml : All ONEX node contract definitions must use the new subcontract architecture pattern, breaking down complex contracts into separate contract_actions.yaml, contract_models.yaml, contract_validation.yaml, and optional contract_cli.yaml and contract_capabilities.yaml files for separation of concerns, maintainability, reusability, modularity, and future tool-as-a-service readiness
Applied to files:
src/omnibase_infra/nodes/node_registration_orchestrator/registry/registry_infra_node_registration_orchestrator.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: When both `handler_contract.yaml` and `contract.yaml` exist in the same directory, the loader must raise an error (AMBIGUOUS_CONTRACT_CONFIGURATION)
Applied to files:
src/omnibase_infra/nodes/node_registration_orchestrator/registry/registry_infra_node_registration_orchestrator.pytests/unit/runtime/contract_loaders/test_handler_routing_loader.pysrc/omnibase_infra/runtime/contract_loaders/handler_routing_loader.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_contract.yaml : Handler contract files must declare handler routing with `routing_strategy`, event models, handler classes, and handler modules
Applied to files:
src/omnibase_infra/nodes/node_registration_orchestrator/registry/registry_infra_node_registration_orchestrator.pytests/unit/runtime/contract_loaders/test_handler_routing_loader.pysrc/omnibase_infra/runtime/contract_loaders/handler_routing_loader.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/reducer/**/*.py : FSM behavior must be defined in YAML contracts, not as multiple Python reducer classes; use one NodeReducer class that loads different FSM contracts
Applied to files:
src/omnibase_infra/nodes/node_registration_orchestrator/registry/registry_infra_node_registration_orchestrator.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 **/v[0-9]_[0-9]_[0-9]/contract.yaml : The main contract.yaml file serves as the source of truth for node interfaces and should reference subcontracts using $ref patterns
Applied to files:
src/omnibase_infra/nodes/node_registration_orchestrator/registry/registry_infra_node_registration_orchestrator.py
📚 Learning: 2025-11-24T17:22:32.195Z
Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: .cursor/rules/canonical_patterns.mdc:0-0
Timestamp: 2025-11-24T17:22:32.195Z
Learning: Applies to **/v[0-9]_[0-9]_[0-9]/contracts/ : Organize contract subcomponents into separate files (contract_actions.yaml, contract_models.yaml, contract_validation.yaml, etc.) and reference them from main contract.yaml
Applied to files:
src/omnibase_infra/nodes/node_registration_orchestrator/registry/registry_infra_node_registration_orchestrator.py
📚 Learning: 2025-11-24T17:22:32.195Z
Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: .cursor/rules/canonical_patterns.mdc:0-0
Timestamp: 2025-11-24T17:22:32.195Z
Learning: Applies to **/v[0-9]_[0-9]_[0-9]/contract.yaml : Use shared schema references with project root paths in contract definitions (e.g., schemas/onex_field_model.schema.yaml, schemas/semver_model.schema.yaml)
Applied to files:
src/omnibase_infra/nodes/node_registration_orchestrator/registry/registry_infra_node_registration_orchestrator.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/protocols/handlers/**/*.py : Protocol naming convention: Handler protocols must follow `Protocol{Type}Handler` pattern
Applied to files:
src/omnibase_infra/nodes/node_registration_orchestrator/registry/registry_infra_node_registration_orchestrator.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/nodes/node_registration_orchestrator/registry/registry_infra_node_registration_orchestrator.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/nodes/node_registration_orchestrator/registry/registry_infra_node_registration_orchestrator.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 `InfraConnectionError`, `InfraTimeoutError`, `InfraAuthenticationError`, `InfraUnavailableError` for transport failures with proper `ModelInfraErrorContext`
Applied to files:
src/omnibase_infra/runtime/contract_loaders/handler_routing_loader.py
🧬 Code graph analysis (2)
tests/unit/runtime/contract_loaders/test_handler_routing_loader.py (5)
src/omnibase_infra/runtime/contract_loaders/handler_routing_loader.py (3)
convert_class_to_handler_key(255-278)load_handler_routing_subcontract(281-381)load_handler_class_info_from_contract(384-455)tests/unit/runtime/contract_loaders/conftest.py (12)
valid_contract_path(164-172)minimal_contract_path(176-184)contract_with_empty_handlers_path(200-208)nonexistent_contract_path(260-266)invalid_yaml_path(224-232)empty_contract_path(236-244)whitespace_only_contract_path(248-256)contract_without_routing_path(188-196)contract_with_incomplete_handler_path(212-220)contract_with_invalid_routing_strategy_path(270-280)contract_with_unknown_routing_strategy_path(284-292)oversized_contract_path(296-311)src/omnibase_infra/models/routing/model_routing_subcontract.py (1)
ModelRoutingSubcontract(21-67)src/omnibase_infra/errors/error_infra.py (1)
ProtocolConfigurationError(154-189)src/omnibase_infra/nodes/node_registration_orchestrator/node.py (1)
_create_handler_routing_subcontract(70-88)
src/omnibase_infra/runtime/contract_loaders/handler_routing_loader.py (5)
src/omnibase_infra/enums/enum_infra_transport_type.py (1)
EnumInfraTransportType(32-60)src/omnibase_infra/models/errors/model_infra_error_context.py (1)
ModelInfraErrorContext(17-96)src/omnibase_infra/errors/error_infra.py (1)
ProtocolConfigurationError(154-189)src/omnibase_infra/models/routing/model_routing_entry.py (1)
ModelRoutingEntry(14-49)src/omnibase_infra/models/routing/model_routing_subcontract.py (1)
ModelRoutingSubcontract(21-67)
🔇 Additional comments (5)
src/omnibase_infra/runtime/handler_contract_source.py (1)
453-464: Expanded TODO rationale is clear and actionable.The added context makes the future FileRegistry refactor intent easy to follow.
src/omnibase_infra/validation/validation_exemptions.yaml (1)
1542-1548: Exemption rationale update looks consistent.The updated reasoning and ticket reference read cleanly.
src/omnibase_infra/nodes/node_registration_orchestrator/registry/registry_infra_node_registration_orchestrator.py (1)
29-61: Handler dependency map trade-off is well documented.Clear rationale and maintenance guidance make the explicit wiring decision easy to audit.
tests/unit/runtime/contract_loaders/test_handler_routing_loader.py (1)
180-242: Characterization test documentation is excellent.The underscore behavior rationale is explicit and should prevent accidental regressions.
src/omnibase_infra/runtime/contract_loaders/handler_routing_loader.py (1)
167-172: This review comment targets the wrong file. The ambiguity detection forAMBIGUOUS_CONTRACT_CONFIGURATIONis already fully implemented insrc/omnibase_infra/runtime/handler_plugin_loader.py(lines 1969-1990), not inhandler_routing_loader.py.
handler_routing_loader.pyis a utility module designed specifically to load and validatecontract.yamlfiles for routing information. It is not a handler discovery mechanism—it only loads a single contract.yaml file when explicitly invoked by orchestrators. Thehandler_plugin_loader.py, which is the comprehensive handler discovery and loading mechanism that deals with bothhandler_contract.yamlandcontract.yaml, already contains the required ambiguity check with proper error handling and is tested intests/integration/runtime/test_handler_plugin_loader_ambiguous_contract.py.Adding this check to
handler_routing_loader.pywould be architecturally incorrect and redundant.Likely an incorrect or invalid review comment.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| def _load_and_validate_contract_yaml( | ||
| contract_path: Path, | ||
| operation: str, | ||
| ) -> tuple[dict, dict]: |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "handler_routing_loader.py" -path "*/contract_loaders/*"Repository: OmniNode-ai/omnibase_infra
Length of output: 141
🏁 Script executed:
cat -n src/omnibase_infra/runtime/contract_loaders/handler_routing_loader.py | head -150Repository: OmniNode-ai/omnibase_infra
Length of output: 6724
🏁 Script executed:
grep -n "def _load_and_validate_contract_yaml" src/omnibase_infra/runtime/contract_loaders/handler_routing_loader.pyRepository: OmniNode-ai/omnibase_infra
Length of output: 111
🏁 Script executed:
cat -n src/omnibase_infra/runtime/contract_loaders/handler_routing_loader.py | sed -n '124,250p'Repository: OmniNode-ai/omnibase_infra
Length of output: 6295
🏁 Script executed:
grep -A 80 "def _load_and_validate_contract_yaml" src/omnibase_infra/runtime/contract_loaders/handler_routing_loader.py | head -100Repository: OmniNode-ai/omnibase_infra
Length of output: 3534
🏁 Script executed:
grep -r "JsonType" src/omnibase_infra/ --include="*.py" | head -20Repository: OmniNode-ai/omnibase_infra
Length of output: 2572
🏁 Script executed:
grep -r "class JsonType\|JsonType =" src/omnibase_core/ --include="*.py" 2>/dev/null | head -10Repository: OmniNode-ai/omnibase_infra
Length of output: 52
🏁 Script executed:
grep -r "from omnibase_core.types import" src/omnibase_infra/ --include="*.py" | head -10Repository: OmniNode-ai/omnibase_infra
Length of output: 1091
🏁 Script executed:
grep -A 5 "JsonType = " src/omnibase_core/types.py 2>/dev/null || grep -A 5 "JsonType" src/omnibase_core/types/__init__.py 2>/dev/null | head -20Repository: OmniNode-ai/omnibase_infra
Length of output: 52
🏁 Script executed:
find . -name "types.py" -o -name "types" -type d | grep omnibase_coreRepository: OmniNode-ai/omnibase_infra
Length of output: 52
🏁 Script executed:
cat src/omnibase_infra/models/types/__init__.pyRepository: OmniNode-ai/omnibase_infra
Length of output: 2952
🏁 Script executed:
grep -n "ModelInfraErrorContext(" src/omnibase_infra/runtime/contract_loaders/handler_routing_loader.pyRepository: OmniNode-ai/omnibase_infra
Length of output: 279
🏁 Script executed:
grep -B 5 -A 5 "correlation_id" src/omnibase_infra/errors.py | head -40Repository: OmniNode-ai/omnibase_infra
Length of output: 131
🏁 Script executed:
grep -A 10 "class ModelInfraErrorContext" src/omnibase_infra/errors.py | head -20Repository: OmniNode-ai/omnibase_infra
Length of output: 131
🏁 Script executed:
find . -name "*.py" -type f | xargs grep -l "class ModelInfraErrorContext" | head -5Repository: OmniNode-ai/omnibase_infra
Length of output: 133
🏁 Script executed:
grep -r "class ModelInfraErrorContext" src/omnibase_infra --include="*.py"Repository: OmniNode-ai/omnibase_infra
Length of output: 172
🏁 Script executed:
grep -n "from omnibase_infra" src/omnibase_infra/runtime/contract_loaders/handler_routing_loader.py | head -5Repository: OmniNode-ai/omnibase_infra
Length of output: 390
🏁 Script executed:
cat -n src/omnibase_infra/models/errors/model_infra_error_context.pyRepository: OmniNode-ai/omnibase_infra
Length of output: 4443
🏁 Script executed:
grep -n "correlation_id" src/omnibase_infra/runtime/contract_loaders/handler_routing_loader.pyRepository: OmniNode-ai/omnibase_infra
Length of output: 52
🏁 Script executed:
find . -name "__init__.py" -path "*/omnibase_infra/errors*" -exec cat {} \;Repository: OmniNode-ai/omnibase_infra
Length of output: 5568
🏁 Script executed:
cat -n src/omnibase_infra/runtime/contract_loaders/handler_routing_loader.py | sed -n '1,80p'Repository: OmniNode-ai/omnibase_infra
Length of output: 3303
🏁 Script executed:
grep -n "handler_contract\|contract_handler" src/omnibase_infra/runtime/contract_loaders/handler_routing_loader.pyRepository: OmniNode-ai/omnibase_infra
Length of output: 148
🏁 Script executed:
ls -la src/omnibase_infra/runtime/contract_loaders/Repository: OmniNode-ai/omnibase_infra
Length of output: 321
🏁 Script executed:
grep -B 5 "def load_handler_routing_subcontract" src/omnibase_infra/runtime/contract_loaders/handler_routing_loader.py | head -15Repository: OmniNode-ai/omnibase_infra
Length of output: 369
🏁 Script executed:
grep -A 20 "def load_handler_routing_subcontract" src/omnibase_infra/runtime/contract_loaders/handler_routing_loader.py | head -30Repository: OmniNode-ai/omnibase_infra
Length of output: 1059
🏁 Script executed:
grep -A 50 "def load_handler_routing_subcontract" src/omnibase_infra/runtime/contract_loaders/handler_routing_loader.py | grep -A 40 "contract, handler_routing ="Repository: OmniNode-ai/omnibase_infra
Length of output: 597
🏁 Script executed:
sed -n '283,400p' src/omnibase_infra/runtime/contract_loaders/handler_routing_loader.pyRepository: OmniNode-ai/omnibase_infra
Length of output: 4414
🏁 Script executed:
grep -n "handler_contract.yaml\|contract.yaml" src/omnibase_infra/runtime/contract_loaders/handler_routing_loader.py | head -20Repository: OmniNode-ai/omnibase_infra
Length of output: 1478
🏁 Script executed:
ls src/omnibase_infra/runtime/contract_loaders/Repository: OmniNode-ai/omnibase_infra
Length of output: 107
🏁 Script executed:
cat src/omnibase_infra/runtime/contract_loaders/__init__.pyRepository: OmniNode-ai/omnibase_infra
Length of output: 1407
🏁 Script executed:
grep -A 30 "def load_handler_class_info_from_contract" src/omnibase_infra/runtime/contract_loaders/handler_routing_loader.py | tail -25Repository: OmniNode-ai/omnibase_infra
Length of output: 959
🏁 Script executed:
python3 << 'EOF'
# Check what yaml.safe_load returns
import yaml
# Sample contract YAML
sample = """
handler_routing:
routing_strategy: "payload_type_match"
handlers:
- event_model:
name: "ModelNodeIntrospectionEvent"
"""
result = yaml.safe_load(sample)
print("Type of result:", type(result))
print("Result:", result)
print("Type of result['handler_routing']:", type(result['handler_routing']))
print("Handler routing value:", result['handler_routing'])
EOFRepository: OmniNode-ai/omnibase_infra
Length of output: 156
🏁 Script executed:
grep -B 5 "uuid4" src/omnibase_infra/runtime/contract_loaders/handler_routing_loader.pyRepository: OmniNode-ai/omnibase_infra
Length of output: 52
🏁 Script executed:
grep -r "uuid4\|uuid4()" src/omnibase_infra --include="*.py" -l | head -5Repository: OmniNode-ai/omnibase_infra
Length of output: 317
🏁 Script executed:
cat -n src/omnibase_infra/runtime/contract_loaders/handler_routing_loader.py | sed -n '260,420p'Repository: OmniNode-ai/omnibase_infra
Length of output: 7283
Improve return type precision in _load_and_validate_contract_yaml to avoid implicit Any.
The return type tuple[dict, dict] omits key and value types, implying Any. Since yaml.safe_load() and dictionary access return JSON-compatible values, use tuple[dict[str, JsonType], dict[str, JsonType]] to maintain explicit typing and satisfy the no-Any rule.
♻️ Proposed fix
from omnibase_core.models.primitives.model_semver import ModelSemVer
+from omnibase_core.types import JsonType
@@
def _load_and_validate_contract_yaml(
contract_path: Path,
operation: str,
-) -> tuple[dict, dict]:
+) -> tuple[dict[str, JsonType], dict[str, JsonType]]:🤖 Prompt for AI Agents
In `@src/omnibase_infra/runtime/contract_loaders/handler_routing_loader.py` around
lines 124 - 127, The return annotation of _load_and_validate_contract_yaml is
too broad (tuple[dict, dict]) and allows implicit Any; change it to
tuple[dict[str, JsonType], dict[str, JsonType]] to express JSON-compatible
key/value types, update the function signature accordingly, and add or import
JsonType (from your project's types or typing helpers) at the top of the module
so yaml.safe_load() results and the returned dicts are typed explicitly.
Address PR #153 review feedback: - Replace unparameterized `type` with `type[object]` in _load_handler_class - Add correlation IDs to all ModelInfraErrorContext instantiations using .with_correlation() factory method for distributed tracing support
Code Review - PR #153: Handler Routing Loader RefactorSummaryThis PR successfully extracts handler routing logic from individual orchestrators into a shared, reusable utility that follows the Handler Plugin Loader pattern. The refactoring eliminates duplicate code, improves maintainability, and aligns with ONEX's contract-driven architecture. Overall Assessment: ✅ APPROVED with minor observations Strengths1. Excellent Architecture & Design 🎯
2. Security Controls 🔒
3. Documentation Quality 📚
4. Test Coverage ✅
Observations & Recommendations1. Handler Dependency Map - Intentional Manual MaintenanceFile: The Why manual over auto-discovery:
Maintenance requirement: When adding a handler to Recommendation: This is the right trade-off. Consider adding a pre-commit hook or CI check that validates contract.yaml handlers match the dependency map keys. 2. Regex-Based Kebab-Case ConversionFile: The "My_Handler" -> "my_-handler" # Mixed underscore-hyphenThis is acceptable since:
Recommendation: No action needed. The characterization test serves as a "change detector" and documents edge cases. 3. Transport Type for File OperationsFile: File loading errors use # File loading - uses FILESYSTEM
transport_type=EnumInfraTransportType.FILESYSTEM,
operation="load_handler_routing_contract",
# Registry creation - uses RUNTIME
transport_type=EnumInfraTransportType.RUNTIME,
operation="create_registry",Recommendation: Consider documenting the distinction in error context guidelines. FILESYSTEM = file I/O errors, RUNTIME = logical/validation errors. 4. Optional Dependency Filtering LogicFile: The Observation: This is exemplary inline documentation. No changes needed. 5. File Size Check Before ParsingFile: The file size check happens BEFORE # Check file size before loading (security control)
_check_file_size(contract_path, operation)
# Load YAML file
with contract_path.open("r", encoding="utf-8") as f:
contract = yaml.safe_load(f)Observation: This is the correct order. Prevents memory exhaustion attacks. Well done. Code Quality Checklist
Security Review ✅All security controls from CLAUDE.md Handler Plugin Loader pattern are implemented:
Performance Considerations
Alignment with CLAUDE.md
Final RecommendationsMust-Do Before MergeNone. The PR is ready to merge as-is. Nice-to-Have Follow-ups
ConclusionThis is high-quality infrastructure refactoring that follows best practices:
Approval Status: ✅ APPROVED Great work on this refactor! The design trade-offs are well-documented, security controls are robust, and the test coverage is excellent. 🎉 Related Issues: Closes OMN-1316 |
1 participant
Summary
runtime/contract_loaders/handler_routing_loader.py)contract.yamlusing Handler Plugin Loader patternChanges
runtime/contract_loaders/__init__.pyruntime/contract_loaders/handler_routing_loader.pynodes/node_registration_orchestrator/node.pynodes/node_registration_orchestrator/registry/...pytests/unit/runtime/contract_loaders/Approach
Per ticket discussion:
load_handler_routing_subcontract()extracted and made publicTest plan
Closes OMN-1316
Summary by CodeRabbit
New Features
Refactor
Chores
Tests
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.