Repository navigation
feat(naming): rename decorators/, errors/, protocols/ files (OMN-1217, OMN-1218, OMN-1219) - #317
Conversation
… naming conventions (OMN-1217, OMN-1218, OMN-1219) Rename files to enforce consistent naming conventions across the codebase: decorators/ (OMN-1217): - allow_dict_any.py → decorator_allow_dict_any.py - convert_to_schema.py → decorator_convert_to_schema.py - error_handling.py → decorator_error_handling.py - pattern_exclusions.py → decorator_pattern_exclusions.py errors/ (OMN-1218): - declarative_errors.py → error_declarative.py - exceptions.py → exception_base.py - runtime_errors.py → error_runtime.py protocols/ (OMN-1219): - core.py → protocol_core.py Updated all imports across src/, tests/, and docs/.
📝 WalkthroughWalkthroughRefactors and renames across decorators, error modules, and protocols: many public symbols re-exported from new modules (e.g., decorator_* and error_*). Import paths updated across src, tests, and docs; public names and signatures preserved. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
Comment |
Code Review - PR #317SummaryThis PR implements a comprehensive file naming convention standardization across ✅ Strengths1. Excellent Naming Convention AdherenceThe renaming follows clear, consistent patterns:
This makes the codebase more navigable and enforces clear module categorization. 2. Comprehensive Import UpdatesAll 41 import sites have been updated:
The thorough nature of this refactor reduces the risk of runtime import errors. 3. Lazy Loading PreservedThe 4. Alphabetical Import OrganizationThe 🔍 Areas for Improvement1. Test Coverage Gap - Import Validation
|
| Category | Rating | Notes |
|---|---|---|
| Correctness | ⭐⭐⭐⭐⭐ | All imports updated comprehensively |
| Consistency | ⭐⭐⭐⭐⭐ | Clear, uniform naming convention |
| Documentation | ⭐⭐⭐⭐☆ | Updated, but could add migration note |
| Testing | ⭐⭐⭐⭐☆ | Good coverage, could add import validation tests |
| Maintainability | ⭐⭐⭐⭐⭐ | Significantly improved discoverability |
✅ Final Verdict
Recommendation: APPROVE pending CI success.
This is a high-quality refactoring PR that improves codebase consistency and maintainability. The comprehensive nature of the import updates across 41 files demonstrates thorough execution. The only minor suggestions are:
- Adding explicit import validation tests
- Considering a migration note for any external consumers
The naming convention enforced here will make the codebase significantly easier to navigate for new developers and aligns with ONEX standards.
Great work on this systematic cleanup! 🎉
Reviewed by: Claude Code (Sonnet 4.5)
Review Date: 2026-01-03
Files Changed: 41 files (+178/-158 lines)
Scope: OMN-1217, OMN-1218, OMN-1219
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/omnibase_core/tools/mypy_plugins/dict_any_checker.py (1)
94-94: Update decorator path constants to match renamed module.The module was renamed from
allow_dict_any.pytodecorator_allow_dict_any.py, but the string constants used for AST pattern matching still reference the old path. This will cause the mypy plugin to fail to recognize the decorator when it's imported from the new module.Update:
- Line 94:
"omnibase_core.decorators.allow_dict_any.allow_dict_any"→"omnibase_core.decorators.decorator_allow_dict_any.allow_dict_any"- Line 605:
"omnibase_core.decorators.allow_dict_any"→"omnibase_core.decorators.decorator_allow_dict_any"🔎 Proposed fix
# Decorator that allows dict[str, Any] usage - ALLOW_DECORATOR = "omnibase_core.decorators.allow_dict_any.allow_dict_any" + ALLOW_DECORATOR = "omnibase_core.decorators.decorator_allow_dict_any.allow_dict_any" ALLOW_DECORATOR_SHORT = "allow_dict_any"if decorator_name in ( self.ALLOW_DECORATOR, self.ALLOW_DECORATOR_SHORT, # Common import patterns "allow_dict_any.allow_dict_any", "decorators.allow_dict_any.allow_dict_any", - "omnibase_core.decorators.allow_dict_any", + "omnibase_core.decorators.decorator_allow_dict_any", ): return TrueAlso applies to: 605-605
tests/unit/errors/test_error_declarative.py (1)
885-899: Update import to use the correctly renamed module.The test imports
declarative_errorswhich doesn't exist. The module was renamed toerror_declarativeand is not aliased for backwards compatibility inerrors/__init__.py. Update the import and references:from omnibase_core.errors import error_declarativeThen change
declarative_errors.__all__toerror_declarative.__all__in the assertion.
🧹 Nitpick comments (1)
src/omnibase_core/protocols/__init__.py (1)
175-181: Remove duplicate section header.Lines 175-177 contain a "Compute Module Exports" header that appears to be leftover from a copy-paste. The actual section is "Core Module Exports" (lines 179-180). This creates unnecessary confusion.
🔎 Proposed cleanup
-# ============================================================================= -# Compute Module Exports -# ============================================================================= # ============================================================================= # Core Module Exports # ============================================================================= from omnibase_core.protocols.protocol_core import ProtocolCanonicalSerializer
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (41)
CLAUDE.mddocs/architecture/EXECUTION_SHAPE_EXAMPLES.mddocs/architecture/adr/ADR-012-VALIDATOR-ERROR-HANDLING.mddocs/architecture/decisions/ADR-003-reducer-output-exception-consistency.mddocs/reference/api/utils.mddocs/reference/templates/EFFECT_NODE_TEMPLATE.mddocs/reference/templates/ORCHESTRATOR_NODE_TEMPLATE.mddocs/release/V040_RELEASE_CHECKLIST.mdsrc/omnibase_core/decorators/__init__.pysrc/omnibase_core/decorators/decorator_allow_dict_any.pysrc/omnibase_core/decorators/decorator_convert_to_schema.pysrc/omnibase_core/decorators/decorator_error_handling.pysrc/omnibase_core/decorators/decorator_pattern_exclusions.pysrc/omnibase_core/errors/__init__.pysrc/omnibase_core/errors/error_declarative.pysrc/omnibase_core/errors/error_runtime.pysrc/omnibase_core/errors/exception_adapter_binding_error.pysrc/omnibase_core/errors/exception_base.pysrc/omnibase_core/errors/exception_node_execution_error.pysrc/omnibase_core/errors/exception_purity_violation_error.pysrc/omnibase_core/errors/exception_unsupported_capability_error.pysrc/omnibase_core/models/capabilities/model_capability_requirement_set.pysrc/omnibase_core/models/container/model_onex_container.pysrc/omnibase_core/models/context/model_effect_template_context.pysrc/omnibase_core/models/effect/model_effect_input.pysrc/omnibase_core/models/intents/model_registration_record_base.pysrc/omnibase_core/models/operations/model_effect_operation_config.pysrc/omnibase_core/models/operations/model_operationparameters.pysrc/omnibase_core/models/services/model_node_service_config.pysrc/omnibase_core/protocols/__init__.pysrc/omnibase_core/protocols/protocol_core.pysrc/omnibase_core/runtime/envelope_router.pysrc/omnibase_core/services/service_protocol_auditor.pysrc/omnibase_core/tools/mypy_plugins/dict_any_checker.pysrc/omnibase_core/validation/__init__.pytests/unit/decorators/test_convert_to_schema.pytests/unit/decorators/test_error_handling_extended.pytests/unit/decorators/test_pattern_exclusions.pytests/unit/errors/test_error_declarative.pytests/unit/errors/test_error_runtime.pytests/unit/validation/test_exception_base.py
🧰 Additional context used
📓 Path-based instructions (4)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Use PEP 604 union syntax for type annotations (e.g.,str | Noneinstead ofOptional[str])
Resolve services using protocol names (e.g.,container.get_service('ProtocolEventBus')) never using concrete class names
Order exception tuples alphabetically by exception class name (e.g.,except (AttributeError, TypeError, ValidationError)notexcept (ValueError, TypeError, AttributeError))
Format code usingpoetry run blackand sort imports usingpoetry run isort
Lint code withpoetry run ruff checkand fix automatically withpoetry run ruff check --fix
Files:
src/omnibase_core/services/service_protocol_auditor.pysrc/omnibase_core/errors/exception_unsupported_capability_error.pytests/unit/decorators/test_error_handling_extended.pysrc/omnibase_core/models/intents/model_registration_record_base.pysrc/omnibase_core/errors/error_runtime.pysrc/omnibase_core/models/context/model_effect_template_context.pysrc/omnibase_core/models/operations/model_effect_operation_config.pysrc/omnibase_core/models/services/model_node_service_config.pysrc/omnibase_core/errors/exception_node_execution_error.pysrc/omnibase_core/models/effect/model_effect_input.pytests/unit/errors/test_error_declarative.pysrc/omnibase_core/runtime/envelope_router.pysrc/omnibase_core/models/container/model_onex_container.pytests/unit/decorators/test_pattern_exclusions.pysrc/omnibase_core/validation/__init__.pysrc/omnibase_core/errors/error_declarative.pysrc/omnibase_core/errors/exception_adapter_binding_error.pytests/unit/errors/test_error_runtime.pysrc/omnibase_core/protocols/__init__.pysrc/omnibase_core/tools/mypy_plugins/dict_any_checker.pysrc/omnibase_core/errors/exception_purity_violation_error.pysrc/omnibase_core/models/capabilities/model_capability_requirement_set.pysrc/omnibase_core/models/operations/model_operationparameters.pytests/unit/decorators/test_convert_to_schema.pysrc/omnibase_core/decorators/__init__.pysrc/omnibase_core/errors/__init__.pytests/unit/validation/test_exception_base.py
src/omnibase_core/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/omnibase_core/**/*.py: Use ModelOnexError with error codes from EnumCoreErrorCode for exception handling, never generic Exception
Use @standard_error_handling decorator from omnibase_core.decorators to eliminate try/catch boilerplate
Add standardized exception handling comment markers (# fallback-ok:,# catch-all-ok:,# cleanup-resilience-ok:,# boundary-ok:,# init-errors-ok:,# tool-resilience-ok:) when using broad exception handlers
Use centralized exception group constants from omnibase_core.errors.exception_groups instead of individual exception types
Ensure all source code passes mypy strict mode with zero errors and pyright basic mode validation
Use FileRegistry for loading YAML contracts and configuration files instead of direct file reading
Use duck typing with protocols for service resolution - never use isinstance checks for protocol-based dependencies
Files:
src/omnibase_core/services/service_protocol_auditor.pysrc/omnibase_core/errors/exception_unsupported_capability_error.pysrc/omnibase_core/models/intents/model_registration_record_base.pysrc/omnibase_core/errors/error_runtime.pysrc/omnibase_core/models/context/model_effect_template_context.pysrc/omnibase_core/models/operations/model_effect_operation_config.pysrc/omnibase_core/models/services/model_node_service_config.pysrc/omnibase_core/errors/exception_node_execution_error.pysrc/omnibase_core/models/effect/model_effect_input.pysrc/omnibase_core/runtime/envelope_router.pysrc/omnibase_core/models/container/model_onex_container.pysrc/omnibase_core/validation/__init__.pysrc/omnibase_core/errors/error_declarative.pysrc/omnibase_core/errors/exception_adapter_binding_error.pysrc/omnibase_core/protocols/__init__.pysrc/omnibase_core/tools/mypy_plugins/dict_any_checker.pysrc/omnibase_core/errors/exception_purity_violation_error.pysrc/omnibase_core/models/capabilities/model_capability_requirement_set.pysrc/omnibase_core/models/operations/model_operationparameters.pysrc/omnibase_core/decorators/__init__.pysrc/omnibase_core/errors/__init__.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Use pytest markers (@pytest.mark.unit, @pytest.mark.integration, @pytest.mark.slow, @pytest.mark.smoke, @pytest.mark.performance) to classify test types
All tests must complete within 60-second timeout protection to ensure CI performance
Files:
tests/unit/decorators/test_error_handling_extended.pytests/unit/errors/test_error_declarative.pytests/unit/decorators/test_pattern_exclusions.pytests/unit/errors/test_error_runtime.pytests/unit/decorators/test_convert_to_schema.pytests/unit/validation/test_exception_base.py
src/omnibase_core/models/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/omnibase_core/models/**/*.py: Addfrom_attributes=Trueto ConfigDict for immutable value objects nested in other Pydantic models to support pytest-xdist parallel execution
Do NOT usefrom __future__ import annotationsin Pydantic model files (use only for forward references and circular import prevention)
Files:
src/omnibase_core/models/intents/model_registration_record_base.pysrc/omnibase_core/models/context/model_effect_template_context.pysrc/omnibase_core/models/operations/model_effect_operation_config.pysrc/omnibase_core/models/services/model_node_service_config.pysrc/omnibase_core/models/effect/model_effect_input.pysrc/omnibase_core/models/container/model_onex_container.pysrc/omnibase_core/models/capabilities/model_capability_requirement_set.pysrc/omnibase_core/models/operations/model_operationparameters.py
🧠 Learnings (14)
📓 Common learnings
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-02T20:42:17.780Z
Learning: Applies to src/omnibase_core/**/*.py : Use standard_error_handling decorator from omnibase_core.decorators to eliminate try/catch boilerplate
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-02T20:42:17.780Z
Learning: Applies to src/omnibase_core/**/*.py : Use centralized exception group constants from omnibase_core.errors.exception_groups instead of individual exception types
📚 Learning: 2026-01-02T20:42:17.780Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-02T20:42:17.780Z
Learning: Applies to src/omnibase_core/**/*.py : Use standard_error_handling decorator from omnibase_core.decorators to eliminate try/catch boilerplate
Applied to files:
docs/reference/templates/EFFECT_NODE_TEMPLATE.mddocs/architecture/EXECUTION_SHAPE_EXAMPLES.mdtests/unit/decorators/test_error_handling_extended.pysrc/omnibase_core/errors/error_runtime.pydocs/architecture/adr/ADR-012-VALIDATOR-ERROR-HANDLING.mdCLAUDE.mdsrc/omnibase_core/models/services/model_node_service_config.pydocs/architecture/decisions/ADR-003-reducer-output-exception-consistency.mdtests/unit/errors/test_error_declarative.pysrc/omnibase_core/runtime/envelope_router.pysrc/omnibase_core/errors/error_declarative.pydocs/reference/templates/ORCHESTRATOR_NODE_TEMPLATE.mddocs/reference/api/utils.mdsrc/omnibase_core/models/operations/model_operationparameters.pysrc/omnibase_core/decorators/__init__.py
📚 Learning: 2026-01-02T20:42:17.780Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-02T20:42:17.780Z
Learning: Applies to src/omnibase_core/**/*.py : Use ModelOnexError with error codes from EnumCoreErrorCode for exception handling, never generic Exception
Applied to files:
docs/reference/templates/EFFECT_NODE_TEMPLATE.mddocs/architecture/EXECUTION_SHAPE_EXAMPLES.mdsrc/omnibase_core/services/service_protocol_auditor.pysrc/omnibase_core/errors/exception_unsupported_capability_error.pysrc/omnibase_core/errors/error_runtime.pydocs/architecture/adr/ADR-012-VALIDATOR-ERROR-HANDLING.mdsrc/omnibase_core/models/services/model_node_service_config.pysrc/omnibase_core/errors/exception_node_execution_error.pysrc/omnibase_core/models/effect/model_effect_input.pytests/unit/errors/test_error_declarative.pysrc/omnibase_core/runtime/envelope_router.pysrc/omnibase_core/validation/__init__.pysrc/omnibase_core/errors/error_declarative.pysrc/omnibase_core/errors/exception_adapter_binding_error.pydocs/reference/templates/ORCHESTRATOR_NODE_TEMPLATE.mddocs/reference/api/utils.mdsrc/omnibase_core/errors/exception_purity_violation_error.pysrc/omnibase_core/models/operations/model_operationparameters.pysrc/omnibase_core/errors/__init__.pytests/unit/validation/test_exception_base.py
📚 Learning: 2026-01-02T20:42:17.780Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-02T20:42:17.780Z
Learning: Applies to src/omnibase_core/nodes/**/*.py : Use ModelONEXContainer (not ModelContainer) for dependency injection in node constructors
Applied to files:
docs/architecture/EXECUTION_SHAPE_EXAMPLES.mddocs/architecture/adr/ADR-012-VALIDATOR-ERROR-HANDLING.mdsrc/omnibase_core/models/services/model_node_service_config.pysrc/omnibase_core/errors/exception_node_execution_error.pytests/unit/errors/test_error_declarative.pysrc/omnibase_core/models/container/model_onex_container.pydocs/reference/templates/ORCHESTRATOR_NODE_TEMPLATE.md
📚 Learning: 2026-01-02T20:42:17.780Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-02T20:42:17.780Z
Learning: Applies to src/omnibase_core/nodes/**/*.py : Inherit from appropriate ONEX node base classes (NodeCompute, NodeReducer, NodeOrchestrator, NodeEffect) and call super().__init__(container)
Applied to files:
docs/architecture/EXECUTION_SHAPE_EXAMPLES.mdsrc/omnibase_core/errors/exception_node_execution_error.pytests/unit/errors/test_error_declarative.pydocs/reference/templates/ORCHESTRATOR_NODE_TEMPLATE.md
📚 Learning: 2026-01-02T20:42:17.780Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-02T20:42:17.780Z
Learning: Applies to src/omnibase_core/**/*.py : Use centralized exception group constants from omnibase_core.errors.exception_groups instead of individual exception types
Applied to files:
src/omnibase_core/services/service_protocol_auditor.pysrc/omnibase_core/errors/exception_unsupported_capability_error.pytests/unit/decorators/test_error_handling_extended.pysrc/omnibase_core/errors/error_runtime.pysrc/omnibase_core/errors/exception_node_execution_error.pytests/unit/errors/test_error_declarative.pysrc/omnibase_core/validation/__init__.pysrc/omnibase_core/errors/error_declarative.pysrc/omnibase_core/errors/exception_adapter_binding_error.pytests/unit/errors/test_error_runtime.pydocs/reference/templates/ORCHESTRATOR_NODE_TEMPLATE.mddocs/reference/api/utils.mdsrc/omnibase_core/errors/exception_purity_violation_error.pysrc/omnibase_core/models/operations/model_operationparameters.pysrc/omnibase_core/errors/__init__.pytests/unit/validation/test_exception_base.py
📚 Learning: 2026-01-02T20:42:17.780Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-02T20:42:17.780Z
Learning: Applies to src/omnibase_core/models/**/*.py : Add `from_attributes=True` to ConfigDict for immutable value objects nested in other Pydantic models to support pytest-xdist parallel execution
Applied to files:
src/omnibase_core/models/intents/model_registration_record_base.pysrc/omnibase_core/models/context/model_effect_template_context.pysrc/omnibase_core/models/services/model_node_service_config.pysrc/omnibase_core/models/capabilities/model_capability_requirement_set.py
📚 Learning: 2026-01-02T20:42:17.780Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-02T20:42:17.780Z
Learning: Applies to src/omnibase_core/**/*.py : Add standardized exception handling comment markers (`# fallback-ok:`, `# catch-all-ok:`, `# cleanup-resilience-ok:`, `# boundary-ok:`, `# init-errors-ok:`, `# tool-resilience-ok:`) when using broad exception handlers
Applied to files:
CLAUDE.mdsrc/omnibase_core/validation/__init__.pydocs/reference/templates/ORCHESTRATOR_NODE_TEMPLATE.mddocs/reference/api/utils.mdsrc/omnibase_core/decorators/__init__.pytests/unit/validation/test_exception_base.py
📚 Learning: 2026-01-02T20:42:17.780Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-02T20:42:17.780Z
Learning: Applies to src/omnibase_core/nodes/**/*.py : Use EnumNodeKind for architectural routing (EFFECT, COMPUTE, REDUCER, ORCHESTRATOR, RUNTIME_HOST) and EnumNodeType for implementation matching (COMPUTE_GENERIC, TRANSFORMER, AGGREGATOR, etc.)
Applied to files:
src/omnibase_core/errors/exception_node_execution_error.py
📚 Learning: 2025-09-23T22:28:00.333Z
Learnt from: jonahgabriel
Repo: OmniNode-ai/omnibase_core PR: 32
File: examples/practical_migration_example.py:125-134
Timestamp: 2025-09-23T22:28:00.333Z
Learning: The omnibase_core codebase extensively uses constrained TypeVars for primitive types with the pattern `T = TypeVar("T", str, int, float, bool)` across multiple models including ModelTypedMetrics, ModelGenericMetadata, and ModelExecutionResult. This is the preferred approach over hardcoded union types like `str | int | bool | float | None`.
Applied to files:
src/omnibase_core/models/container/model_onex_container.py
📚 Learning: 2026-01-02T20:42:17.780Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-02T20:42:17.780Z
Learning: Applies to src/omnibase_core/**/*.py : Use FileRegistry for loading YAML contracts and configuration files instead of direct file reading
Applied to files:
docs/reference/api/utils.md
📚 Learning: 2026-01-02T20:42:17.780Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-02T20:42:17.780Z
Learning: Applies to src/omnibase_core/nodes/**/*.py : Use declarative YAML contracts for node configuration and workflow definitions (not imperative code)
Applied to files:
docs/reference/api/utils.md
📚 Learning: 2026-01-02T20:42:17.780Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-02T20:42:17.780Z
Learning: Applies to src/omnibase_core/**/*.py : Use duck typing with protocols for service resolution - never use isinstance checks for protocol-based dependencies
Applied to files:
src/omnibase_core/protocols/__init__.py
📚 Learning: 2026-01-02T20:42:17.780Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-02T20:42:17.780Z
Learning: Applies to **/*.py : Resolve services using protocol names (e.g., `container.get_service('ProtocolEventBus')`) never using concrete class names
Applied to files:
src/omnibase_core/protocols/__init__.py
🧬 Code graph analysis (19)
src/omnibase_core/errors/exception_unsupported_capability_error.py (1)
src/omnibase_core/errors/error_runtime_host.py (1)
RuntimeHostError(38-102)
src/omnibase_core/models/intents/model_registration_record_base.py (2)
src/omnibase_core/decorators/decorator_allow_dict_any.py (1)
allow_dict_any(24-70)src/omnibase_core/decorators/decorator_pattern_exclusions.py (1)
allow_dict_any(81-99)
src/omnibase_core/models/context/model_effect_template_context.py (3)
src/omnibase_core/decorators/decorator_allow_dict_any.py (1)
allow_dict_any(24-70)src/omnibase_core/decorators/decorator_pattern_exclusions.py (1)
allow_dict_any(81-99)src/omnibase_core/mixins/mixin_effect_execution.py (1)
allow_dict_any(123-146)
src/omnibase_core/models/operations/model_effect_operation_config.py (3)
src/omnibase_core/decorators/decorator_allow_dict_any.py (1)
allow_dict_any(24-70)src/omnibase_core/decorators/decorator_pattern_exclusions.py (1)
allow_dict_any(81-99)src/omnibase_core/mixins/mixin_effect_execution.py (1)
allow_dict_any(123-146)
src/omnibase_core/models/services/model_node_service_config.py (1)
src/omnibase_core/decorators/decorator_error_handling.py (1)
standard_error_handling(26-93)
src/omnibase_core/errors/exception_node_execution_error.py (1)
src/omnibase_core/errors/error_runtime_host.py (1)
RuntimeHostError(38-102)
src/omnibase_core/models/effect/model_effect_input.py (3)
src/omnibase_core/decorators/decorator_allow_dict_any.py (1)
allow_dict_any(24-70)src/omnibase_core/decorators/decorator_pattern_exclusions.py (1)
allow_dict_any(81-99)src/omnibase_core/mixins/mixin_effect_execution.py (1)
allow_dict_any(123-146)
tests/unit/errors/test_error_declarative.py (5)
src/omnibase_core/errors/exception_adapter_binding_error.py (1)
AdapterBindingError(34-99)src/omnibase_core/enums/enum_core_error_code.py (1)
EnumCoreErrorCode(9-241)src/omnibase_core/errors/error_runtime_host.py (1)
RuntimeHostError(38-102)src/omnibase_core/errors/exception_purity_violation_error.py (1)
PurityViolationError(34-100)src/omnibase_core/errors/exception_node_execution_error.py (1)
NodeExecutionError(34-99)
src/omnibase_core/runtime/envelope_router.py (1)
src/omnibase_core/decorators/decorator_error_handling.py (1)
standard_error_handling(26-93)
src/omnibase_core/models/container/model_onex_container.py (3)
src/omnibase_core/decorators/decorator_allow_dict_any.py (1)
allow_dict_any(24-70)src/omnibase_core/decorators/decorator_pattern_exclusions.py (1)
allow_dict_any(81-99)src/omnibase_core/mixins/mixin_effect_execution.py (1)
allow_dict_any(123-146)
src/omnibase_core/errors/exception_adapter_binding_error.py (1)
src/omnibase_core/errors/error_runtime_host.py (1)
RuntimeHostError(38-102)
tests/unit/errors/test_error_runtime.py (5)
src/omnibase_core/errors/error_runtime_host.py (1)
RuntimeHostError(38-102)src/omnibase_core/errors/error_handler_execution.py (1)
HandlerExecutionError(16-79)src/omnibase_core/errors/error_event_bus.py (1)
EventBusError(16-68)src/omnibase_core/errors/error_invalid_operation.py (1)
InvalidOperationError(16-68)src/omnibase_core/errors/error_contract_validation.py (1)
ContractValidationError(16-68)
src/omnibase_core/protocols/__init__.py (1)
src/omnibase_core/protocols/protocol_core.py (1)
ProtocolCanonicalSerializer(27-105)
src/omnibase_core/tools/mypy_plugins/dict_any_checker.py (1)
src/omnibase_core/decorators/decorator_allow_dict_any.py (1)
allow_dict_any(24-70)
src/omnibase_core/errors/exception_purity_violation_error.py (1)
src/omnibase_core/errors/error_runtime_host.py (1)
RuntimeHostError(38-102)
src/omnibase_core/models/operations/model_operationparameters.py (1)
src/omnibase_core/decorators/decorator_error_handling.py (1)
standard_error_handling(26-93)
tests/unit/decorators/test_convert_to_schema.py (1)
src/omnibase_core/decorators/decorator_convert_to_schema.py (2)
_parse_version_component(172-224)_escape_field_name_for_validator(278-303)
src/omnibase_core/decorators/__init__.py (2)
src/omnibase_core/decorators/decorator_allow_dict_any.py (1)
allow_dict_any(24-70)src/omnibase_core/decorators/decorator_pattern_exclusions.py (1)
allow_dict_any(81-99)
src/omnibase_core/errors/__init__.py (4)
src/omnibase_core/errors/error_callable_not_found.py (1)
CallableNotFoundError(10-18)src/omnibase_core/errors/error_runtime_host.py (1)
RuntimeHostError(38-102)src/omnibase_core/errors/error_unknown_dependency.py (1)
UnknownDependencyError(10-22)src/omnibase_core/errors/exception_compute_pipeline_error.py (1)
ComputePipelineError(56-129)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Code Quality
- GitHub Check: claude-review
🔇 Additional comments (40)
src/omnibase_core/errors/error_runtime.py (1)
28-28: LGTM! Usage documentation updated correctly.The import path in the usage example now correctly references
error_runtimeinstead of the oldruntime_errorsmodule name.src/omnibase_core/errors/exception_unsupported_capability_error.py (1)
31-31: LGTM! Import path updated correctly.The
RuntimeHostErrorimport now references the renamed moduleerror_runtime.src/omnibase_core/errors/exception_adapter_binding_error.py (1)
31-31: LGTM! Import path updated correctly.The
RuntimeHostErrorimport now references the renamed moduleerror_runtime.src/omnibase_core/errors/exception_node_execution_error.py (1)
31-31: LGTM! Import path updated correctly.The
RuntimeHostErrorimport now references the renamed moduleerror_runtime.src/omnibase_core/protocols/__init__.py (2)
26-26: LGTM! Comment updated correctly.The module organization comment now correctly references
protocol_core.pyinstead of the generic term.
181-181: LGTM! Import path updated correctly.The
ProtocolCanonicalSerializerimport now references the renamed moduleprotocol_core.src/omnibase_core/models/effect/model_effect_input.py (1)
73-73: LGTM! Clean import path update.The import path correctly reflects the file rename from
allow_dict_any.py→decorator_allow_dict_any.py. The decorator signature and behavior remain unchanged.src/omnibase_core/models/container/model_onex_container.py (1)
37-37: LGTM! Import path updated correctly.The import aligns with the decorator file rename. Usage at line 177 remains consistent with the decorator's signature.
src/omnibase_core/models/services/model_node_service_config.py (1)
17-17: LGTM! Import path reflects file rename.The updated import path correctly points to
decorator_error_handling.py. Thestandard_error_handlingdecorator signature and behavior are preserved, as confirmed by usage at line 189.src/omnibase_core/services/service_protocol_auditor.py (1)
15-18: LGTM! Exception imports updated correctly.The import path now references
exception_base.py(renamed fromexceptions.py). The exception classesExceptionConfigurationErrorandExceptionInputValidationErrormaintain their interfaces and are used correctly at lines 84, 86, 177, and 179.src/omnibase_core/models/operations/model_operationparameters.py (1)
3-3: LGTM! Import path updated for decorator file rename.The import correctly references
decorator_error_handling.py. The decorator usage at line 39 remains consistent with the unchanged decorator signature.src/omnibase_core/models/capabilities/model_capability_requirement_set.py (1)
55-55: LGTM: Import path updated correctly.The import path change from
omnibase_core.decorators.allow_dict_anytoomnibase_core.decorators.decorator_allow_dict_anyfollows the systematic naming convention refactor. The decorator functionality and usage remain unchanged.tests/unit/validation/test_exception_base.py (1)
9-18: LGTM: Import path updated correctly.The import path change from
omnibase_core.errors.exceptionstoomnibase_core.errors.exception_basefollows the systematic naming convention refactor for error modules. All imported exception classes remain functionally unchanged.src/omnibase_core/models/operations/model_effect_operation_config.py (1)
38-38: LGTM: Import path updated correctly.The import path change from
omnibase_core.decorators.allow_dict_anytoomnibase_core.decorators.decorator_allow_dict_anyis consistent with the systematic decorator module renaming. Decorator usage throughout the file (lines 225, 236) remains unchanged and functional.src/omnibase_core/models/intents/model_registration_record_base.py (1)
64-64: LGTM: Import path updated correctly.The import path change from
omnibase_core.decorators.allow_dict_anytoomnibase_core.decorators.decorator_allow_dict_anyaligns with the systematic decorator module renaming. The decorator usage at line 113 remains unchanged and functional.CLAUDE.md (1)
392-392: LGTM: Documentation import path updated correctly.The import path change from
omnibase_core.decorators.error_handlingtoomnibase_core.decorators.decorator_error_handlingkeeps the documentation example synchronized with the systematic decorator module renaming. This ensures developers see the correct import path in the project documentation.docs/reference/templates/ORCHESTRATOR_NODE_TEMPLATE.md (1)
90-90: LGTM! Import paths correctly updated in template.The template now references the renamed decorator module
decorator_error_handling, ensuring users copying this template will use the correct import paths. Both occurrences are consistently updated.Also applies to: 1609-1609
src/omnibase_core/models/context/model_effect_template_context.py (1)
48-48: LGTM! Import path updated correctly.The import path has been updated to reflect the renamed decorator module. The decorator usage throughout the file (lines 53, 158, 175) remains unchanged, and static analysis confirms the import resolution is correct.
tests/unit/decorators/test_pattern_exclusions.py (1)
19-28: LGTM! Test imports updated correctly.The import statement has been updated to reference the renamed module
decorator_pattern_exclusions. All imported symbols remain the same, and the test logic is unchanged. This ensures tests continue to validate the decorator functionality under the new module structure.src/omnibase_core/errors/exception_purity_violation_error.py (1)
31-31: LGTM! Error module import updated correctly.The import path has been updated from
runtime_errorstoerror_runtime, following the new naming convention. TheRuntimeHostErrorbase class usage at line 34 remains unchanged, and type checking confirms the inheritance structure is intact.src/omnibase_core/errors/error_declarative.py (1)
21-26: LGTM! Documentation updated to reflect new module path.The docstring usage example has been updated to show the correct import path
omnibase_core.errors.error_declarative, aligning with the file's new name. This ensures developers referencing this documentation will use the correct import paths.docs/architecture/EXECUTION_SHAPE_EXAMPLES.md (1)
326-326: LGTM! Import paths correctly updated.The documentation examples now reference the renamed module
decorator_error_handling, maintaining consistency with the refactoring.Also applies to: 667-667
src/omnibase_core/runtime/envelope_router.py (1)
49-49: LGTM! Import path correctly updated.The import now references the renamed module
decorator_error_handling, consistent with the refactoring objectives.tests/unit/decorators/test_error_handling_extended.py (1)
18-22: LGTM! Import paths correctly updated.The test file now imports all three decorators from the renamed module
decorator_error_handling, maintaining test coverage for the refactored code.docs/reference/api/utils.md (1)
23-23: LGTM! Documentation correctly updated.All import examples now reference the renamed module
decorator_error_handling, ensuring developers use the correct import paths.Also applies to: 57-57, 293-293, 361-361
src/omnibase_core/tools/mypy_plugins/dict_any_checker.py (1)
65-65: Import path correctly updated.The import statement now references the renamed module
decorator_allow_dict_any.docs/release/V040_RELEASE_CHECKLIST.md (1)
1201-1201: ✅ Import path correctly updated to new error module.The import path for
NodeExecutionErroris correctly updated fromomnibase_core.errors.declarative_errorstoomnibase_core.errors.error_declarative, aligning with the PR's module reorganization.docs/architecture/adr/ADR-012-VALIDATOR-ERROR-HANDLING.md (1)
220-221: ✅ Decorator import path correctly updated.The import for
standard_error_handlingdecorator is correctly updated fromomnibase_core.decorators.error_handlingtoomnibase_core.decorators.decorator_error_handling, consistent with the PR's decorator module reorganization.docs/reference/templates/EFFECT_NODE_TEMPLATE.md (1)
95-95: ✅ Decorator import path correctly updated in template.The import for
standard_error_handlingis correctly updated fromomnibase_core.decorators.error_handlingtoomnibase_core.decorators.decorator_error_handling. This template is a critical reference for developers; the updated import path ensures it provides correct guidance for new node implementations.tests/unit/decorators/test_convert_to_schema.py (1)
19-23: LGTM! Import paths updated correctly.All import statements have been consistently updated from
omnibase_core.decorators.convert_to_schematoomnibase_core.decorators.decorator_convert_to_schema, aligning with the new naming convention. No functional changes detected.Also applies to: 724-726, 734-736, 748-750, 760-762, 769-771, 780-782, 793-795, 813-815, 831-833, 850-852, 866-868, 875-877, 884-886, 892-894, 902-904
tests/unit/errors/test_error_declarative.py (2)
12-12: LGTM! Documentation reference updated correctly.Comment now correctly references the renamed module
error_declarative.py.
47-47: LGTM! Error class imports updated correctly.All imports of declarative error classes have been consistently updated from
omnibase_core.errors.declarative_errorstoomnibase_core.errors.error_declarative, andRuntimeHostErrorimports updated fromerror_runtime_errorstoerror_runtime. Changes align with the module reorganization.Also applies to: 59-59, 78-78, 98-98, 116-116, 124-124, 134-135, 147-147, 164-164, 176-176, 195-195, 215-215, 235-235, 243-243, 253-254, 265-265, 286-287, 303-303, 315-315, 334-334, 354-354, 374-374, 382-382, 392-393, 404-405, 426-427, 443-443, 455-455, 474-474, 494-494, 514-514, 522-522, 532-533, 544-544, 566-567, 583-588, 604-610, 624-629, 652-657, 675-680, 697-702, 724-724, 739-739, 753-753, 767-767, 787-787, 805-807, 824-826, 846-847, 872-877, 916-927
tests/unit/errors/test_error_runtime.py (1)
24-24: LGTM! Runtime error imports updated correctly.All import statements have been consistently updated from
omnibase_core.errors.runtime_errorstoomnibase_core.errors.error_runtime, aligning with the new error module naming convention. No functional changes.Also applies to: 38-38, 50-50, 61-61, 84-84, 97-97, 110-110, 123-123, 147-147, 157-157, 168-168, 187-187, 197-197, 208-211, 226-226, 236-236, 247-247, 271-277, 294-294, 303-307, 319-319
docs/architecture/decisions/ADR-003-reducer-output-exception-consistency.md (1)
413-413: LGTM! Documentation updated correctly.Import path in the code example correctly updated from
omnibase_core.decorators.error_handlingtoomnibase_core.decorators.decorator_error_handling, ensuring documentation stays in sync with the decorator module rename.src/omnibase_core/validation/__init__.py (1)
32-36: LGTM! Exception imports updated correctly.Import source updated from
omnibase_core.errors.exceptionstoomnibase_core.errors.exception_basefor the three exception classes. Public exports remain unchanged, maintaining API compatibility.src/omnibase_core/errors/__init__.py (4)
7-30: LGTM: TYPE_CHECKING imports correctly updated to new module names.The imports have been systematically updated to reflect the new module naming convention (e.g.,
error_declarative,error_runtime,error_callable_not_found). All error classes remain accessible through the same public API.
126-127: LGTM: Comments accurately reflect new module structure.The documentation correctly references the new module names
error_runtimeanderror_declarative.
180-182: LGTM: Lazy loading correctly updated for runtime errors.The lazy import mechanism properly references
error_runtime(wasruntime_errors), maintaining the deferred loading behavior while using the new module path.
207-209: LGTM: Lazy loading correctly updated for declarative errors.The lazy import mechanism properly references
error_declarative(wasdeclarative_errors), maintaining the deferred loading behavior while using the new module path.src/omnibase_core/decorators/__init__.py (1)
6-23: LGTM: Decorator imports correctly updated to new module names.All decorator imports have been systematically updated to use the new naming convention with the
decorator_prefix. The public API is preserved through re-exports.Note:
allow_dict_anyexists as two separate implementations — one indecorator_allow_dict_any.py(with optionalreasonparameter) and another indecorator_pattern_exclusions.py(simplified, pass-through version). The public API exports only the full version fromdecorator_allow_dict_any.py. Consider whether both implementations should coexist or if the duplicate indecorator_pattern_exclusions.pyshould be consolidated with the primary implementation to avoid confusion and maintain DRY principles.
…renames - Update documentation references to use new file paths (decorator_error_handling, error_declarative) - Add comprehensive docstrings to protocol_contract_profile_factory.py - Fix duplicate section headers in docs - Add 41 import validation tests verifying renamed imports work correctly
PR Review: File Naming Convention Alignment (OMN-1217, OMN-1218, OMN-1219)OverviewThis PR implements a systematic refactoring to align file naming conventions across three critical directories (decorators/, errors/, protocols/). The changes are well-structured, comprehensive, and follow best practices for large-scale refactoring. ✅ Strengths1. Excellent Refactoring Methodology
2. Strong Test CoverageThe new test file demonstrates excellent practices:
3. Proper Lazy Loading for Circular Import PreventionThe errors/init.py uses proper lazy loading via getattr which is the correct pattern for avoiding circular dependencies in Python. 4. Comprehensive Scope
🔍 Code Quality AssessmentType Safety: ✅ EXCELLENT
Error Handling: ✅ EXCELLENT
Documentation: ✅ COMPREHENSIVEUpdated files include:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
docs/reference/api/utils.md (2)
22-30: Add language specifier to code block.The fenced code block should specify
pythonas the language for proper syntax highlighting.🔎 Proposed fix
-``` +```python from omnibase_core.decorators.decorator_error_handling import standard_error_handling
56-63: Add language specifier to code block.The fenced code block should specify
pythonas the language for proper syntax highlighting.🔎 Proposed fix
-``` +```python from omnibase_core.decorators.decorator_error_handling import validation_error_handling
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
docs/MVP_PLAN.mddocs/reference/api/utils.mddocs/reference/templates/ORCHESTRATOR_NODE_TEMPLATE.mdsrc/omnibase_core/protocols/protocol_contract_profile_factory.pytests/unit/api_snapshots/test_renamed_imports_pr317.pytests/unit/decorators/test_error_handling_extended.pytests/unit/decorators/test_pattern_exclusions.pytests/unit/errors/test_error_declarative.py
✅ Files skipped from review due to trivial changes (1)
- docs/MVP_PLAN.md
🚧 Files skipped from review as they are similar to previous changes (3)
- docs/reference/templates/ORCHESTRATOR_NODE_TEMPLATE.md
- tests/unit/decorators/test_pattern_exclusions.py
- tests/unit/decorators/test_error_handling_extended.py
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{py,sh}
📄 CodeRabbit inference engine (CLAUDE.md)
Always use
poetry runfor Python commands (pytest, mypy, pip, python -m). Never use pip or python directly.
Files:
src/omnibase_core/protocols/protocol_contract_profile_factory.pytests/unit/api_snapshots/test_renamed_imports_pr317.pytests/unit/errors/test_error_declarative.py
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Always callsuper().__init__(container)in node class constructors after inheriting from base node classes (NodeEffect, NodeCompute, NodeReducer, NodeOrchestrator). This eliminates 80+ lines of boilerplate code.
Use ModelONEXContainer for dependency injection in node constructors, NOT ModelContainer. ModelContainer is for value wrapping only.
Use protocol names (not concrete class names) when resolving services via DI:container.get_service("ProtocolEventBus")instead ofcontainer.get_service("EventBusService").
Use ModelOnexError with EnumCoreErrorCode for error handling instead of generic Exception.
Use @standard_error_handling decorator from omnibase_core.decorators.error_handling to eliminate try/catch boilerplate in async methods.
Add standardized comment markers when using catch-all or broad exception handlers: # fallback-ok:, # catch-all-ok:, # cleanup-resilience-ok:, # boundary-ok:, # init-errors-ok:, # tool-resilience-ok:
Order exception tuples alphabetically by exception class name in except clauses.
Use centralized exception groups from omnibase_core.errors.exception_groups (e.g., PYDANTIC_MODEL_ERRORS, VALIDATION_ERRORS) instead of hardcoding exception tuples.
Addfrom_attributes=Trueto ConfigDict for immutable value objects nested in other Pydantic models to support pytest-xdist parallel execution.
Use PEP 604 union syntax (str | None) instead of legacy Union/Optional syntax (enforced by ruff UP007).
ORCHESTRATOR nodes CANNOT return typed results - they can only emit events and intents. Only COMPUTE nodes return typed results.
Use declarative YAML contracts for all nodes. Load contracts using FileRegistry:registry = FileRegistry(); contract = registry.load(Path("config/runtime_host.yaml"))
Use FileRegistry for contract loading with error handling for codes: FILE_NOT_FOUND, FILE_READ_ERROR, CONFIGURATION_PARSE_ERROR, CONTRACT_VALIDATION_ERROR, DUPLICATE_REGISTRATION, DIRECTORY_NOT_FOUND
Implement mixins for cross-cutting concerns...
Files:
src/omnibase_core/protocols/protocol_contract_profile_factory.pytests/unit/api_snapshots/test_renamed_imports_pr317.pytests/unit/errors/test_error_declarative.py
src/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.py: Achieve 100% mypy strict mode compliance with 0 errors across all source files.
Ensure both mypy AND pyright pass in CI without errors.
Files:
src/omnibase_core/protocols/protocol_contract_profile_factory.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Run pytest with markers for test organization: @pytest.mark.unit, @pytest.mark.integration, @pytest.mark.slow, @pytest.mark.smoke, @pytest.mark.performance
Files:
tests/unit/api_snapshots/test_renamed_imports_pr317.pytests/unit/errors/test_error_declarative.py
🧠 Learnings (3)
📚 Learning: 2026-01-03T21:44:56.028Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-03T21:44:56.028Z
Learning: Applies to **/*.py : Use ModelOnexError with EnumCoreErrorCode for error handling instead of generic Exception.
Applied to files:
tests/unit/api_snapshots/test_renamed_imports_pr317.pytests/unit/errors/test_error_declarative.pydocs/reference/api/utils.md
📚 Learning: 2026-01-03T21:44:56.028Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-03T21:44:56.028Z
Learning: Applies to **/*.py : Use standard_error_handling decorator from omnibase_core.decorators.error_handling to eliminate try/catch boilerplate in async methods.
Applied to files:
tests/unit/api_snapshots/test_renamed_imports_pr317.pydocs/reference/api/utils.md
📚 Learning: 2026-01-03T21:44:56.028Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-03T21:44:56.028Z
Learning: Applies to **/*.py : Use centralized exception groups from omnibase_core.errors.exception_groups (e.g., PYDANTIC_MODEL_ERRORS, VALIDATION_ERRORS) instead of hardcoding exception tuples.
Applied to files:
tests/unit/errors/test_error_declarative.pydocs/reference/api/utils.md
🧬 Code graph analysis (3)
src/omnibase_core/protocols/protocol_contract_profile_factory.py (2)
src/omnibase_core/factories/contract_profile_factory.py (2)
available_profiles(290-325)available_profiles(351-353)src/omnibase_core/enums/enum_node_type.py (1)
EnumNodeType(96-320)
tests/unit/api_snapshots/test_renamed_imports_pr317.py (7)
src/omnibase_core/decorators/decorator_error_handling.py (2)
io_error_handling(157-216)standard_error_handling(26-93)src/omnibase_core/errors/exception_adapter_binding_error.py (1)
AdapterBindingError(34-99)src/omnibase_core/errors/error_runtime_host.py (1)
RuntimeHostError(38-102)src/omnibase_core/protocols/protocol_core.py (1)
ProtocolCanonicalSerializer(27-105)src/omnibase_core/protocols/protocol_import_tracker.py (1)
ProtocolImportTracker(13-36)src/omnibase_core/protocols/protocol_logger_like.py (1)
ProtocolLoggerLike(18-51)src/omnibase_core/protocols/protocol_payload_data.py (1)
ProtocolPayloadData(28-125)
tests/unit/errors/test_error_declarative.py (5)
src/omnibase_core/errors/exception_adapter_binding_error.py (1)
AdapterBindingError(34-99)src/omnibase_core/enums/enum_core_error_code.py (1)
EnumCoreErrorCode(9-241)src/omnibase_core/errors/exception_purity_violation_error.py (1)
PurityViolationError(34-100)src/omnibase_core/errors/exception_node_execution_error.py (1)
NodeExecutionError(34-99)src/omnibase_core/errors/exception_unsupported_capability_error.py (1)
UnsupportedCapabilityError(34-99)
🪛 LanguageTool
docs/reference/api/utils.md
[grammar] ~18-~18: Use a hyphen to join words.
Context: ...: Decorator that provides standard error handling pattern for ONEX operations. T...
(QB_NEW_EN_HYPHEN)
[grammar] ~20-~20: Use a hyphen to join words.
Context: ...eliminates 6+ lines of boilerplate error handling code and ensures consistent err...
(QB_NEW_EN_HYPHEN)
🪛 markdownlint-cli2 (0.18.1)
docs/reference/api/utils.md
22-22: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
56-56: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (21)
- GitHub Check: Tests (Split 18/20)
- GitHub Check: Tests (Split 16/20)
- GitHub Check: Tests (Split 13/20)
- GitHub Check: Tests (Split 20/20)
- GitHub Check: Tests (Split 15/20)
- GitHub Check: Tests (Split 19/20)
- GitHub Check: Tests (Split 11/20)
- GitHub Check: Tests (Split 12/20)
- GitHub Check: Tests (Split 17/20)
- GitHub Check: Tests (Split 14/20)
- GitHub Check: Tests (Split 5/20)
- GitHub Check: Tests (Split 10/20)
- GitHub Check: Tests (Split 9/20)
- GitHub Check: Tests (Split 7/20)
- GitHub Check: Tests (Split 8/20)
- GitHub Check: Tests (Split 6/20)
- GitHub Check: Tests (Split 3/20)
- GitHub Check: Tests (Split 1/20)
- GitHub Check: Tests (Split 2/20)
- GitHub Check: Tests (Split 4/20)
- GitHub Check: claude-review
🔇 Additional comments (6)
docs/reference/api/utils.md (1)
16-30: Documentation updates look good.The import path updates from
omnibase_core.decorators.error_handlingtoomnibase_core.decorators.decorator_error_handlingare correct and align with the PR objectives.tests/unit/errors/test_error_declarative.py (1)
1-951: LGTM! Systematic import path updates are correct.All import statements have been correctly updated to reflect the file renames:
declarative_errors→error_declarativeruntime_errors→error_runtimeThe test logic remains unchanged, and the comprehensive test coverage for declarative errors is preserved. The updates align perfectly with the PR objectives for OMN-1217, OMN-1218, and OMN-1219.
tests/unit/api_snapshots/test_renamed_imports_pr317.py (1)
1-554: Excellent comprehensive test coverage for the PR #317 renames!This test file provides thorough validation of all renamed imports across decorators, errors, and protocols. The test structure is well-organized and covers:
✅ Direct imports from new module paths
✅ Re-exports from__init__.pymodules
✅__all__exports validation
✅ Import consistency (identity checks)
✅ Circular import prevention
✅ Functional decorator testsThe comprehensive coverage provides strong confidence that the renames preserve all public APIs and don't introduce breaking changes.
src/omnibase_core/protocols/protocol_contract_profile_factory.py (3)
1-14: LGTM! Well-structured module documentation.The expanded module docstring clearly articulates the design principles and aligns with the coding guidelines for protocol-first design and type safety.
88-102: LGTM! Clear method documentation.The docstring accurately documents the method contract, including the important note that the returned list may be empty.
56-86: Protocol documentation does not match actual implementation exception behavior.The documented exceptions (KeyError for missing profiles, ValueError for invalid version) do not match the actual implementation. The concrete implementation in
contract_profile_factory.pyraisesOnexErrorwithEnumCoreErrorCode.VALIDATION_ERRORfor missing profiles and performs no semantic version validation at all. Update the protocol docstring to reflect what implementations actually raise:Raises: OnexError: If the requested profile does not exist for the given node type or if the node type is invalid.Likely an incorrect or invalid review comment.
| Example: | ||
| .. code-block:: python | ||
|
|
||
| from omnibase_core.protocols import ProtocolContractProfileFactory | ||
| from omnibase_core.enums import EnumNodeType | ||
|
|
||
| def create_node_contract( | ||
| factory: ProtocolContractProfileFactory, | ||
| node_type: EnumNodeType, | ||
| ) -> ModelContractBase: | ||
| '''Create a default contract for a node type.''' | ||
| # Check available profiles | ||
| profiles = factory.available_profiles(node_type) | ||
| if "default" in profiles: | ||
| return factory.get_profile(node_type, "default") | ||
| return factory.get_profile(node_type, profiles[0]) | ||
| """ |
There was a problem hiding this comment.
Fix IndexError risk in example code.
The example accesses profiles[0] without checking if the list is empty. According to the available_profiles documentation (Line 100), the method "May be empty if no profiles are defined for the node type," which would cause an IndexError at Line 53.
🔎 Suggested fix for the example
def create_node_contract(
factory: ProtocolContractProfileFactory,
node_type: EnumNodeType,
) -> ModelContractBase:
'''Create a default contract for a node type.'''
# Check available profiles
profiles = factory.available_profiles(node_type)
+ if not profiles:
+ raise ValueError(f"No profiles available for {node_type}")
if "default" in profiles:
return factory.get_profile(node_type, "default")
return factory.get_profile(node_type, profiles[0])🤖 Prompt for AI Agents
In src/omnibase_core/protocols/protocol_contract_profile_factory.py around lines
38 to 54, the example can raise IndexError because it uses profiles[0] without
ensuring profiles is non-empty; update the example to check if profiles is empty
before indexing and handle that case (e.g., if not profiles: raise a clear
ValueError or return None) otherwise proceed to return
factory.get_profile(node_type, profiles[0]).
The PR #317 renamed several files (decorators/, errors/, protocols/) but CI was using cached virtualenv from before the renames. This caused ModuleNotFoundError for old module paths that no longer exist. Bumping CACHE_VERSION from 0.1.4 to 0.1.5 forces cache invalidation, ensuring CI installs fresh dependencies with the new file structure. Error seen in CI: - ModuleNotFoundError: No module named 'omnibase_core.decorators.allow_dict_any' - ImportError: cannot import name 'declarative_errors' from 'omnibase_core.errors' Related: OMN-1217, OMN-1218, OMN-1219
Code Review - PR #317: File Naming Convention ComplianceSummaryThis PR successfully renames 8 core files across decorators/, errors/, and protocols/ directories to follow ONEX naming conventions, updating all 41+ dependent files accordingly. ✅ Strengths1. Comprehensive Import Updates
2. Excellent Test Coverage
3. Documentation Updates
4. Consistent Naming Pattern
5. Low-Risk Refactor
🔍 Code Quality ObservationsProtocol-Driven DI Compliance ✅The PR correctly maintains protocol-based imports throughout - container.get_service() calls remain unchanged. Error Handling Pattern ✅The decorator import updates follow CLAUDE.md guidelines, changing from omnibase_core.decorators.error_handling to omnibase_core.decorators.decorator_error_handling. Thread Safety ✅No changes to thread safety semantics - purely file renames. 📋 Validation Checklist
🎯 Performance ConsiderationsImpact: Minimal
🔒 Security ConcernsImpact: None identified
📊 Test Coverage AnalysisNew Tests Addedtests/unit/api_snapshots/test_renamed_imports_pr317.py (554 lines) Coverage: Excellent
Test StructureThe TestDecoratorRenamedImports, TestErrorRenamedImports, and TestProtocolRenamedImports classes provide comprehensive coverage with both direct import and re-export validation. 🚀 RecommendationsBefore Merge
Post-Merge
📝 Minor Suggestions1. Test File Naming ConsistencyThe new test file test_renamed_imports_pr317.py is great for tracking the PR context, but consider renaming post-merge to something like test_import_paths.py or test_module_reexports.py. This makes it easier for future developers to understand the test's purpose without PR context. 2. Import Path DocumentationConsider adding a brief comment in each renamed file's docstring noting the rename from PR #317 for historical context. 🎉 Final AssessmentVerdict: ✅ APPROVE (pending CI completion) This is a well-executed refactoring that:
Risk Level: 🟢 Low
Impact: 🟡 Medium
📚 Related IssuesCloses:
Great work on maintaining code quality standards! 🚀 The systematic approach to renaming (plan → execute → test → document) is exemplary. This sets a strong precedent for future naming convention updates. Review performed by Claude Code following CLAUDE.md guidelines |
- Update decorator path constants in dict_any_checker.py to use new decorator_allow_dict_any module path after file rename - Remove duplicate "Compute Module Exports" section header from protocols/__init__.py Addresses PR #317 review feedback from CodeRabbit.
Code Review - PR #317SummaryThis PR renames 8 files across decorators/, errors/, and protocols/ directories to follow consistent naming conventions. The refactoring includes updating 41 import statements across source, tests, and documentation. ✅ Strengths
🔍 Observations
|
Resolved merge conflict in model_capability_requirement_set.py: - Used renamed decorator path (decorator_allow_dict_any) - Kept existing type_json import path Fixed import path in mixin_caching.py: - Updated allow_dict_any -> decorator_allow_dict_any
Redis class in redis-py 5.x no longer accepts type arguments. Fixes mypy and pyright type-arg errors.
Code Review - PR #317SummaryThis PR implements a systematic file naming convention refactor across three critical directories: ✅ Strengths1. Excellent Migration Execution
2. Strong Naming ConsistencyThe new naming conventions are clear and follow a logical pattern:
This makes it immediately clear what type of module you're importing from. 3. Thorough Testing StrategyThe
4. Documentation Updates
5. Type Safety FixThe Redis type annotation fix ( -self._client: Redis[str] | None = None
+self._client: Redis | None = NoneThis is correct because Redis 5.x no longer accepts type arguments. 🔍 Code Quality AssessmentAdherence to CLAUDE.md Standards✅ Poetry usage: All operations use poetry Potential Issues Found: NONEI specifically checked for:
📊 Impact AnalysisFiles Changed: 47
Migration Completeness🎯 Performance ConsiderationsCI Impact:
Import Performance:
🔐 Security ConsiderationsNo security concerns identified. This is a pure refactoring PR:
📋 Test CoverageExisting TestsThe PR maintains compatibility with the existing 12,000+ test suite by updating all import paths. New TestsAdded comprehensive import validation suite with:
Coverage Assessment: ⭐⭐⭐⭐⭐ Excellent The test suite specifically validates:
✨ Recommendations1. Monitor CI Completion (Required)The PR description shows CI tests are still pending. Ensure all 20 parallel splits pass before merging. 2. Consider Adding to Migration Docs (Optional)Consider documenting this naming convention change in a migration guide for external consumers of omnibase_core, similar to the existing 3. Verify Downstream Projects (If Applicable)If any downstream projects depend on omnibase_core and import these modules directly (not via
🎉 ConclusionVerdict: ✅ APPROVE (pending CI passing) This is a well-executed refactoring PR that:
The naming convention changes are sensible and will make the codebase more maintainable going forward. The migration was executed thoroughly with excellent attention to detail. Merge when: All CI checks pass (currently in progress) Checklist Summary
Great work @jonahgabriel! 🚀 Review performed by Claude Code following omnibase_core CLAUDE.md standards |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
CLAUDE.mdsrc/omnibase_core/backends/cache/backend_cache_redis.pysrc/omnibase_core/mixins/mixin_caching.pysrc/omnibase_core/models/capabilities/model_capability_requirement_set.pysrc/omnibase_core/protocols/__init__.pysrc/omnibase_core/validation/__init__.py
🚧 Files skipped from review as they are similar to previous changes (2)
- src/omnibase_core/protocols/init.py
- src/omnibase_core/models/capabilities/model_capability_requirement_set.py
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{py,sh}
📄 CodeRabbit inference engine (CLAUDE.md)
Always use
poetry runfor Python commands (pytest, mypy, pip, python -m). Never use pip or python directly.
Files:
src/omnibase_core/backends/cache/backend_cache_redis.pysrc/omnibase_core/mixins/mixin_caching.pysrc/omnibase_core/validation/__init__.py
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Always callsuper().__init__(container)in node class constructors after inheriting from base node classes (NodeEffect, NodeCompute, NodeReducer, NodeOrchestrator). This eliminates 80+ lines of boilerplate code.
Use ModelONEXContainer for dependency injection in node constructors, NOT ModelContainer. ModelContainer is for value wrapping only.
Use protocol names (not concrete class names) when resolving services via DI:container.get_service("ProtocolEventBus")instead ofcontainer.get_service("EventBusService").
Use ModelOnexError with EnumCoreErrorCode for error handling instead of generic Exception.
Use @standard_error_handling decorator from omnibase_core.decorators.error_handling to eliminate try/catch boilerplate in async methods.
Add standardized comment markers when using catch-all or broad exception handlers: # fallback-ok:, # catch-all-ok:, # cleanup-resilience-ok:, # boundary-ok:, # init-errors-ok:, # tool-resilience-ok:
Order exception tuples alphabetically by exception class name in except clauses.
Use centralized exception groups from omnibase_core.errors.exception_groups (e.g., PYDANTIC_MODEL_ERRORS, VALIDATION_ERRORS) instead of hardcoding exception tuples.
Addfrom_attributes=Trueto ConfigDict for immutable value objects nested in other Pydantic models to support pytest-xdist parallel execution.
Use PEP 604 union syntax (str | None) instead of legacy Union/Optional syntax (enforced by ruff UP007).
ORCHESTRATOR nodes CANNOT return typed results - they can only emit events and intents. Only COMPUTE nodes return typed results.
Use declarative YAML contracts for all nodes. Load contracts using FileRegistry:registry = FileRegistry(); contract = registry.load(Path("config/runtime_host.yaml"))
Use FileRegistry for contract loading with error handling for codes: FILE_NOT_FOUND, FILE_READ_ERROR, CONFIGURATION_PARSE_ERROR, CONTRACT_VALIDATION_ERROR, DUPLICATE_REGISTRATION, DIRECTORY_NOT_FOUND
Implement mixins for cross-cutting concerns...
Files:
src/omnibase_core/backends/cache/backend_cache_redis.pysrc/omnibase_core/mixins/mixin_caching.pysrc/omnibase_core/validation/__init__.py
src/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.py: Achieve 100% mypy strict mode compliance with 0 errors across all source files.
Ensure both mypy AND pyright pass in CI without errors.
Files:
src/omnibase_core/backends/cache/backend_cache_redis.pysrc/omnibase_core/mixins/mixin_caching.pysrc/omnibase_core/validation/__init__.py
🧠 Learnings (3)
📚 Learning: 2026-01-03T21:44:56.028Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-03T21:44:56.028Z
Learning: Applies to **/*.py : Use standard_error_handling decorator from omnibase_core.decorators.error_handling to eliminate try/catch boilerplate in async methods.
Applied to files:
CLAUDE.md
📚 Learning: 2026-01-03T21:44:56.028Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-03T21:44:56.028Z
Learning: Applies to **/*.py : Use centralized exception groups from omnibase_core.errors.exception_groups (e.g., PYDANTIC_MODEL_ERRORS, VALIDATION_ERRORS) instead of hardcoding exception tuples.
Applied to files:
src/omnibase_core/validation/__init__.py
📚 Learning: 2026-01-03T21:44:56.028Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-03T21:44:56.028Z
Learning: Applies to **/*.py : Use ModelOnexError with EnumCoreErrorCode for error handling instead of generic Exception.
Applied to files:
src/omnibase_core/validation/__init__.py
🧬 Code graph analysis (1)
src/omnibase_core/mixins/mixin_caching.py (1)
src/omnibase_core/mixins/mixin_effect_execution.py (1)
allow_dict_any(123-146)
🔇 Additional comments (4)
src/omnibase_core/validation/__init__.py (1)
32-36: LGTM! Import path updated correctly.The import path change from
omnibase_core.errors.exceptionstoomnibase_core.errors.exception_basealigns with the PR's file renaming objectives. The three exception classes remain properly re-exported, preserving the public API surface.src/omnibase_core/mixins/mixin_caching.py (1)
67-67: LGTM: Import path correctly updated.The import path has been properly updated to reflect the file rename from
allow_dict_any.pytodecorator_allow_dict_any.py. The decorator usage at line 406 remains unchanged and correct.CLAUDE.md (1)
442-442: No action required—import path is valid.The
omnibase_core.errors.exception_groupsimport path remains valid and accessible. Theexception_groups.pymodule was not affected by the error module renames, as it uses theexception_*naming convention rather thanerror_*. The module and its exception group constants (PYDANTIC_MODEL_ERRORS, VALIDATION_ERRORS, etc.) are properly imported and exported inerrors/__init__.py.src/omnibase_core/backends/cache/backend_cache_redis.py (1)
350-350: Type annotation correctly updated for redis-py 5.x compatibility.Redis 5.0 removed support for generic type parameters, so
Redis | Noneis the correct annotation. This change resolves mypy strict mode type-argument errors and aligns with the updated library API.
|
|
||
| ```python | ||
| from omnibase_core.decorators.error_handling import standard_error_handling | ||
| from omnibase_core.decorators.decorator_error_handling import standard_error_handling |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if standard_error_handling is re-exported from decorators __init__
echo "=== Checking decorators/__init__.py for re-exports ==="
rg "standard_error_handling" src/omnibase_core/decorators/__init__.py -A 3
echo ""
echo "=== Checking if decorator_error_handling.py exists ==="
fd -t f "decorator_error_handling.py" src/omnibase_core/decorators/
echo ""
echo "=== Checking error module structure and exception_groups ==="
fd -t f "exception_groups" src/omnibase_core/errors/
echo ""
echo "=== Checking decorators/__init__.py full content for imports ==="
cat src/omnibase_core/decorators/__init__.py
echo ""
echo "=== Checking errors/__init__.py for re-exports ==="
cat src/omnibase_core/errors/__init__.pyRepository: OmniNode-ai/omnibase_core
Length of output: 10978
Update import to follow public API conventions.
The import at line 399 currently uses the direct module path:
from omnibase_core.decorators.decorator_error_handling import standard_error_handlingWhile this works, the standard_error_handling decorator is already re-exported in the public API (src/omnibase_core/decorators/__init__.py). Update the documentation to use the package-level import for stability and to guide users toward the intended public API:
from omnibase_core.decorators import standard_error_handlingThis ensures documentation demonstrates the stable, supported import pattern rather than internal module paths.
🤖 Prompt for AI Agents
In CLAUDE.md around line 399, the import uses the internal module path; replace
the direct module import with the package-level re-export. Update the line to
import standard_error_handling from omnibase_core.decorators (i.e., use the
public API import) so the documentation demonstrates the supported, stable
import pattern.
1 participant
Summary
Rename files in
decorators/,errors/, andprotocols/directories to follow naming conventions.OMN-1217: decorators/ (4 files)
allow_dict_any.py→decorator_allow_dict_any.pyconvert_to_schema.py→decorator_convert_to_schema.pyerror_handling.py→decorator_error_handling.pypattern_exclusions.py→decorator_pattern_exclusions.pyOMN-1218: errors/ (3 files)
declarative_errors.py→error_declarative.pyexceptions.py→exception_base.pyruntime_errors.py→error_runtime.pyOMN-1219: protocols/ (1 file)
core.py→protocol_core.pyChanges
Test Plan
poetry run python -c "import omnibase_core")Related Issues
Closes OMN-1217
Closes OMN-1218
Closes OMN-1219
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.