Repository navigation
refactor(enums): replace hardcoded status strings with enum references (OMN-1309) - #385
Conversation
…s (OMN-1309) Replace hardcoded status strings with type-safe enum references in 4 priority files, maintaining backward-compatible JSON serialization. Changes: - Update EnumCircuitBreakerState to inherit from str,Enum for serialization - Add EnumNodeLifecycleStatus for node lifecycle states - Add EnumActionStatus for action metadata states - Update model_circuit_breaker.py to use EnumCircuitBreakerState - Update mixin_health_check.py to use EnumNodeHealthStatus - Update node_core_base.py to use EnumNodeLifecycleStatus - Update model_action_metadata.py to use EnumActionStatus - Add serialization snapshot tests verifying byte-for-byte JSON compatibility - Add mixin_health_check.py to single-class-per-file exclusion (same pattern as mixin_event_bus.py) - Fix fallback-ok comment placement for pre-commit hook detection Models accept both legacy strings and enum values on input, normalize internally, and serialize to lowercase strings for backward compatibility.
📝 WalkthroughWalkthroughAdds two new enums (action status, node lifecycle), converts circuit breaker enum to a string-backed enum, replaces literal state/status strings with enum.value across models, infrastructure, and health-check mixins, adds normalization validators, updates package exports, tweaks pre-commit exclusions, and adds tests for enum serialization and transitions. Changes
Sequence Diagram(s)Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used📓 Path-based instructions (4)**/*.py📄 CodeRabbit inference engine (CLAUDE.md)
Files:
src/omnibase_core/**/*.py📄 CodeRabbit inference engine (CLAUDE.md)
Files:
src/omnibase_core/models/**/*.py📄 CodeRabbit inference engine (CLAUDE.md)
Files:
src/omnibase_core/enums/enum_*.py📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🧠 Learnings (1)📚 Learning: 2026-01-13T16:21:16.950ZApplied to files:
🧬 Code graph analysis (3)src/omnibase_core/models/core/model_action_metadata.py (1)
src/omnibase_core/enums/enum_action_status.py (1)
src/omnibase_core/models/configuration/model_circuit_breaker.py (1)
⏰ 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). (20)
🔇 Additional comments (13)
Comment |
![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
🧹 Nitpick comments (3)
src/omnibase_core/enums/enum_action_status.py (1)
171-181: Consider makingvalid_transitionsa class-level constant.The
valid_transitionsdictionary is recreated on every call tocan_transition_to. Moving it to a class-level constant would avoid repeated allocation if this method is called frequently.♻️ Optional refactor
+ # Class-level constant for valid state transitions + _VALID_TRANSITIONS: ClassVar[dict[EnumActionStatus, set[EnumActionStatus]]] = { + CREATED: {READY, FAILED}, + READY: {RUNNING, FAILED}, + RUNNING: {COMPLETED, FAILED}, + COMPLETED: set(), + FAILED: set(), + } + def can_transition_to(self, target: "EnumActionStatus") -> bool: ... - valid_transitions: dict[EnumActionStatus, set[EnumActionStatus]] = { - EnumActionStatus.CREATED: {EnumActionStatus.READY, EnumActionStatus.FAILED}, - ... - } - return target in valid_transitions.get(self, set()) + return target in self._VALID_TRANSITIONS.get(self, set())src/omnibase_core/enums/enum_node_lifecycle_status.py (1)
15-62: Consider using StrValueHelper mixin per coding guidelines.The enum manually implements
__str__to returnself.value, which is functionally correct. However, per coding guidelines, string-based enums should use theStrValueHelpermixin for consistent behavior across the codebase.♻️ Suggested refactor using StrValueHelper
from enum import Enum, unique + +from omnibase_core.enums.mixin_str_value_helper import StrValueHelper @unique -class EnumNodeLifecycleStatus(str, Enum): +class EnumNodeLifecycleStatus(StrValueHelper, str, Enum): """ Node lifecycle status values. ... """ INITIALIZED = "initialized" ... - def __str__(self) -> str: - """Return the string value of the lifecycle status.""" - return self.value - def is_terminal(self) -> bool:Based on learnings, the coding guidelines specify: "Use StrValueHelper mixin for all string-based enums to ensure consistent str behavior returning self.value."
src/omnibase_core/models/core/model_action_metadata.py (1)
260-261: Prefer a set literal for membership testing.Using a set is more idiomatic for membership checks and has O(1) lookup (though negligible with 2 elements).
🔧 Suggested change
return ( self.trust_score >= minimum_trust_score and not self.is_expired() - and self.status - in [EnumActionStatus.CREATED.value, EnumActionStatus.READY.value] + and self.status in {EnumActionStatus.CREATED.value, EnumActionStatus.READY.value} )
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
.pre-commit-config.yamlsrc/omnibase_core/enums/__init__.pysrc/omnibase_core/enums/enum_action_status.pysrc/omnibase_core/enums/enum_circuit_breaker_state.pysrc/omnibase_core/enums/enum_node_lifecycle_status.pysrc/omnibase_core/infrastructure/node_core_base.pysrc/omnibase_core/mixins/mixin_health_check.pysrc/omnibase_core/models/configuration/model_circuit_breaker.pysrc/omnibase_core/models/core/model_action_metadata.pytests/unit/enums/test_enum_circuit_breaker_state.pytests/unit/models/test_enum_serialization_omn_1309.py
🧰 Additional context used
📓 Path-based instructions (5)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Always use Poetry for Python package management and task execution. Never use pip or python directly.
Use protocol names for dependency injection (e.g., 'ProtocolEventBus'), never concrete class names.
Use PEP 604 union syntax (X | Y) instead of Union[X, Y] and use Optional syntax is forbidden in favor of X | None.
Files:
src/omnibase_core/enums/__init__.pysrc/omnibase_core/mixins/mixin_health_check.pysrc/omnibase_core/models/core/model_action_metadata.pytests/unit/enums/test_enum_circuit_breaker_state.pysrc/omnibase_core/enums/enum_node_lifecycle_status.pysrc/omnibase_core/models/configuration/model_circuit_breaker.pysrc/omnibase_core/enums/enum_circuit_breaker_state.pytests/unit/models/test_enum_serialization_omn_1309.pysrc/omnibase_core/enums/enum_action_status.pysrc/omnibase_core/infrastructure/node_core_base.py
src/omnibase_core/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/omnibase_core/**/*.py: Always use ModelOnexError with error codes instead of generic Exception.
Use the @standard_error_handling decorator to eliminate try/catch boilerplate in async operations.
Python files in src/omnibase_core/ must follow directory-specific naming prefixes enforced by checker_naming_convention.py.
Never use relative imports across package boundaries. Use absolute imports for stability and readability.
All TODO comments must reference a Linear ticket in the format # TODO(OMN-XXXX): description or use # TODO(OMN-TBD): description [NEEDS TICKET].
All type: ignore comments must include specific error codes and explanations above the line.
Docstrings must explain WHY, describe non-obvious behavior, or document edge cases. Do not write tautological docstrings that restate the method name.
Exception tuples in except clauses must be alphabetically ordered by exception class name.
Use exception comment markers (fallback-ok:, catch-all-ok:, cleanup-resilience-ok:, boundary-ok:, init-errors-ok:, tool-resilience-ok:) to document intent for broad exception handlers.
Use centralized exception constants from omnibase_core.errors.exception_groups instead of writing exception tuples inline.
Both mypy and pyright must pass in CI with 100% strict mode compliance (0 errors across all source files).
Files:
src/omnibase_core/enums/__init__.pysrc/omnibase_core/mixins/mixin_health_check.pysrc/omnibase_core/models/core/model_action_metadata.pysrc/omnibase_core/enums/enum_node_lifecycle_status.pysrc/omnibase_core/models/configuration/model_circuit_breaker.pysrc/omnibase_core/enums/enum_circuit_breaker_state.pysrc/omnibase_core/enums/enum_action_status.pysrc/omnibase_core/infrastructure/node_core_base.py
src/omnibase_core/models/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/omnibase_core/models/**/*.py: Every Pydantic model must have an explicit model_config using ConfigDict. Empty ConfigDict() is not allowed.
Frozen Pydantic models must include from_attributes=True in ConfigDict for pytest-xdist compatibility.
Use default_factory for mutable Pydantic field defaults (list, dict) to avoid shared state bugs.
Files:
src/omnibase_core/models/core/model_action_metadata.pysrc/omnibase_core/models/configuration/model_circuit_breaker.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use @pytest.mark decorators (unit, integration, slow, smoke, performance) to categorize tests.
Files:
tests/unit/enums/test_enum_circuit_breaker_state.pytests/unit/models/test_enum_serialization_omn_1309.py
src/omnibase_core/enums/enum_*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use StrValueHelper mixin for all string-based enums to ensure consistent str behavior returning self.value.
Files:
src/omnibase_core/enums/enum_node_lifecycle_status.pysrc/omnibase_core/enums/enum_circuit_breaker_state.pysrc/omnibase_core/enums/enum_action_status.py
🧠 Learnings (4)
📚 Learning: 2026-01-13T16:21:16.950Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T16:21:16.950Z
Learning: Applies to src/omnibase_core/**/*.py : Python files in src/omnibase_core/ must follow directory-specific naming prefixes enforced by checker_naming_convention.py.
Applied to files:
.pre-commit-config.yaml
📚 Learning: 2026-01-13T16:21:16.950Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T16:21:16.950Z
Learning: Applies to src/omnibase_core/models/**/*.py : Every Pydantic model must have an explicit model_config using ConfigDict. Empty ConfigDict() is not allowed.
Applied to files:
src/omnibase_core/models/core/model_action_metadata.pysrc/omnibase_core/models/configuration/model_circuit_breaker.py
📚 Learning: 2026-01-13T16:21:16.950Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T16:21:16.950Z
Learning: Applies to src/omnibase_core/enums/enum_*.py : Use StrValueHelper mixin for all string-based enums to ensure consistent __str__ behavior returning self.value.
Applied to files:
tests/unit/enums/test_enum_circuit_breaker_state.pysrc/omnibase_core/enums/enum_circuit_breaker_state.py
📚 Learning: 2026-01-13T16:21:16.950Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T16:21:16.950Z
Learning: Applies to src/omnibase_core/**/*.py : Use centralized exception constants from omnibase_core.errors.exception_groups instead of writing exception tuples inline.
Applied to files:
src/omnibase_core/infrastructure/node_core_base.py
🧬 Code graph analysis (8)
src/omnibase_core/enums/__init__.py (1)
src/omnibase_core/enums/enum_action_status.py (1)
EnumActionStatus(24-181)
src/omnibase_core/mixins/mixin_health_check.py (2)
src/omnibase_core/models/node_metadata/model_node_metadata_info.py (3)
is_healthy(230-232)health(103-105)health(108-116)src/omnibase_core/models/node_metadata/model_node_core_metadata_class.py (1)
is_healthy(61-63)
src/omnibase_core/models/core/model_action_metadata.py (1)
src/omnibase_core/enums/enum_action_status.py (1)
EnumActionStatus(24-181)
tests/unit/enums/test_enum_circuit_breaker_state.py (1)
src/omnibase_core/enums/enum_circuit_breaker_state.py (1)
EnumCircuitBreakerState(22-117)
src/omnibase_core/enums/enum_node_lifecycle_status.py (2)
src/omnibase_core/protocols/event_bus/protocol_event_message.py (1)
value(33-35)src/omnibase_core/enums/enum_action_status.py (2)
is_terminal(61-77)is_active(79-94)
src/omnibase_core/models/configuration/model_circuit_breaker.py (1)
src/omnibase_core/enums/enum_circuit_breaker_state.py (1)
EnumCircuitBreakerState(22-117)
src/omnibase_core/enums/enum_action_status.py (1)
src/omnibase_core/enums/enum_node_lifecycle_status.py (2)
is_terminal(64-70)is_active(72-78)
src/omnibase_core/infrastructure/node_core_base.py (1)
src/omnibase_core/enums/enum_node_lifecycle_status.py (1)
EnumNodeLifecycleStatus(16-85)
🔇 Additional comments (27)
.pre-commit-config.yaml (1)
360-361: LGTM - justified exclusion for protocol definitions.The exclusion of
mixin_health_check.pyfrom single-class-per-file validation is appropriate since the file defines multiple duck-typed Protocol classes (ProtocolConnectionPool,ProtocolKafkaProducerAio, etc.) alongside the mixin implementation. This follows the existing pattern formixin_event_bus.py.src/omnibase_core/enums/enum_action_status.py (2)
1-56: Well-structured enum with comprehensive documentation.The enum definition, state machine documentation, and docstring examples are thorough. The
@uniquedecorator ensures value uniqueness, and the inheritance from(str, Enum)enables proper JSON serialization.
57-59: No changes needed. The__str__implementation inenum_action_status.pyis consistent with all other string-based enums in the codebase.StrValueHelperdoes not exist in the repository and is not used by any existing enums.src/omnibase_core/enums/__init__.py (2)
10-11: LGTM - proper import and categorization.The new
EnumActionStatusimport follows the established pattern with an appropriate ticket reference comment.
378-378: LGTM - correctly exported in Orchestrator domain.The export placement in the Orchestrator domain section is appropriate since actions are orchestrator workflow concepts.
src/omnibase_core/mixins/mixin_health_check.py (4)
260-266: LGTM - correct enum value comparison pattern.The comparison
result.status == EnumNodeHealthStatus.UNHEALTHY.valueis correct sinceModelHealthStatus.statusis a string field. Comparing against.valueensures type-safe string comparison while maintaining backward compatibility.
448-454: LGTM - consistent with sync health check pattern.The async health check follows the same enum value comparison pattern as the synchronous version, maintaining consistency.
530-530: LGTM - consistent enum-based health check.Using
EnumNodeHealthStatus.HEALTHY.valuefor theis_healthyboolean derivation aligns with the other status comparisons in this file.
571-571: LGTM - fallback-ok comment placement.The inline comment placement follows the project's exception handling documentation pattern.
src/omnibase_core/enums/enum_circuit_breaker_state.py (3)
22-22: LGTM - appropriate inheritance change for JSON serialization.Changing to
(str, Enum)enables proper JSON serialization where enum values serialize directly to their string values. This is the standard pattern for Pydantic-compatible enums.
70-71: Good documentation practice - versionchanged directive.The
versionchangeddirective clearly documents the backward-incompatible change for users upgrading from earlier versions.
83-85: StrValueHelper mixin does not exist in the codebase. The suggested refactor references a mixin that cannot be found. Additionally, the comparison pointEnumActionStatusalso implements__str__inline (returningself.value), not via a mixin. All string-based enums in the directory consistently use the same inline__str__pattern as the code under review. The current implementation is consistent with established codebase conventions.Likely an incorrect or invalid review comment.
tests/unit/enums/test_enum_circuit_breaker_state.py (1)
64-79: LGTM!The updated test correctly verifies the StrEnum behavior where
str()returns the value directly (e.g., "open", "closed", "half_open") for JSON serialization compatibility. The docstring withNOTE(OMN-1309)clearly documents the intentional behavior change, and testing all three enum members provides comprehensive coverage.src/omnibase_core/enums/enum_node_lifecycle_status.py (1)
64-85: LGTM!The helper methods
is_terminal(),is_active(), andis_error()are well-designed and follow the same pattern asEnumActionStatus. The state categorizations are logically correct:
- Terminal states correctly include all states where no further transitions occur.
- Active states correctly identify processing phases.
- Error states correctly identify failure conditions.
tests/unit/models/test_enum_serialization_omn_1309.py (2)
18-57: LGTM!Comprehensive serialization tests for
ModelCircuitBreakercovering:
- Lowercase string serialization for all states
- Enum-to-string input equivalence ensuring backward compatibility
- JSON roundtrip preservation with parametrized tests
The test structure effectively validates the PR's goal of maintaining byte-for-byte JSON compatibility after the enum refactoring.
60-117: LGTM!The
TestActionMetadataSerializationclass provides thorough coverage:
- Status serialization to lowercase strings
- Enum-string input equivalence (with appropriate comment about timestamp differences)
- Parametrized roundtrip tests for all valid statuses
- Lifecycle method verification (
mark_started,mark_completed,mark_failed)The
mark_failedtest withModelErrorDetailsvalidates the complete error handling flow.src/omnibase_core/models/configuration/model_circuit_breaker.py (3)
303-315: LGTM!The
_normalize_statevalidator correctly:
- Accepts both
EnumCircuitBreakerStateenum values and raw strings- Normalizes enum inputs to their string values for serialization compatibility
- Validates that string inputs match valid enum values
- Raises
ValueErrorwith a descriptive message for invalid inputsThis enables backward-compatible input handling while maintaining type safety.
380-408: LGTM!The
is_openproperty andshould_allow_requestmethod correctly useEnumCircuitBreakerState.*.valuefor all state comparisons, replacing the hardcoded strings while maintaining the same logic and behavior.
513-608: LGTM!All state transition methods (
_reset_state_unlocked,_transition_to_open_unlocked,_transition_to_half_open_unlocked,_transition_to_closed_unlocked) correctly assignEnumCircuitBreakerState.*.valueinstead of hardcoded strings. The refactoring is consistent and complete throughout the file.src/omnibase_core/infrastructure/node_core_base.py (5)
44-45: LGTM!The new imports for
EnumNodeHealthStatusandEnumNodeLifecycleStatusare correctly added, enabling the enum-based status representation throughout the file.
119-121: LGTM!The initial state correctly uses
EnumNodeLifecycleStatus.INITIALIZED.valueinstead of a hardcoded string, establishing the enum-based pattern from node construction.
201-242: LGTM!The
initialize()method correctly usesEnumNodeLifecycleStatusfor all state transitions:
INITIALIZINGat start of initializationREADYon successful completionFAILEDon exceptionThis maintains the lifecycle flow documented in the enum definition.
272-310: LGTM!The
cleanup()method correctly usesEnumNodeLifecycleStatusfor all state transitions:
CLEANING_UPat start of cleanupCLEANED_UPon successful completionCLEANUP_FAILEDon exception (with appropriatecatch-all-okcomment for the BaseException handler)
886-894: LGTM!The
_get_health_status_comprehensivemethod correctly usesEnumNodeHealthStatus.HEALTHY.valueandEnumNodeHealthStatus.DEGRADED.valuefor the overall status, replacing hardcoded strings while maintaining the same aggregation logic.src/omnibase_core/models/core/model_action_metadata.py (3)
12-14: LGTM!The imports are correctly structured using absolute imports, and
field_validatoris appropriately added to support the new status normalization.
123-142: Well-structured validator for backward compatibility.The validator correctly accepts both enum and string inputs while normalizing to strings for serialization. The approach maintains backward compatibility as intended.
Note: Since
validate_assignment=Trueis not set inmodel_config, direct assignments likeself.status = "..."in lifecycle methods bypass validation. This is acceptable here since those methods useEnumActionStatus.*.valuewhich guarantees valid strings, but worth keeping in mind for future maintenance.
181-200: LGTM!Lifecycle methods consistently use enum values for status transitions, improving type safety and maintainability.
Consider: The
EnumActionStatusenum provides acan_transition_to()method for validating state transitions. If stricter state machine semantics are desired in the future, these methods could validate transitions before applying them.
Merge: - Resolved conflict in enum_circuit_breaker_state.py, preserving: - @unique decorator from main - str inheritance from this branch for JSON serialization PR Review Fixes: - Use StrValueHelper mixin for new enums (EnumActionStatus, EnumCircuitBreakerState, EnumNodeLifecycleStatus) - Move valid_transitions to module-level constant in EnumActionStatus - Use set literal for membership testing in model_circuit_breaker.py
…e positive Renamed CheckerEnumMemberCasing to MemberCasingChecker to avoid triggering the enum naming convention validation, which flags classes containing "Enum" in their name.
Code Review: OMN-1309 Enum RefactoringOverall Assessment: ✅ Excellent Implementation This PR demonstrates excellent engineering discipline in replacing hardcoded status strings with type-safe enums while maintaining backward compatibility. The implementation follows ONEX conventions meticulously and includes comprehensive testing. ✅ Strengths1. Type Safety & Architecture (Outstanding)
2. Comprehensive Test Coverage (Excellent)
3. Documentation Quality (Very Good)
4. Convention Compliance (Perfect)
5. Implementation Details (Solid)
🔍 Minor Observations1. Inconsistent Validator Error MessagesLocation: # model_circuit_breaker.py - basic error message
raise ValueError(f"Invalid circuit breaker state: {v}")
# model_action_metadata.py - detailed error message
raise ValueError(
f"Invalid action status: {v!r}. Valid statuses: {sorted(valid_statuses)}"
)Suggestion: Consider harmonizing error messages. The Severity: Low (cosmetic consistency) 2. Potential DRY Opportunity in ValidatorsBoth validators follow the same pattern:
Consider: A shared validator utility function if this pattern repeats in future enum conversions. Not necessary for just 2 instances. Severity: Low (future optimization) 3.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @src/omnibase_core/enums/enum_action_status.py:
- Around line 10-14: The ASCII state-machine diagram is missing the CREATED ->
FAILED transition; update the diagram text to include that edge so it matches
the logic in _VALID_TRANSITIONS and the can_transition_to docstring (e.g., add a
branch from CREATED to FAILED in the diagram). Ensure the diagram in
enum_action_status.py reflects CREATED -> READY -> RUNNING -> COMPLETED plus
CREATED -> FAILED and RUNNING -> FAILED for consistency with _VALID_TRANSITIONS
and can_transition_to.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.pre-commit-config.yamlsrc/omnibase_core/enums/enum_action_status.pysrc/omnibase_core/enums/enum_circuit_breaker_state.pysrc/omnibase_core/enums/enum_node_lifecycle_status.py
🚧 Files skipped from review as they are similar to previous changes (2)
- .pre-commit-config.yaml
- src/omnibase_core/enums/enum_node_lifecycle_status.py
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Always use Poetry for Python package management and task execution. Never use pip or python directly.
Use protocol names for dependency injection (e.g., 'ProtocolEventBus'), never concrete class names.
Use PEP 604 union syntax (X | Y) instead of Union[X, Y] and use Optional syntax is forbidden in favor of X | None.
Files:
src/omnibase_core/enums/enum_circuit_breaker_state.pysrc/omnibase_core/enums/enum_action_status.py
src/omnibase_core/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/omnibase_core/**/*.py: Always use ModelOnexError with error codes instead of generic Exception.
Use the @standard_error_handling decorator to eliminate try/catch boilerplate in async operations.
Python files in src/omnibase_core/ must follow directory-specific naming prefixes enforced by checker_naming_convention.py.
Never use relative imports across package boundaries. Use absolute imports for stability and readability.
All TODO comments must reference a Linear ticket in the format # TODO(OMN-XXXX): description or use # TODO(OMN-TBD): description [NEEDS TICKET].
All type: ignore comments must include specific error codes and explanations above the line.
Docstrings must explain WHY, describe non-obvious behavior, or document edge cases. Do not write tautological docstrings that restate the method name.
Exception tuples in except clauses must be alphabetically ordered by exception class name.
Use exception comment markers (fallback-ok:, catch-all-ok:, cleanup-resilience-ok:, boundary-ok:, init-errors-ok:, tool-resilience-ok:) to document intent for broad exception handlers.
Use centralized exception constants from omnibase_core.errors.exception_groups instead of writing exception tuples inline.
Both mypy and pyright must pass in CI with 100% strict mode compliance (0 errors across all source files).
Files:
src/omnibase_core/enums/enum_circuit_breaker_state.pysrc/omnibase_core/enums/enum_action_status.py
src/omnibase_core/enums/enum_*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use StrValueHelper mixin for all string-based enums to ensure consistent str behavior returning self.value.
Files:
src/omnibase_core/enums/enum_circuit_breaker_state.pysrc/omnibase_core/enums/enum_action_status.py
🧠 Learnings (1)
📚 Learning: 2026-01-13T16:21:16.950Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T16:21:16.950Z
Learning: Applies to src/omnibase_core/enums/enum_*.py : Use StrValueHelper mixin for all string-based enums to ensure consistent __str__ behavior returning self.value.
Applied to files:
src/omnibase_core/enums/enum_circuit_breaker_state.py
🧬 Code graph analysis (1)
src/omnibase_core/enums/enum_action_status.py (1)
src/omnibase_core/enums/enum_node_lifecycle_status.py (2)
is_terminal(62-68)is_active(70-76)
⏰ 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 6/20)
- GitHub Check: Tests (Split 15/20)
- GitHub Check: Tests (Split 7/20)
- GitHub Check: Tests (Split 4/20)
- GitHub Check: Tests (Split 11/20)
- GitHub Check: Tests (Split 14/20)
- GitHub Check: Tests (Split 19/20)
- GitHub Check: Tests (Split 16/20)
- GitHub Check: Tests (Split 8/20)
- GitHub Check: Tests (Split 17/20)
- GitHub Check: Tests (Split 9/20)
- GitHub Check: Tests (Split 20/20)
- GitHub Check: Tests (Split 13/20)
- GitHub Check: Tests (Split 12/20)
- GitHub Check: Tests (Split 18/20)
- GitHub Check: Tests (Split 3/20)
- GitHub Check: Tests (Split 10/20)
- GitHub Check: Tests (Split 2/20)
- GitHub Check: Tests (Split 1/20)
- GitHub Check: Tests (Split 5/20)
- GitHub Check: claude-review
🔇 Additional comments (13)
src/omnibase_core/enums/enum_circuit_breaker_state.py (3)
8-14: LGTM! Correctly usesStrValueHelpermixin for string-based enum.The inheritance order
(StrValueHelper, str, Enum)is correct for MRO, and this aligns with the coding guideline requiringStrValueHelperfor all string-based enums inenum_*.pyfiles. Thestrinheritance enables proper JSON serialization as noted in the docstring.Based on learnings, this follows the established pattern for consistent
__str__behavior returningself.value.
15-64: LGTM! Comprehensive docstring with excellent documentation.The docstring effectively explains WHY (prevents cascading failures), documents the state machine transitions, and provides practical usage examples including exhaustive match statement handling. The version annotations properly track the serialization change introduced in this PR (OMN-1309).
66-105: LGTM! Enum values and exhaustiveness helper are appropriately implemented.The
assert_exhaustivemethod correctly usesNevertyping for compile-time exhaustiveness checking. The# error-ok:comment on line 104 appropriately documents whyAssertionErroris used instead ofModelOnexError(avoiding circular imports between enums and models).src/omnibase_core/enums/enum_action_status.py (10)
20-22: LGTM!Imports are clean and use the required
StrValueHelpermixin per coding guidelines.
25-57: LGTM!The enum class correctly uses
StrValueHelpermixin as required by coding guidelines, and the inheritance order(StrValueHelper, str, Enum)ensures proper MRO for JSON serialization. The docstring with examples is well-written.
59-75: LGTM!Implementation is correct and consistent with the pattern used in
EnumNodeLifecycleStatus.is_terminalfrom the related enum.
77-92: LGTM!The equality check is appropriate here since there's only one active state (
RUNNING), unlikeEnumNodeLifecycleStatuswhich has multiple active states.
94-109: LGTM!Correctly identifies pending states as those before execution begins.
111-124: LGTM!Simple and correct predicate for success state.
126-139: LGTM!Simple and correct predicate for failure state.
141-169: LGTM!The transition validation logic is clean with proper defensive fallback via
.get(self, set()). The docstring accurately documents all valid transitions.
172-180: LGTM!The transition map is well-structured. Explicit empty sets for terminal states (
COMPLETED,FAILED) clearly communicate that no outgoing transitions are allowed.
183-183: LGTM!Appropriate public API surface—
_VALID_TRANSITIONSis correctly kept private sincecan_transition_toprovides the public interface for transition validation.
- Fix inconsistent validator error messages in model_circuit_breaker.py - Add missing CREATED → FAILED transition to state machine diagram - Add SPDX license header to enum_node_lifecycle_status.py - Use set literal for membership testing in model_action_metadata.py
PR Review: OMN-1309 Enum RefactoringSummaryThis PR successfully replaces hardcoded status strings with type-safe enum references while maintaining byte-for-byte backward compatibility in JSON serialization. The implementation is solid and aligns well with the repository's standards. ✅ Strengths1. Excellent Type Safety Design
2. Comprehensive DocumentationThe new enums (
3. Rigorous Testing
4. Clean Implementation
🔍 Code Quality ObservationsMinor Issues1. Missing Dedicated Test Coverage for New EnumsLocation: Issue: While serialization tests exist ( Recommendation: Add test files:
Test the utility methods: # Test cases to add
def test_action_status_is_terminal():
assert EnumActionStatus.COMPLETED.is_terminal()
assert EnumActionStatus.FAILED.is_terminal()
assert not EnumActionStatus.RUNNING.is_terminal()
def test_action_status_can_transition_to():
assert EnumActionStatus.CREATED.can_transition_to(EnumActionStatus.READY)
assert not EnumActionStatus.COMPLETED.can_transition_to(EnumActionStatus.RUNNING)
def test_node_lifecycle_is_error():
assert EnumNodeLifecycleStatus.FAILED.is_error()
assert not EnumNodeLifecycleStatus.READY.is_error()Note: 2. Export ConsistencyLocation: Issue: Impact: Low - can still be imported directly, but breaks consistency. Recommendation: Add to exports: from .enum_node_lifecycle_status import EnumNodeLifecycleStatus
__all__ = [
# ... existing exports ...
"EnumNodeLifecycleStatus",
"EnumActionStatus",
]3. Pre-commit Exclusion DocumentationLocation: Observation: Question: Are there actually multiple Protocol classes in this file? From the diff, I only see imports and usage of Recommendation: Verify this exclusion is still needed. If the multiple-class issue was resolved, remove from exclusions. 🎯 Architecture & PatternsExcellent Adherence to Repository Standards✅ Pydantic Field Definition - Uses State Machine ImplementationThe 🔒 Security & PerformanceNo Security Concerns - Validation prevents invalid state transitions 📊 Test Coverage Analysis
Recommendation: Add dedicated enum test files to bring utility method coverage to 100%. 🚀 Recommendations SummaryRequired Before MergeNone - the PR is functionally complete and safe to merge. Suggested Follow-ups (Post-Merge)
📝 Final VerdictAPPROVED ✅ - This is high-quality work that:
The missing enum utility tests are a minor gap that can be addressed in a follow-up PR without blocking this merge. Great job on the state machine documentation and the rigorous backward-compatibility testing! 🎉 Reviewed by: Claude Sonnet 4.5 |
1 participant
Summary
Replace hardcoded status strings with type-safe enum references in 4 priority files, maintaining backward-compatible JSON serialization.
EnumCircuitBreakerStateto inherit fromstr, Enumfor serializationEnumNodeLifecycleStatusfor node lifecycle statesEnumActionStatusfor action metadata statesmodel_circuit_breaker.pyto useEnumCircuitBreakerStatemixin_health_check.pyto useEnumNodeHealthStatusnode_core_base.pyto useEnumNodeLifecycleStatusmodel_action_metadata.pyto useEnumActionStatusKey Implementation Details:
str, Enumfor proper JSON serializationTest plan
test_enum_serialization_omn_1309.py)Linear
Closes OMN-1309
Summary by CodeRabbit
Refactor
New Features
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.