Repository navigation
feat(fsm): add MVP FSM contract models for NodeReducer v1.0 [OMN-578,… - #165
Conversation
… OMN-579, OMN-580, OMN-581] Add four new MVP FSM contract models for the NodeReducer v1.0 contract-driven architecture. Models use 'Mvp' prefix to differentiate from existing subcontract models which have different structure (require version: ModelSemVer). New models: - ModelMvpFSMTransitionCondition (OMN-578): Guard conditions with 3-token expressions, required flag, and reserved v1.1+ fields (retry_count, timeout_ms) - ModelMvpFSMTransitionAction (OMN-579): State/transition actions with execution order, criticality, and typed action_config (ActionConfigValue) - ModelMvpFSMOperation (OMN-580): Reserved model for v1.1+ operation definitions - ModelMvpActionConfigParameter (OMN-581): Typed action config parameters with type validation (MvpParameterType) for default values Technical notes: - ActionConfigValue type alias replaces dict[str, Any] to comply with ONEX anti-pattern rules: str | int | float | bool | list[str] | None - All models follow ONEX patterns with protocol implementations (execute, serialize, validate_instance) - Comprehensive test coverage: 214 tests across 4 test files - Removed empty tests/integration/mixins directory
|
Warning Rate limit exceeded@jonahgabriel has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 13 minutes and 52 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (17)
WalkthroughThis PR significantly restructures the FSM and configuration models by relocating models between modules, converting mutable collections to immutable tuples for deep immutability, introducing stricter typing throughout the codebase, and consolidating type definitions. It includes reorganization of ModelActionConfigParameter and FSM-related models, updates to import paths, enhanced type system with discriminators and TypedDicts, and refactoring of FSM execution logic to use stronger typing. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Areas requiring extra attention:
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
Comment |
Code Review - PR #165: MVP FSM Contract Models for NodeReducer v1.0SummaryThis PR adds four new MVP FSM contract models for NodeReducer v1.0 with comprehensive test coverage (214 tests). The implementation follows ONEX patterns and architectural standards well. Overall, this is high-quality work with a few minor suggestions for improvement. ✅ Strengths
🔍 Code Quality ObservationsModelMvpActionConfigParameter (
|
| Category | Status | Notes |
|---|---|---|
| Follows ONEX Patterns | ✅ | Uses ModelOnexError, protocol implementations, proper exports |
| Type Safety | ✅ | Strict typing, no Any in public interfaces |
| Error Handling | ✅ | Structured errors with context |
| Documentation | ✅ | Clear docstrings with examples |
| Test Coverage | ✅ | 214 comprehensive tests |
| Unused Imports | 3 files have unused imports (mypy will catch) | |
| Field Validation | Missing constraints on execution_order, timeout_ms, expression |
|
| Code Consistency | Minor inconsistencies in Field default syntax |
🎯 Recommendations (Priority Order)
High Priority
None - no blocking issues
Medium Priority
- Add validation constraints for
execution_orderandtimeout_ms(prevent negative values if unintended) - Add expression format validation or document that runtime handles it
- Clarify protocol method usage - are
execute(),serialize(),validate_instance()used in v1.0?
Low Priority (CI will catch)
- Remove unused imports:
Optional,Unionin 3 files - Standardize Field syntax: Use
Field(...)instead ofField(default=...) - Fix docstring position in
model_mvp_fsm_transition_condition.py
✅ Approval Recommendation
APPROVED with minor suggestions. This is high-quality work that:
- Follows all ONEX architectural patterns
- Has excellent test coverage (214 tests)
- Uses proper error handling and type safety
- Is well-documented and maintainable
The identified issues are minor (unused imports, missing constraints) and don't block merging. They can be addressed in a follow-up PR or before merge at the author's discretion.
Great job on the type validation logic (bool/int edge case handling) and the comprehensive test suite! 🎉
Reviewed by: Claude Code (Sonnet 4.5)
Review Date: 2025-12-11
…ns [OMN-578, OMN-579, OMN-580, OMN-581] Consolidate FSM models to single canonical locations and remove duplicates: Deleted duplicates from subcontracts/: - model_fsm_transition_condition.py - model_fsm_transition_action.py - model_fsm_operation.py - model_action_config_parameter.py Canonical locations (per ticket spec): - models/fsm/model_fsm_transition_condition.py (OMN-578) - models/fsm/model_fsm_transition_action.py (OMN-579) - models/fsm/model_fsm_operation.py (OMN-580) - models/contracts/model_action_config_parameter.py (OMN-581) Fixed dict[str, Any] anti-pattern in fsm_executor.py: - Changed all context parameters from dict[str, Any] to dict[str, object] - Added type casts for Sized and SupportsFloat operations - Updated module docstring Updated imports in: - subcontracts/__init__.py - subcontracts/model_fsm_state_transition.py - subcontracts/model_fsm_subcontract.py - utils/fsm_executor.py - tests/unit/utils/test_fsm_executor.py
Resolve merge conflict by keeping model_fsm_transition_condition.py deleted (consolidated to models/fsm/ in this branch). Main branch added immutability and validation rules via PR #166 which are preserved in merged files.
Code Review: FSM Contract Models for NodeReducer v1.0Thank you for this comprehensive PR! The implementation adds MVP FSM contract models for the NodeReducer v1.0 architecture. Here's my detailed review: ✅ Strengths
|
- Add frozen=True to all FSM models for thread safety (per CLAUDE.md) - Add field validation constraints (ge=0 for retry_count/execution_order, gt=0 for timeout_ms) - Add expression format validation (3-token grammar) in ModelFSMTransitionCondition - Fix execute() methods to work with frozen models (return True, documented as v1.1+ reserved) - Implement actual validate_instance() logic with field validation - Remove unused imports (Optional, Union) from 3 files - Remove default=... anti-pattern in ModelFSMOperation required fields - Fix docstring position (before __future__ import) - Change typing_extensions.Self to typing.Self (Python 3.11+) - Improve docstrings with comprehensive parameter documentation - Update 13 tests to match new validation behavior [OMN-578, OMN-579, OMN-580, OMN-581]
Code Review: PR #165 - MVP FSM Contract Models for NodeReducer v1.0SummaryThis PR adds four new MVP FSM contract models with comprehensive test coverage (214 tests). The code quality is excellent overall, following ONEX patterns and best practices. However, there are several important issues that should be addressed. 🔴 Critical Issues1. Duplicate Test Files (Potential Conflict)Location: Tests directory
Impact: This may cause test duplication, confusion, or conflicts. The old model was deleted from subcontracts/, but the test file remains. Recommendation: Delete the old test file at tests/unit/models/contracts/subcontracts/test_model_action_config_parameter.py to avoid confusion and maintain consistency with the model reorganization.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (10)
src/omnibase_core/utils/fsm_executor.py (2)
488-489: Verify runtime behavior of cast operations for length checks.The
cast(Sized, field_value)operations provide static type hints but don't perform runtime validation. Iffield_valueis not actuallySized,len()will raiseTypeError, which is caught by theexceptclause. However, consider whether explicithasattr(field_value, '__len__')checks would be clearer.Current approach (cast with exception handling):
try: return len(cast(Sized, field_value)) >= int(expected_value or "0") except (TypeError, ValueError): return FalseAlternative approach (explicit runtime check):
if not hasattr(field_value, '__len__'): return False try: return len(field_value) >= int(expected_value or "0") except (TypeError, ValueError): return FalseBoth approaches work correctly. The current implementation is valid, but the alternative may be more explicit about the runtime check.
Also applies to: 496-497
502-505: Verify runtime behavior of cast operations for numeric comparisons.The
cast(SupportsFloat, field_value)operations provide static type hints but don't perform runtime validation. Iffield_valuedoesn't support float conversion,float()will raiseTypeError, which is caught. Consider whether explicit type checks would be clearer.Current approach relies on exception handling:
try: return float(cast(SupportsFloat, field_value) or 0) > float(expected_value or "0") except (TypeError, ValueError): return FalseAlternative approach with explicit checks:
try: field_float = float(field_value) if field_value is not None else 0.0 expected_float = float(expected_value or "0") return field_float > expected_float except (TypeError, ValueError): return FalseBoth approaches are functionally equivalent. The current implementation is valid.
Also applies to: 510-513
src/omnibase_core/models/fsm/model_fsm_transition_condition.py (1)
95-122: Redundant expression validation invalidate_instance().The expression token count is already validated by
validate_expression_formatduring model construction (lines 95-122). Since the model is frozen and expression validation happens at construction time, the token count check invalidate_instance()(lines 197-211) can never fail for a valid instance—if the expression had incorrect tokens, the model would not have been created in the first place.This redundancy is harmless but adds unnecessary code. If the intent is to allow
validate_instance()to be called on partially constructed objects in the future, consider adding a comment explaining this.Also applies to: 195-211
tests/unit/models/fsm/test_model_fsm_operation.py (1)
351-365: Consider addingvalidate_instance()assertion for empty string edge cases.Tests
test_empty_string_operation_nameandtest_empty_string_operation_typeverify that empty strings are accepted at instantiation, but they don't verify the expectedvalidate_instance()behavior. Based onModelFSMOperation.validate_instance()logic which checksstrip(), these should returnFalse.def test_empty_string_operation_name(self): """Test operation with empty string name.""" operation = ModelFSMOperation( operation_name="", operation_type="sync", ) assert operation.operation_name == "" + # Empty string should fail validate_instance check + assert operation.validate_instance() is False def test_empty_string_operation_type(self): """Test operation with empty string type.""" operation = ModelFSMOperation( operation_name="test", operation_type="", ) assert operation.operation_type == "" + # Empty string should fail validate_instance check + assert operation.validate_instance() is Falsesrc/omnibase_core/models/contracts/model_action_config_parameter.py (2)
99-102:default: Any | Nonecontradicts the "strict typing" principle stated in the docstring.Line 10 states "Strict typing is enforced: No Any types allowed in public interface," but the
defaultfield usesAny | None. While runtime validation ensures type safety, the static type signature allows arbitrary types.Consider using a union of allowed types to enforce this at the type level:
# Option 1: Explicit union (may be verbose) DefaultValue = str | int | float | bool | list[Any] | dict[str, Any] | None # Option 2: Keep Any but update docstring to clarify runtime validationThis is a minor deviation since runtime validation catches mismatches. However, for full mypy strict compliance as per coding guidelines, a narrower type would be preferable.
194-198: Consider addingfrozen=Truefor consistency with other FSM-related models.The sibling models (
ModelFSMOperation,ModelFSMTransitionAction,ModelFSMTransitionCondition) all usefrozen=Truefor immutability and thread safety. If this model is intended to be used similarly in contract definitions, consider adding it for consistency.If mutability is intentional for this model, a brief comment explaining why would be helpful.
src/omnibase_core/models/fsm/model_fsm_transition_action.py (1)
109-114: Minor redundancy:validate_assignment=Truehas no effect whenfrozen=True.Since
frozen=Trueprevents any field assignment after construction (raising aValidationError), thevalidate_assignment=Truesetting becomes redundant. This won't cause issues but adds unnecessary configuration.model_config = { "extra": "ignore", "frozen": True, "use_enum_values": False, - "validate_assignment": True, }tests/unit/models/fsm/test_model_fsm_transition_action.py (3)
430-461: Missing tests forvalidate_instance()returningFalse.The protocol tests only verify
validate_instance()returnsTruefor valid instances. There are no tests verifying it returnsFalsefor:
- Empty
action_name(constructed withaction_name="")- Empty
action_type(constructed withaction_type="")action_configcontaining a list with non-string items (viamodel_construct()to bypass Pydantic validation)Consider adding tests like:
def test_validate_instance_empty_action_name_returns_false(self): """Test validate_instance returns False for empty action_name.""" action = ModelFSMTransitionAction( action_name="", action_type="log", ) assert action.validate_instance() is False def test_validate_instance_empty_action_type_returns_false(self): """Test validate_instance returns False for empty action_type.""" action = ModelFSMTransitionAction( action_name="test", action_type="", ) assert action.validate_instance() is False def test_validate_instance_invalid_list_contents_returns_false(self): """Test validate_instance returns False for list containing non-strings.""" # Use model_construct to bypass Pydantic validation action = ModelFSMTransitionAction.model_construct( action_name="test", action_type="log", action_config={"items": [1, 2, 3]}, # list[int] not list[str] ) assert action.validate_instance() is False
560-574: Edge case tests verify construction but not validation behavior.These tests verify that empty strings are accepted during construction, which is correct behavior since Pydantic doesn't enforce non-empty strings by default. However, per the earlier comment, these should be paired with
validate_instance()calls to verify the validation method correctly identifies these as invalid instances.
23-24: Consider adding pytest markers per coding guidelines.The coding guidelines specify using pytest markers (
@pytest.mark.unit, etc.) to categorize tests. Consider adding@pytest.mark.unitto the test classes or at module level.+import pytest + +pytestmark = pytest.mark.unit + class TestModelFSMTransitionActionInstantiation: """Test cases for ModelFSMTransitionAction instantiation."""
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
src/omnibase_core/models/contracts/__init__.py(2 hunks)src/omnibase_core/models/contracts/model_action_config_parameter.py(1 hunks)src/omnibase_core/models/contracts/subcontracts/__init__.py(1 hunks)src/omnibase_core/models/contracts/subcontracts/model_action_config_parameter.py(0 hunks)src/omnibase_core/models/contracts/subcontracts/model_fsm_operation.py(0 hunks)src/omnibase_core/models/contracts/subcontracts/model_fsm_state_transition.py(1 hunks)src/omnibase_core/models/contracts/subcontracts/model_fsm_subcontract.py(1 hunks)src/omnibase_core/models/contracts/subcontracts/model_fsm_transition_action.py(0 hunks)src/omnibase_core/models/contracts/subcontracts/model_fsm_transition_condition.py(0 hunks)src/omnibase_core/models/fsm/__init__.py(1 hunks)src/omnibase_core/models/fsm/model_fsm_operation.py(1 hunks)src/omnibase_core/models/fsm/model_fsm_transition_action.py(1 hunks)src/omnibase_core/models/fsm/model_fsm_transition_condition.py(1 hunks)src/omnibase_core/utils/fsm_executor.py(8 hunks)tests/unit/models/contracts/test_model_action_config_parameter.py(1 hunks)tests/unit/models/fsm/test_model_fsm_operation.py(1 hunks)tests/unit/models/fsm/test_model_fsm_transition_action.py(1 hunks)tests/unit/models/fsm/test_model_fsm_transition_condition.py(1 hunks)tests/unit/utils/test_fsm_executor.py(1 hunks)
💤 Files with no reviewable changes (4)
- src/omnibase_core/models/contracts/subcontracts/model_fsm_transition_action.py
- src/omnibase_core/models/contracts/subcontracts/model_fsm_operation.py
- src/omnibase_core/models/contracts/subcontracts/model_fsm_transition_condition.py
- src/omnibase_core/models/contracts/subcontracts/model_action_config_parameter.py
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{py,txt}
📄 CodeRabbit inference engine (CLAUDE.md)
Always use Poetry for Python package management and task execution. Use
poetry runprefix for all commands (pytest, mypy, python, etc.) - never use pip or python directly
Files:
src/omnibase_core/models/contracts/subcontracts/model_fsm_subcontract.pysrc/omnibase_core/models/contracts/model_action_config_parameter.pytests/unit/models/fsm/test_model_fsm_operation.pysrc/omnibase_core/models/fsm/model_fsm_transition_condition.pytests/unit/models/contracts/test_model_action_config_parameter.pysrc/omnibase_core/utils/fsm_executor.pytests/unit/models/fsm/test_model_fsm_transition_action.pysrc/omnibase_core/models/contracts/__init__.pytests/unit/utils/test_fsm_executor.pysrc/omnibase_core/models/fsm/model_fsm_transition_action.pysrc/omnibase_core/models/contracts/subcontracts/model_fsm_state_transition.pytests/unit/models/fsm/test_model_fsm_transition_condition.pysrc/omnibase_core/models/fsm/__init__.pysrc/omnibase_core/models/fsm/model_fsm_operation.pysrc/omnibase_core/models/contracts/subcontracts/__init__.py
src/omnibase_core/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/omnibase_core/**/*.py: Use protocol names for dependency injection, never concrete class names. Example:container.get_service('ProtocolEventBus')not concrete class names
Always use ModelOnexError for error handling, never generic Exception. Include error_code from EnumCoreErrorCode and context information
Use @standard_error_handling decorator to eliminate 6+ lines of try/catch boilerplate in async operations
Enforce 100% strict mypy compliance - all functions must have type annotations withdisallow_untyped_defs = true
Use EnumNodeKind for architectural classification (EFFECT, COMPUTE, REDUCER, ORCHESTRATOR, RUNTIME_HOST) in workflow routing
Use EnumNodeType for specific implementation type discovery and capability matching (TRANSFORMER, AGGREGATOR, COMPUTE_GENERIC, etc.)
Format code using Black and sort imports using isort - runpoetry run black src/ tests/andpoetry run isort src/ tests/before commit
Run all pre-commit hooks before pushing:pre-commit run --all-files- includes mypy strict, black, isort, ruff
Use duck typing with protocols - never use isinstance checks. Protocols use structural subtyping, not explicit inheritance
Process inter-service communication using ModelEventEnvelope pattern for event-driven architecture
Use thread-local storage or separate node instances for concurrent thread access - ModelComputeCache, ModelCircuitBreaker, NodeCompute/NodeEffect are NOT thread-safe
Files:
src/omnibase_core/models/contracts/subcontracts/model_fsm_subcontract.pysrc/omnibase_core/models/contracts/model_action_config_parameter.pysrc/omnibase_core/models/fsm/model_fsm_transition_condition.pysrc/omnibase_core/utils/fsm_executor.pysrc/omnibase_core/models/contracts/__init__.pysrc/omnibase_core/models/fsm/model_fsm_transition_action.pysrc/omnibase_core/models/contracts/subcontracts/model_fsm_state_transition.pysrc/omnibase_core/models/fsm/__init__.pysrc/omnibase_core/models/fsm/model_fsm_operation.pysrc/omnibase_core/models/contracts/subcontracts/__init__.py
src/omnibase_core/models/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use Pydantic models for all data structures with v2.11+ compatibility, never use plain dataclasses or Python dicts for domain models
Files:
src/omnibase_core/models/contracts/subcontracts/model_fsm_subcontract.pysrc/omnibase_core/models/contracts/model_action_config_parameter.pysrc/omnibase_core/models/fsm/model_fsm_transition_condition.pysrc/omnibase_core/models/contracts/__init__.pysrc/omnibase_core/models/fsm/model_fsm_transition_action.pysrc/omnibase_core/models/contracts/subcontracts/model_fsm_state_transition.pysrc/omnibase_core/models/fsm/__init__.pysrc/omnibase_core/models/fsm/model_fsm_operation.pysrc/omnibase_core/models/contracts/subcontracts/__init__.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Maintain minimum 60% test coverage requirement - runpoetry run pytest tests/ --cov=src/omnibase_core --cov-report=term-missing
Run pytest with parallel workers for faster local testing:poetry run pytest tests/ -n 4(default: 4 workers, can disable with-n 0for debugging)
Use pytest markers to categorize tests: @pytest.mark.unit, @pytest.mark.integration, @pytest.mark.slow, @pytest.mark.smoke, @pytest.mark.performance
CI runs 20 parallel test splits with timeout protection (60s per test) - expected per-split runtime is 2m30s to 3m30s, critical alert if > 4m30s
Files:
tests/unit/models/fsm/test_model_fsm_operation.pytests/unit/models/contracts/test_model_action_config_parameter.pytests/unit/models/fsm/test_model_fsm_transition_action.pytests/unit/utils/test_fsm_executor.pytests/unit/models/fsm/test_model_fsm_transition_condition.py
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-11T21:05:41.997Z
Learning: Applies to src/omnibase_core/nodes/node_reducer.py : Implement REDUCER nodes using NodeReducer base class with FSM-driven state transitions using ModelIntent pattern
📚 Learning: 2025-12-11T21:05:41.997Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-11T21:05:41.997Z
Learning: Applies to src/omnibase_core/**/*.py : Always use ModelOnexError for error handling, never generic Exception. Include error_code from EnumCoreErrorCode and context information
Applied to files:
src/omnibase_core/models/contracts/subcontracts/model_fsm_subcontract.py
🧬 Code graph analysis (13)
src/omnibase_core/models/contracts/subcontracts/model_fsm_subcontract.py (1)
src/omnibase_core/models/fsm/model_fsm_operation.py (1)
ModelFSMOperation(21-143)
src/omnibase_core/models/contracts/model_action_config_parameter.py (3)
src/omnibase_core/enums/enum_core_error_code.py (1)
EnumCoreErrorCode(9-163)src/omnibase_core/models/common/model_error_context.py (2)
ModelErrorContext(20-149)with_context(60-84)src/omnibase_core/models/common/model_schema_value.py (1)
ModelSchemaValue(22-347)
tests/unit/models/fsm/test_model_fsm_operation.py (1)
src/omnibase_core/models/fsm/model_fsm_operation.py (4)
ModelFSMOperation(21-143)execute(82-108)serialize(110-116)validate_instance(118-143)
src/omnibase_core/models/fsm/model_fsm_transition_condition.py (4)
src/omnibase_core/enums/enum_core_error_code.py (1)
EnumCoreErrorCode(9-163)src/omnibase_core/protocols/event_bus/protocol_event_bus_headers.py (1)
retry_count(105-107)src/omnibase_core/models/fsm/model_fsm_operation.py (3)
execute(82-108)serialize(110-116)validate_instance(118-143)src/omnibase_core/models/fsm/model_fsm_transition_action.py (3)
execute(118-140)serialize(142-148)validate_instance(150-180)
src/omnibase_core/utils/fsm_executor.py (1)
src/omnibase_core/models/fsm/model_fsm_transition_condition.py (1)
ModelFSMTransitionCondition(21-213)
tests/unit/models/fsm/test_model_fsm_transition_action.py (1)
src/omnibase_core/models/fsm/model_fsm_transition_action.py (4)
ModelFSMTransitionAction(31-180)execute(118-140)serialize(142-148)validate_instance(150-180)
src/omnibase_core/models/contracts/__init__.py (1)
src/omnibase_core/models/contracts/model_action_config_parameter.py (1)
ModelActionConfigParameter(42-198)
tests/unit/utils/test_fsm_executor.py (1)
src/omnibase_core/models/fsm/model_fsm_transition_condition.py (1)
ModelFSMTransitionCondition(21-213)
src/omnibase_core/models/fsm/model_fsm_transition_action.py (3)
src/omnibase_core/models/fsm/model_fsm_operation.py (3)
execute(82-108)serialize(110-116)validate_instance(118-143)src/omnibase_core/models/fsm/model_fsm_transition_condition.py (3)
execute(126-142)serialize(144-150)validate_instance(152-213)src/omnibase_core/protocols/event_bus/protocol_event_message.py (1)
value(33-35)
tests/unit/models/fsm/test_model_fsm_transition_condition.py (1)
src/omnibase_core/models/fsm/model_fsm_transition_condition.py (4)
ModelFSMTransitionCondition(21-213)execute(126-142)serialize(144-150)validate_instance(152-213)
src/omnibase_core/models/fsm/__init__.py (5)
src/omnibase_core/models/fsm/model_fsm_operation.py (1)
ModelFSMOperation(21-143)src/omnibase_core/models/fsm/model_fsm_state_snapshot.py (1)
ModelFSMStateSnapshot(13-26)src/omnibase_core/models/fsm/model_fsm_transition_action.py (1)
ModelFSMTransitionAction(31-180)src/omnibase_core/models/fsm/model_fsm_transition_condition.py (1)
ModelFSMTransitionCondition(21-213)src/omnibase_core/models/fsm/model_fsm_transition_result.py (1)
ModelFSMTransitionResult(9-45)
src/omnibase_core/models/fsm/model_fsm_operation.py (2)
src/omnibase_core/models/fsm/model_fsm_transition_action.py (3)
execute(118-140)serialize(142-148)validate_instance(150-180)src/omnibase_core/models/fsm/model_fsm_transition_condition.py (3)
execute(126-142)serialize(144-150)validate_instance(152-213)
src/omnibase_core/models/contracts/subcontracts/__init__.py (3)
src/omnibase_core/models/fsm/model_fsm_operation.py (1)
ModelFSMOperation(21-143)src/omnibase_core/models/fsm/model_fsm_transition_action.py (1)
ModelFSMTransitionAction(31-180)src/omnibase_core/models/fsm/model_fsm_transition_condition.py (1)
ModelFSMTransitionCondition(21-213)
⏰ 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: claude-review
- GitHub Check: Smoke Tests
🔇 Additional comments (27)
src/omnibase_core/models/contracts/subcontracts/model_fsm_state_transition.py (1)
20-25: LGTM! Import path update aligns with FSM model relocation.The absolute import paths for
ModelFSMTransitionActionandModelFSMTransitionConditionfrom theomnibase_core.models.fsmmodule correctly reflect the FSM model reorganization described in the PR objectives.src/omnibase_core/utils/fsm_executor.py (2)
7-9: LGTM! Enhanced typing documentation.The docstring update clarifies the stronger typing approach using
dict[str, object]for runtime context flexibility.
12-14: LGTM! Import additions support stronger typing.The new imports (
Sized,SupportsFloat,cast) and the updated import path forModelFSMTransitionConditioncorrectly support the typing improvements throughout the file.Also applies to: 30-32
src/omnibase_core/models/contracts/subcontracts/model_fsm_subcontract.py (1)
32-32: LGTM! Import path update aligns with FSM model relocation.The absolute import path for
ModelFSMOperationfromomnibase_core.models.fsmcorrectly reflects the FSM model reorganization.src/omnibase_core/models/contracts/__init__.py (1)
72-75: LGTM! Public API extended with action configuration types.The addition of
ModelActionConfigParameterandParameterTypeto the contracts module's public API correctly exposes the new strongly-typed action configuration model introduced in this PR.Also applies to: 157-158
tests/unit/utils/test_fsm_executor.py (1)
22-24: LGTM! Test import updated to reflect model relocation.The import path for
ModelFSMTransitionConditioncorrectly references the new location in theomnibase_core.models.fsmmodule.tests/unit/models/contracts/test_model_action_config_parameter.py (1)
1-817: LGTM! Comprehensive test coverage for ModelActionConfigParameter.The test suite provides excellent coverage across all aspects of the model:
- ✅ Minimal and full instantiation tests
- ✅ All parameter types (string, int, bool, float, list, dict)
- ✅ Type validation with edge cases (bool/int precedence)
- ✅ Required vs optional semantics
- ✅ Serialization/deserialization round-trips
- ✅ Edge cases (empty strings, Unicode, extreme values, nested structures)
- ✅ Model configuration behavior (frozen, extra fields)
- ✅ Public API import tests
- ✅ Error message quality validation
The test organization is clear with well-documented sections and descriptive test names.
src/omnibase_core/models/fsm/model_fsm_operation.py (2)
82-108: LGTM! Placeholder execute() method is well-documented.The placeholder implementation correctly documents that:
- The model is frozen (immutable)
- Actual execution logic is reserved for v1.1+
- Future implementation will use external state management
110-116: LGTM! Protocol method implementations are correct.Both
serialize()andvalidate_instance()methods follow the established patterns from other FSM models in the codebase:
serialize()usesmodel_dump(exclude_none=False, by_alias=True)validate_instance()checks for non-empty strings after stripping whitespaceAlso applies to: 118-143
tests/unit/models/fsm/test_model_fsm_transition_condition.py (1)
1-792: LGTM! Comprehensive test coverage for ModelFSMTransitionCondition.The test suite provides thorough coverage:
- ✅ Minimal and full instantiation
- ✅ Default values (required=True, optional fields=None)
- ✅ Field type validation (all fields)
- ✅ Protocol implementations (execute, serialize, validate_instance)
- ✅ Serialization/deserialization with JSON round-trips
- ✅ Edge cases (empty strings, special characters, operators, large values)
- ✅ Model equality and frozen behavior
- ✅ Expression validation (3-token requirement)
- ✅ Import verification from fsm module
The test organization follows the established pattern with clear class groupings and descriptive test names.
src/omnibase_core/models/fsm/model_fsm_transition_condition.py (2)
126-142: LGTM - execute() placeholder is consistent with sibling models.The frozen model design and v1.0 placeholder behavior (always returns True) aligns with
ModelFSMOperationandModelFSMTransitionAction. The docstring clearly documents the reserved v1.1+ behavior.
51-93: LGTM - Field definitions and model configuration are well-structured.Required fields use
default=..., optional/reserved fields have sensible defaults withge=0constraints where appropriate. The frozen=True config ensures thread safety as per coding guidelines.tests/unit/models/fsm/test_model_fsm_operation.py (3)
22-59: LGTM - Comprehensive instantiation tests.Tests cover minimal required fields, full field population, and empty description edge case. Assertions are clear and verify expected field values.
158-250: LGTM - Protocol method tests are thorough.Tests verify
execute()returns True regardless of kwargs (frozen model),serialize()produces correct dict output, andvalidate_instance()returns True for valid instances. The frozen behavior test at lines 183-187 correctly verifies that mutations don't occur.
331-346: LGTM - Roundtrip serialization test ensures data integrity.The test verifies JSON serialization/deserialization produces equivalent model instances, which is essential for FSM contract persistence.
src/omnibase_core/models/fsm/__init__.py (1)
8-23: LGTM - Public API surface properly expanded.New FSM models and the
ActionConfigValuetype alias are correctly imported and exported via__all__. The alphabetical ordering in__all__aids discoverability.src/omnibase_core/models/contracts/model_action_config_parameter.py (2)
144-167: LGTM - Special case for bool vs int is correctly handled.In Python,
boolis a subclass ofint, soisinstance(True, int)returnsTrue. The explicit check before the generalisinstancecall correctly prevents booleans from being accepted as int defaults. Well-documented with clear error context.
32-39: LGTM - Type mapping is correct and well-structured.The mapping correctly handles Python's numeric coercion (int acceptable for float) and covers all
ParameterTypevalues. Using a module-level constant with underscore prefix correctly signals internal use.src/omnibase_core/models/contracts/subcontracts/__init__.py (2)
28-34: LGTM - FSM models correctly re-exported from subcontracts.The imports from
omnibase_core.models.fsmand their inclusion in__all__(lines 195-199) provide a convenient import path for users who expect FSM-related models in the contracts submodule. This maintains backward compatibility while the canonical location is now thefsmmodule.
193-199: Dual export paths are intentional and properly documented in architecture.The re-exports in
omnibase_core.models.contracts.subcontractsare intentional. TheSUBCONTRACT_ARCHITECTURE.mddocumentation shows thatomnibase_core.models.contracts.subcontractsis the canonical import path when working with FSM models within the contract-driven architecture pattern. The FSM models are defined in thefsmmodule and re-exported here for convenience when composing node contracts. This pattern is consistent with how all other subcontract models are imported and used across the codebase. No additional documentation is needed.src/omnibase_core/models/fsm/model_fsm_transition_action.py (3)
1-28: LGTM - Clean module setup and type alias definition.The module docstring clearly documents the purpose, version notes, and reserved fields. The
ActionConfigValuetype alias properly restricts configuration values to primitives andlist[str], aligning with ONEX anti-pattern rules mentioned in the PR objectives.
118-140: LGTM - Execute method follows established FSM model patterns.The placeholder implementation with clear v1.1+ documentation is consistent with
ModelFSMOperation.execute(). The explicit_ = kwargsidiom clearly marks the parameter as intentionally unused.
150-180: LGTM - Validation logic is sound with appropriate runtime checks.The
validate_instancemethod correctly:
- Validates non-empty strings using both truthiness and
.strip()checks- Performs runtime validation for list contents since
list[str]cannot be fully validated at compile time- Returns
Falseconsistently on validation failure, matchingModelFSMOperation.validate_instance()patterntests/unit/models/fsm/test_model_fsm_transition_action.py (4)
1-21: LGTM - Well-documented test module with appropriate imports.Good specification reference and comprehensive list of tested aspects in the module docstring.
23-176: LGTM - Thorough instantiation tests.Good coverage of minimal/full instantiation, default values, and ActionConfigValue type testing with appropriate notes about ONEX typing standards.
178-326: LGTM - Comprehensive validation tests.Good coverage of required field validation, type constraints, and error scenarios for all fields.
837-913: LGTM - Good coverage for reserved fields, imports, and documentation.Reserved field tests appropriately verify that fields can be set but have no v1.0 effect. Import tests verify both package and direct module imports. Docstring tests ensure documentation is present.
…st file - Fix import sorting in test_model_action_config_parameter.py - Fix import sorting in test_fsm_analysis.py - Rename test_model_fsmtransitionaction.py to test_model_fsm_transition_action.py to follow ONEX naming conventions Root cause: Imports were interleaved with code statements (DEFAULT_VERSION) and comments that broke import blocks, causing CI ruff check to fail while pre-commit's --fix auto-corrected locally.
Code Review: FSM Contract Models for NodeReducer v1.0SummaryThis PR introduces MVP FSM contract models for the NodeReducer v1.0 contract-driven architecture. The changes move models from ✅ Strengths
🔍 Issues & Recommendations1. CRITICAL: Import Path Breaking ChangeIssue: Moving models from # OLD (pre-v0.4.0)
from omnibase_core.models.contracts.subcontracts.model_fsm_transition_action import ModelFSMTransitionAction
# NEW (v0.4.0)
from omnibase_core.models.fsm.model_fsm_transition_action import ModelFSMTransitionActionImpact:
Recommendation: 2. ModelActionConfigParameter: Type Validation Edge CaseLocation: Issue: Bool/int edge case handling is good, but there's a subtle issue: # Line 142-164: Special case for bool/int
if self.type == "int" and isinstance(self.default, bool):
raise ModelOnexError(...) # Good!
# Line 166-187: General type check
if not isinstance(self.default, expected_types):
raise ModelOnexError(...)Problem: Float accepts both Test Case: # This should FAIL but might PASS
param = ModelActionConfigParameter(
name="threshold",
type="float",
default=True, # bool is int, int is accepted for float
)Recommendation: # After line 164, add:
if self.type == "float" and isinstance(self.default, bool):
msg = f"Default value for parameter '{self.name}' has type 'bool', expected type '{self.type}'"
raise ModelOnexError(...)3. ModelFSMTransitionCondition: 3-Token Expression ValidationLocation: Issue: The 3-token validation is too strict for complex expressions. # Line 108: Simple split on whitespace
tokens = self.expression.split()
if len(tokens) != 3:
raise ModelOnexError(...)Problem: This breaks valid expressions with spaces in values: # FAILS but should be valid
expression = 'message equals "hello world"' # 4 tokens after split()
# FAILS but should be valid
expression = 'path equals /usr/local/bin' # 3 tokens but path has /Impact:
Recommendation: # Option 1: Document limitation
"""
Expression format: "field operator value" (no spaces in values)
Examples:
✅ "status == active"
✅ "count > 0"
❌ "message == hello world" (use underscores: hello_world)
"""
# Option 2: Add quote handling (v1.1+)
# Reserved for future enhancement4. fsm_executor.py: Type Coercion DocumentationLocation: Observation: The string coercion behavior for # Line 470-476: STRING-BASED COMPARISON
return str(field_value) == str(expected_value)Why this is good:
Recommendation: 5. Unused Import in fsm_executor.pyLocation: Issue: from omnibase_core.models.fsm.model_fsm_transition_condition import (
ModelFSMTransitionCondition,
)Impact:
Recommendation: from typing import TYPE_CHECKING
if TYPE_CHECKING:
from omnibase_core.models.fsm.model_fsm_transition_condition import (
ModelFSMTransitionCondition,
)6. Test File Naming InconsistencyLocation: Test files Issue: Some test files updated, some moved:
Question: Should the old subcontract test file be deleted or is there a reason to keep both? 🔒 Security Considerations✅ No security concerns identified
⚡ Performance Considerations✅ Performance looks good
Minor optimization opportunity: 📊 Test Coverage AssessmentTest Statistics: 214 tests added across 4 test files Coverage Analysis:
Missing coverage (nice-to-have):
Verdict: Coverage is excellent for unit tests. Missing integration tests are acceptable for v1.0. 📝 Documentation ReviewStrengths:
Suggestions:
🎯 Final VerdictApproval Status: ✅ APPROVE with minor suggestions Blocking Issues: None Non-Blocking Suggestions:
Confidence Level: High - This is well-architected, thoroughly tested, and follows project conventions. 💡 Next Steps
Great work! This PR demonstrates strong understanding of ONEX patterns, excellent testing discipline, and thoughtful API design. The reserved fields pattern shows good forward-thinking for v1.1+. Reviewed by: Claude Code (Sonnet 4.5) |
- Add min_length=1 constraint to ModelFSMOperation fields - Remove redundant validate_assignment=True (no effect with frozen=True) - Add comprehensive validate_instance() False return tests - Add @pytest.mark.unit markers to all FSM test classes - Add tests for whitespace, tabs, newlines edge cases Addresses: validate_instance() test coverage, field constraints, model config cleanup, pytest markers per coding guidelines
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/omnibase_core/models/fsm/model_fsm_operation.py (1)
21-155: Model implementation looks solid; consider aligning reserved-field docs with actual fieldsThe model configuration, field constraints, and protocol methods (
execute,serialize,validate_instance) are coherent and match the intended “reserved for v1.1+” semantics. One small consistency nit: the docstring documentsretry_count,timeout_ms, androllback_actionas “Reserved Fields (v1.1+)”, but these are not currently defined as model attributes, unlike howretry_count/timeout_msare modeled inModelFSMTransitionCondition.If you intend them to be parsed-but-ignored in v1.0 (for forward compatibility), consider adding them as optional fields (mirroring the condition model), or otherwise tweak the docstring to make clear they are spec-level reserved fields, not yet present on the model.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/omnibase_core/models/fsm/model_fsm_operation.py(1 hunks)src/omnibase_core/models/fsm/model_fsm_transition_action.py(1 hunks)src/omnibase_core/models/fsm/model_fsm_transition_condition.py(1 hunks)tests/unit/models/contracts/subcontracts/test_model_action_config_parameter.py(2 hunks)tests/unit/models/contracts/subcontracts/test_model_fsm_transition_action.py(1 hunks)tests/unit/models/fsm/test_model_fsm_operation.py(1 hunks)tests/unit/models/fsm/test_model_fsm_transition_action.py(1 hunks)tests/unit/models/fsm/test_model_fsm_transition_condition.py(1 hunks)tests/unit/validation/test_fsm_analysis.py(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- tests/unit/models/contracts/subcontracts/test_model_action_config_parameter.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/unit/models/fsm/test_model_fsm_operation.py
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{py,txt}
📄 CodeRabbit inference engine (CLAUDE.md)
Always use Poetry for Python package management and task execution. Use
poetry runprefix for all commands (pytest, mypy, python, etc.) - never use pip or python directly
Files:
src/omnibase_core/models/fsm/model_fsm_operation.pysrc/omnibase_core/models/fsm/model_fsm_transition_condition.pytests/unit/models/fsm/test_model_fsm_transition_condition.pysrc/omnibase_core/models/fsm/model_fsm_transition_action.pytests/unit/models/fsm/test_model_fsm_transition_action.pytests/unit/validation/test_fsm_analysis.pytests/unit/models/contracts/subcontracts/test_model_fsm_transition_action.py
src/omnibase_core/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/omnibase_core/**/*.py: Use protocol names for dependency injection, never concrete class names. Example:container.get_service('ProtocolEventBus')not concrete class names
Always use ModelOnexError for error handling, never generic Exception. Include error_code from EnumCoreErrorCode and context information
Use @standard_error_handling decorator to eliminate 6+ lines of try/catch boilerplate in async operations
Enforce 100% strict mypy compliance - all functions must have type annotations withdisallow_untyped_defs = true
Use EnumNodeKind for architectural classification (EFFECT, COMPUTE, REDUCER, ORCHESTRATOR, RUNTIME_HOST) in workflow routing
Use EnumNodeType for specific implementation type discovery and capability matching (TRANSFORMER, AGGREGATOR, COMPUTE_GENERIC, etc.)
Format code using Black and sort imports using isort - runpoetry run black src/ tests/andpoetry run isort src/ tests/before commit
Run all pre-commit hooks before pushing:pre-commit run --all-files- includes mypy strict, black, isort, ruff
Use duck typing with protocols - never use isinstance checks. Protocols use structural subtyping, not explicit inheritance
Process inter-service communication using ModelEventEnvelope pattern for event-driven architecture
Use thread-local storage or separate node instances for concurrent thread access - ModelComputeCache, ModelCircuitBreaker, NodeCompute/NodeEffect are NOT thread-safe
Files:
src/omnibase_core/models/fsm/model_fsm_operation.pysrc/omnibase_core/models/fsm/model_fsm_transition_condition.pysrc/omnibase_core/models/fsm/model_fsm_transition_action.py
src/omnibase_core/models/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use Pydantic models for all data structures with v2.11+ compatibility, never use plain dataclasses or Python dicts for domain models
Files:
src/omnibase_core/models/fsm/model_fsm_operation.pysrc/omnibase_core/models/fsm/model_fsm_transition_condition.pysrc/omnibase_core/models/fsm/model_fsm_transition_action.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Maintain minimum 60% test coverage requirement - runpoetry run pytest tests/ --cov=src/omnibase_core --cov-report=term-missing
Run pytest with parallel workers for faster local testing:poetry run pytest tests/ -n 4(default: 4 workers, can disable with-n 0for debugging)
Use pytest markers to categorize tests: @pytest.mark.unit, @pytest.mark.integration, @pytest.mark.slow, @pytest.mark.smoke, @pytest.mark.performance
CI runs 20 parallel test splits with timeout protection (60s per test) - expected per-split runtime is 2m30s to 3m30s, critical alert if > 4m30s
Files:
tests/unit/models/fsm/test_model_fsm_transition_condition.pytests/unit/models/fsm/test_model_fsm_transition_action.pytests/unit/validation/test_fsm_analysis.pytests/unit/models/contracts/subcontracts/test_model_fsm_transition_action.py
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-11T21:05:41.997Z
Learning: Applies to src/omnibase_core/nodes/node_reducer.py : Implement REDUCER nodes using NodeReducer base class with FSM-driven state transitions using ModelIntent pattern
🧬 Code graph analysis (5)
src/omnibase_core/models/fsm/model_fsm_operation.py (3)
src/omnibase_core/protocols/event_bus/protocol_event_bus_headers.py (1)
operation_name(85-87)src/omnibase_core/models/fsm/model_fsm_transition_action.py (3)
execute(117-139)serialize(141-147)validate_instance(149-179)src/omnibase_core/models/fsm/model_fsm_transition_condition.py (3)
execute(125-141)serialize(143-149)validate_instance(151-212)
src/omnibase_core/models/fsm/model_fsm_transition_condition.py (3)
src/omnibase_core/enums/enum_core_error_code.py (1)
EnumCoreErrorCode(9-163)src/omnibase_core/models/fsm/model_fsm_operation.py (3)
execute(83-109)serialize(111-117)validate_instance(119-151)src/omnibase_core/models/fsm/model_fsm_transition_action.py (3)
execute(117-139)serialize(141-147)validate_instance(149-179)
src/omnibase_core/models/fsm/model_fsm_transition_action.py (2)
src/omnibase_core/models/fsm/model_fsm_operation.py (3)
execute(83-109)serialize(111-117)validate_instance(119-151)src/omnibase_core/models/fsm/model_fsm_transition_condition.py (3)
execute(125-141)serialize(143-149)validate_instance(151-212)
tests/unit/models/fsm/test_model_fsm_transition_action.py (1)
src/omnibase_core/models/fsm/model_fsm_operation.py (3)
validate_instance(119-151)execute(83-109)serialize(111-117)
tests/unit/models/contracts/subcontracts/test_model_fsm_transition_action.py (1)
src/omnibase_core/models/primitives/model_semver.py (1)
ModelSemVer(24-143)
🔇 Additional comments (6)
tests/unit/validation/test_fsm_analysis.py (1)
16-24: Whitespace-only import formatting change is fineThe added blank line between imports has no behavioral impact; the test suite remains consistent with the FSM models and analysis behavior.
src/omnibase_core/models/fsm/model_fsm_transition_condition.py (1)
21-216: Transition condition model and validation logic look correctField definitions, the
model_validatorenforcing 3-token expressions, and protocol methods (execute,serialize,validate_instance) are consistent and match the intended semantics (including reserved retry/timeout fields andrequired‑aware error behavior). The model_config (extra=ignore, frozen=True) and use ofModelOnexErrorwithEnumCoreErrorCode.VALIDATION_ERRORare in line with the broader ONEX patterns.tests/unit/models/contracts/subcontracts/test_model_fsm_transition_action.py (1)
8-23: Version wiring and ValidationError checks in subcontract tests look goodUsing a shared
DEFAULT_VERSIONand assertingValidationErrorfor missing/invalid fields keeps these subcontract tests aligned with stricter model requirements, without changing the logical coverage of duplicate-parameter behavior.tests/unit/models/fsm/test_model_fsm_transition_condition.py (1)
1-1072: Thorough and well-aligned tests forModelFSMTransitionConditionThis suite exercises construction, Pydantic validation,
validate_instancesemantics (includingrequiredbehavior and token-count logic), protocol methods, serialization roundtrips, frozen/extra handling, and import paths. It closely matches the model’s implementation and should provide strong regression coverage.src/omnibase_core/models/fsm/model_fsm_transition_action.py (1)
31-183: FSM transition action model matches the intended v1.0 contract and typing constraintsThe
ActionConfigValuealias, field definitions (includingexecution_order/timeout_msconstraints), frozen model_config, and protocol methods (execute,serialize,validate_instance) all line up with the documented behavior and the ONEX rule of keeping action_config flat with restricted value types. The additional runtime check forlist[str]contents is a good complement to the static type annotation.tests/unit/models/fsm/test_model_fsm_transition_action.py (1)
1-1039: Comprehensive tests forModelFSMTransitionActionaligned with the new modelThese tests thoroughly cover construction, Pydantic validation,
validate_instancebehavior (including invalid list contents), protocol methods, serialization roundtrips, edge cases, reserved fields, and import/docstring expectations. They closely track the FSM action model’s implementation and should catch regressions effectively.
Code Review: FSM Contract Models for NodeReducer v1.0SummaryThis PR introduces 4 new MVP FSM contract models with 214 comprehensive tests. High-quality work with strong type safety and proper error handling. ✅ Strengths
🔴 Critical Issues1. Missing min_length=1 constraintsLocations: ModelFSMTransitionCondition, ModelFSMTransitionAction, ModelFSMOperation Required string fields (condition_name, action_name, etc.) are missing min_length=1. Without this, empty strings pass Pydantic validation but fail in validate_instance(), creating inconsistent behavior. Fix: Add min_length=1 to all required string fields. 2. Inconsistent validation pathsModels have both @model_validator and validate_instance() checking the same things. This creates redundant code. Fix: Simplify validate_instance() after adding min_length=1 constraints. 🟡 Medium Priority3. Optional parameters without defaultsModelActionConfigParameter allows required=False with default=None, which is semantically ambiguous. Recommendation: Add validator to enforce that required=False implies default is not None. 4. Type coercion surprisesfsm_executor.py string-coerces comparisons (10 == "10" → True). While documented, consider adding equals_strict operator for type-aware comparison. 📊 Test CoverageExcellent! 214 tests covering:
🎯 VerdictApprove with minor changes Fix the min_length=1 constraints and optional parameter validation, then this is ready to merge. Reviewed by: Claude Code (Sonnet 4.5) |
…idation MAJOR fixes: - Add frozen=True to ModelFsmState, ModelFsmTransition, ModelFsmData for consistency - Add validation logic to validate_instance() methods (non-empty, non-whitespace checks) - Add 18 new tests for validate_instance() returning False - Update execute() methods to be no-op (models are now immutable) - Remove unused imports (ModelOnexError, EnumCoreErrorCode) from model_fsm_data_primary MINOR/NITPICK fixes: - Update ModelFSMOperation docstring: "Reserved Fields" -> "Planned Fields (v1.1+)" - Clarify that planned fields are NOT YET IMPLEMENTED - Enhance ModelFSMStateSnapshot docstrings explaining dict[str, Any] design decision - Update tests for frozen model behavior (no mutation on execute/assignment) - Add FSMContextType type alias to resolve dict[str, Any] validation Files modified: - src/omnibase_core/models/fsm/model_fsm_state.py - src/omnibase_core/models/fsm/model_fsm_transition.py - src/omnibase_core/models/fsm/model_fsm_data_primary.py - src/omnibase_core/models/fsm/model_fsm_operation.py - src/omnibase_core/models/fsm/model_fsm_state_snapshot.py - src/omnibase_core/types/type_fsm_context.py (new) - tests/unit/models/fsm/test_model_fsm_state.py - tests/unit/models/fsm/test_model_fsm_transition.py - tests/unit/models/fsm/test_model_fsm_data.py All 309 FSM tests pass. mypy and ruff checks pass.
PR Review: FSM Contract Models for NodeReducer v1.0SummaryThis PR adds four new MVP FSM contract models as part of the NodeReducer v1.0 contract-driven architecture. The implementation follows ONEX patterns well and includes comprehensive test coverage (214 tests). Overall, this is high-quality work with strong adherence to project conventions. ✅ Strengths
|
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/omnibase_core/models/fsm/model_fsm_state_snapshot.py (1)
1-11: Docstrings still referencedict[str, Any](and “immutable” is only shallow).
Now thatcontext: FSMContextTypeis the public contract, the module/class docstrings should stop claimingdict[str, Any]. Also, “frozen” doesn’t prevent callers from mutating the underlying context/history containers—either tighten the types (e.g.,Mapping/tuple) or clarify that immutability is shallow.@@ Note: - The context field uses dict[str, Any] to allow flexible FSM context data. + The context field uses FSMContextType to allow flexible FSM context data. @@ context: Runtime context data for the FSM. Uses dict[str, Any] to support flexible context structures that vary per FSM implementation. + (Note: container immutability is shallow; callers must not mutate.)Also applies to: 20-36, 49-50
🧹 Nitpick comments (3)
src/omnibase_core/models/fsm/model_fsm_transition.py (1)
25-27: Fail fast on empty strings via Pydantic constraints (keepvalidate_instance()for whitespace-only).- from_state: str = Field(default=..., description="Source state of transition") - to_state: str = Field(default=..., description="Target state of transition") - trigger: str = Field(default=..., description="Event that triggers the transition") + from_state: str = Field(..., min_length=1, description="Source state of transition") + to_state: str = Field(..., min_length=1, description="Target state of transition") + trigger: str = Field(..., min_length=1, description="Event that triggers the transition")Also applies to: 57-77
src/omnibase_core/models/fsm/model_fsm_data_primary.py (1)
109-125: Consider wiringvalidate_instance()tovalidate_fsm_structure()(if protocol expects structural validity).If
validate_instance()is the “single call” used by consumers, it currently ignores transition/state consistency checks that you already implemented invalidate_fsm_structure().def validate_instance(self) -> bool: @@ if not self.initial_state or not self.initial_state.strip(): return False + if self.validate_fsm_structure(): + return False return Truesrc/omnibase_core/models/fsm/model_fsm_state.py (1)
41-45: PreferConfigDict(typed) and consider schema-level non-empty enforcement.
UsingConfigDictimproves typing/mypy friendliness, and ifnamemust never be blank, enforcing it at the Pydantic layer (not only viavalidate_instance()) reduces invalid states floating around.@@ -from pydantic import BaseModel +from pydantic import BaseModel, ConfigDict @@ - model_config = { - "extra": "ignore", - "use_enum_values": False, - "frozen": True, - } + model_config = ConfigDict(extra="ignore", use_enum_values=False, frozen=True)@@ - name: str = Field(default=..., description="State name") + name: str = Field(default=..., min_length=1, description="State name")Also applies to: 63-75
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/omnibase_core/models/fsm/model_fsm_data_primary.py(2 hunks)src/omnibase_core/models/fsm/model_fsm_operation.py(1 hunks)src/omnibase_core/models/fsm/model_fsm_state.py(2 hunks)src/omnibase_core/models/fsm/model_fsm_state_snapshot.py(1 hunks)src/omnibase_core/models/fsm/model_fsm_transition.py(2 hunks)src/omnibase_core/types/type_fsm_context.py(1 hunks)tests/unit/models/fsm/test_model_fsm_data.py(3 hunks)tests/unit/models/fsm/test_model_fsm_state.py(3 hunks)tests/unit/models/fsm/test_model_fsm_transition.py(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- tests/unit/models/fsm/test_model_fsm_data.py
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{py,txt}
📄 CodeRabbit inference engine (CLAUDE.md)
Always use Poetry for Python package management and task execution. Use
poetry runprefix for all commands (pytest, mypy, python, etc.) - never use pip or python directly
Files:
src/omnibase_core/types/type_fsm_context.pysrc/omnibase_core/models/fsm/model_fsm_operation.pysrc/omnibase_core/models/fsm/model_fsm_transition.pysrc/omnibase_core/models/fsm/model_fsm_data_primary.pysrc/omnibase_core/models/fsm/model_fsm_state.pysrc/omnibase_core/models/fsm/model_fsm_state_snapshot.pytests/unit/models/fsm/test_model_fsm_transition.pytests/unit/models/fsm/test_model_fsm_state.py
src/omnibase_core/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/omnibase_core/**/*.py: Use protocol names for dependency injection, never concrete class names. Example:container.get_service('ProtocolEventBus')not concrete class names
Always use ModelOnexError for error handling, never generic Exception. Include error_code from EnumCoreErrorCode and context information
Use @standard_error_handling decorator to eliminate 6+ lines of try/catch boilerplate in async operations
Enforce 100% strict mypy compliance - all functions must have type annotations withdisallow_untyped_defs = true
Use EnumNodeKind for architectural classification (EFFECT, COMPUTE, REDUCER, ORCHESTRATOR, RUNTIME_HOST) in workflow routing
Use EnumNodeType for specific implementation type discovery and capability matching (TRANSFORMER, AGGREGATOR, COMPUTE_GENERIC, etc.)
Format code using Black and sort imports using isort - runpoetry run black src/ tests/andpoetry run isort src/ tests/before commit
Run all pre-commit hooks before pushing:pre-commit run --all-files- includes mypy strict, black, isort, ruff
Use duck typing with protocols - never use isinstance checks. Protocols use structural subtyping, not explicit inheritance
Process inter-service communication using ModelEventEnvelope pattern for event-driven architecture
Use thread-local storage or separate node instances for concurrent thread access - ModelComputeCache, ModelCircuitBreaker, NodeCompute/NodeEffect are NOT thread-safe
Files:
src/omnibase_core/types/type_fsm_context.pysrc/omnibase_core/models/fsm/model_fsm_operation.pysrc/omnibase_core/models/fsm/model_fsm_transition.pysrc/omnibase_core/models/fsm/model_fsm_data_primary.pysrc/omnibase_core/models/fsm/model_fsm_state.pysrc/omnibase_core/models/fsm/model_fsm_state_snapshot.py
src/omnibase_core/models/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use Pydantic models for all data structures with v2.11+ compatibility, never use plain dataclasses or Python dicts for domain models
Files:
src/omnibase_core/models/fsm/model_fsm_operation.pysrc/omnibase_core/models/fsm/model_fsm_transition.pysrc/omnibase_core/models/fsm/model_fsm_data_primary.pysrc/omnibase_core/models/fsm/model_fsm_state.pysrc/omnibase_core/models/fsm/model_fsm_state_snapshot.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Maintain minimum 60% test coverage requirement - runpoetry run pytest tests/ --cov=src/omnibase_core --cov-report=term-missing
Run pytest with parallel workers for faster local testing:poetry run pytest tests/ -n 4(default: 4 workers, can disable with-n 0for debugging)
Use pytest markers to categorize tests: @pytest.mark.unit, @pytest.mark.integration, @pytest.mark.slow, @pytest.mark.smoke, @pytest.mark.performance
CI runs 20 parallel test splits with timeout protection (60s per test) - expected per-split runtime is 2m30s to 3m30s, critical alert if > 4m30s
Files:
tests/unit/models/fsm/test_model_fsm_transition.pytests/unit/models/fsm/test_model_fsm_state.py
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-11T21:05:41.997Z
Learning: Applies to src/omnibase_core/nodes/node_reducer.py : Implement REDUCER nodes using NodeReducer base class with FSM-driven state transitions using ModelIntent pattern
📚 Learning: 2025-12-11T21:05:41.997Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-11T21:05:41.997Z
Learning: Applies to src/omnibase_core/**/*.py : Always use ModelOnexError for error handling, never generic Exception. Include error_code from EnumCoreErrorCode and context information
Applied to files:
src/omnibase_core/models/fsm/model_fsm_data_primary.py
🧬 Code graph analysis (4)
src/omnibase_core/models/fsm/model_fsm_data_primary.py (3)
src/omnibase_core/models/fsm/model_fsm_operation.py (3)
execute(85-111)serialize(113-119)validate_instance(121-153)src/omnibase_core/models/fsm/model_fsm_state.py (3)
execute(49-57)serialize(59-61)validate_instance(63-75)src/omnibase_core/models/fsm/model_fsm_transition.py (3)
execute(43-51)serialize(53-55)validate_instance(57-77)
src/omnibase_core/models/fsm/model_fsm_state.py (3)
src/omnibase_core/models/fsm/model_fsm_data_primary.py (1)
execute(95-103)src/omnibase_core/models/fsm/model_fsm_operation.py (1)
execute(85-111)src/omnibase_core/models/fsm/model_fsm_transition.py (1)
execute(43-51)
tests/unit/models/fsm/test_model_fsm_transition.py (1)
src/omnibase_core/models/fsm/model_fsm_transition.py (3)
ModelFsmTransition(16-77)execute(43-51)validate_instance(57-77)
tests/unit/models/fsm/test_model_fsm_state.py (5)
src/omnibase_core/models/fsm/model_fsm_state.py (3)
ModelFsmState(16-75)execute(49-57)validate_instance(63-75)src/omnibase_core/models/fsm/model_fsm_data_primary.py (2)
execute(95-103)validate_instance(109-125)src/omnibase_core/models/fsm/model_fsm_operation.py (2)
execute(85-111)validate_instance(121-153)src/omnibase_core/models/fsm/model_fsm_transition.py (2)
execute(43-51)validate_instance(57-77)tests/unit/models/fsm/test_model_fsm_transition.py (1)
test_frozen_config(464-474)
⏰ 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: Smoke Tests
- GitHub Check: claude-review
🔇 Additional comments (11)
src/omnibase_core/models/fsm/model_fsm_transition.py (1)
43-51: Non-mutatingexecute()matches the frozen model semantics.src/omnibase_core/models/fsm/model_fsm_operation.py (1)
64-81: Good:min_length=1+ frozen config is aligned with v1.0 validation goals.src/omnibase_core/types/type_fsm_context.py (1)
1-25: LGTM: centralizedFSMContextTypekeeps the “intentional Any” surface explicit.tests/unit/models/fsm/test_model_fsm_state.py (3)
195-208: Good coverage:execute(**kwargs)stays non-mutating underfrozen=True.
412-423: Good: frozen mutation assertions mirror the expected Pydantic behavior.
438-470: Nice:validate_instance()negative cases cover whitespace variants (tabs/newlines/mixed).src/omnibase_core/models/fsm/model_fsm_data_primary.py (1)
95-104: Non-mutatingexecute()is consistent with the frozen model direction.src/omnibase_core/models/fsm/model_fsm_state.py (1)
41-57: Freezing the model + no-opexecute()is consistent with the v1.0 immutability contract.
This aligns well with the other FSM models’ “frozen + execute returns True without mutation” pattern.tests/unit/models/fsm/test_model_fsm_transition.py (3)
205-220: Good:execute(**kwargs)test now asserts “no mutation” underfrozen=True.
The assertiontransition.trigger == "start"is the key behavior to lock in for thread-safety semantics.
464-475: Frozen-mutation test looks right; verify expected exception type on your Pydantic version.
Some setups surface different exception flavors/messages for frozen-instance assignment; worth confirming CI behavior stays stable across your supported Pydantic range.
512-562: Nice coverage forvalidate_instance()false/true cases.The test class provides good coverage with multiple test methods validating both valid and invalid states (empty strings, whitespace-only values) for
from_state,to_state, andtriggerattributes. No duplicate class definitions found in the repository.
| from dataclasses import dataclass, field | ||
| from typing import Any | ||
|
|
||
| from omnibase_core.types.type_fsm_context import FSMContextType | ||
|
|
||
|
|
||
| @dataclass(frozen=True) | ||
| class ModelFSMStateSnapshot: |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Model in src/omnibase_core/models/** should be Pydantic (not dataclass).
Per guidelines, this should be a Pydantic v2 model to keep the domain-model layer consistent (serialization/validation behavior, config patterns, etc.).
As per coding guidelines, ...
@@
-from dataclasses import dataclass, field
+from pydantic import BaseModel, ConfigDict, Field
@@
-@dataclass(frozen=True)
-class ModelFSMStateSnapshot:
+class ModelFSMStateSnapshot(BaseModel):
@@
- current_state: str
- context: FSMContextType
- history: list[str] = field(default_factory=list)
+ model_config = ConfigDict(frozen=True, extra="ignore")
+
+ current_state: str = Field(default=..., description="Current state name")
+ context: FSMContextType = Field(default=..., description="FSM runtime context")
+ history: list[str] = Field(default_factory=list, description="Visited states")Also applies to: 48-53
Address all remaining PR #165 review feedback: Deep Immutability: - Convert list fields to tuple[...] for true frozen behavior - ModelFsmTransition: conditions/actions now tuple[str, ...] - ModelFsmData: states/transitions/global_actions now tuples - ModelFsmData: variables/metadata now tuple[tuple[str, str], ...] - ModelFSMStateSnapshot: history now tuple[str, ...] instead of list - Add field validators for convenient list/dict input conversion Code Quality: - Use ConfigDict instead of dict for model_config (typed) - Mark unused kwargs with _ = kwargs in execute() methods - Add min_length=1 constraints to ModelFsmTransition fields - Add @pytest.mark.unit to all test classes - Enhanced docstrings with immutability contract documentation - Use FSMContextType alias instead of dict[str, Any] All 309 FSM model tests pass. mypy strict and ruff lint pass.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/omnibase_core/utils/fsm_executor.py (2)
392-406: Condition evaluation semantics + comparison fallbacks can produce false positives (missing field ⇒ passes).
- Skipping
required=Falseconditions means they never gate transitions (confirm spec intent).greater_than/less_than: missing/None currently coerces to0, somissing > -1becomesTrue.min_length: empty sized values should compare by length (e.g.,len([]) >= 0), butif not field_value: return Falsebreaks that.A safer pattern is: always evaluate; only fail the transition when
condition.requiredand the condition is not met; and treatNone/missing as “not met” for numeric/length operators.async def _evaluate_conditions( @@ - for condition in transition.conditions: - # Skip optional conditions if not required - if not condition.required: - continue - - # Evaluate condition based on type and expression - condition_met = await _evaluate_single_condition(condition, context) - - if not condition_met: - return False + for condition in transition.conditions: + condition_met = await _evaluate_single_condition(condition, context) + if not condition_met and condition.required: + return False @@ elif operator == "min_length": - if not field_value: - return False + if field_value is None: + return False try: # Cast to Sized for len() - TypeError caught if not actually Sized return len(cast(Sized, field_value)) >= int(expected_value or "0") @@ elif operator == "max_length": - if not field_value: - return True + if field_value is None: + return False try: @@ elif operator == "greater_than": + if field_value is None: + return False try: - # Cast to SupportsFloat - TypeError caught if not actually numeric - return float(cast(SupportsFloat, field_value) or 0) > float( - expected_value or "0" - ) + return float(cast(SupportsFloat, field_value)) > float(expected_value or "0") @@ elif operator == "less_than": + if field_value is None: + return False try: - # Cast to SupportsFloat - TypeError caught if not actually numeric - return float(cast(SupportsFloat, field_value) or 0) < float( - expected_value or "0" - ) + return float(cast(SupportsFloat, field_value)) < float(expected_value or "0")Also applies to: 484-515
12-35: Fix condition evaluation logic and field value handling in FSM executor.Three confirmed issues:
_evaluate_conditions (lines 378-382) skips optional conditions (
required=False) entirely instead of evaluating them. If a transition has only optional conditions, the method returnsTruewithout any evaluation. Conditions markedrequired=Falseshould still be evaluated; they should just not fail the transition if they'reFalse.min_length operator (lines 409-412) incorrectly rejects empty collections:
if not field_value: return Falsecausesmin_length([], 0)to returnFalse, but an empty collection has length 0 and should pass amin_length 0check.Missing field handling (lines 525-531) in numeric comparisons silently converts missing/None values to 0 via
field_value or 0. Missing fields should be handled explicitly rather than defaulting to 0, which may mask data errors.The subcontract transition condition typing is correct—
ModelFSMStateTransition.conditionsis properly typed aslist[ModelFSMTransitionCondition]and matches the import in fsm_executor.py.src/omnibase_core/mixins/mixin_fsm_execution.py (1)
55-114: Store context as a shallow copy to avoid mutating shared mutable dict references across FSMState snapshots and intent payloads.The mutable
contextdict is stored by reference in FSMState (lines 112, 204), which violates the documented immutability contract in ModelFSMStateSnapshot (lines 28-30: "FSM executors should create new snapshots with new context dicts"). The same context reference then appears in multiple intent payloads (lines 177, 600), creating a shared mutable reference that could be modified by downstream code. While ModelIntent enforces frozen=True, its documentation explicitly notes shallow immutability: "mutable field values (like dict/list contents) can still be modified" and recommendsmodel_copy(deep=True)for full thread safety.Apply the suggested shallow copy fix to maintain the documented immutability contract:
async def execute_fsm_transition( @@ - result = await execute_transition( + context_snapshot: dict[str, object] = dict(context) + result = await execute_transition( fsm_contract, current_state, trigger, - context, + context_snapshot, ) @@ self._fsm_state = FSMState( current_state=result.new_state, - context=context, + context=context_snapshot, history=new_history, ) @@ def initialize_fsm_state( self, fsm_contract: ModelFSMSubcontract, context: FSMContextType | None = None ) -> None: @@ self._fsm_state = FSMState( current_state=fsm_contract.initial_state, - context=context or {}, + context=dict(context) if context else {}, history=(), )
♻️ Duplicate comments (1)
src/omnibase_core/models/fsm/model_fsm_state_snapshot.py (1)
36-42: Model should use Pydantic BaseModel, not dataclass.Per coding guidelines, all models in
src/omnibase_core/models/**must use Pydantic v2.11+ for consistency with the domain model layer. This was flagged in a prior review. The extensive docstring explaining dict mutability limitations (lines 19-30) could be simplified by using Pydantic'sfrozen=Trueconfig.-from dataclasses import dataclass +from pydantic import BaseModel, ConfigDict, Field from omnibase_core.types.type_fsm_context import FSMContextType -@dataclass(frozen=True) -class ModelFSMStateSnapshot: +class ModelFSMStateSnapshot(BaseModel): + """Immutable FSM state snapshot for pure FSM operations.""" + + model_config = ConfigDict(frozen=True, extra="ignore") + + current_state: str = Field(..., description="Current state name") + context: FSMContextType = Field(..., description="FSM runtime context") + history: tuple[str, ...] = Field(default=(), description="Visited states")
🧹 Nitpick comments (5)
src/omnibase_core/models/fsm/model_fsm_transition_condition.py (1)
88-92:validate_assignment=Trueis redundant withfrozen=True.Since the model is frozen, field assignment is blocked entirely, making
validate_assignmentunnecessary. Consider removing it for clarity.model_config = ConfigDict( extra="ignore", use_enum_values=False, frozen=True, + # validate_assignment is unnecessary when frozen=True )src/omnibase_core/models/fsm/model_fsm_state.py (1)
28-36: Mutable collections break frozen immutability guarantee.
entry_actions,exit_actions, andpropertiesuse mutable types (list,dict). Withfrozen=True, the fields can't be reassigned but their contents can still be mutated in-place. For consistency with the immutability design (seeModelFsmTransition.conditions/actionswhich use tuples), consider:- entry_actions: list[str] = Field( - default_factory=list, description="Actions on state entry" + entry_actions: tuple[str, ...] = Field( + default=(), description="Actions on state entry" ) - exit_actions: list[str] = Field( - default_factory=list, description="Actions on state exit" + exit_actions: tuple[str, ...] = Field( + default=(), description="Actions on state exit" ) - properties: dict[str, str] = Field( - default_factory=dict, description="State properties" + properties: tuple[tuple[str, str], ...] = Field( + default=(), description="State properties as key-value tuples" )Alternatively, if dict semantics are needed for
properties, document the mutability contract as done inModelFSMStateSnapshot.tests/unit/models/fsm/test_model_fsm_data.py (1)
763-816: Newvalidate_instance()false/true coverage is good; consider dropping any “empty name is allowed” instantiation tests if you later enforcemin_length=1.
This suite now explicitly defines “invalid-but-constructible” vs “invalid-and-rejected” behavior—worth keeping consistent across FSM models.tests/unit/models/fsm/test_model_fsm_transition.py (1)
474-485: Make frozen-assignment tests resilient to Pydantic’s exact exception type.
Depending on Pydantic/config, assignment to a frozen model may raiseValidationErrororTypeError.- with pytest.raises(ValidationError): + with pytest.raises((ValidationError, TypeError)): transition.trigger = "updated"src/omnibase_core/models/fsm/model_fsm_data_primary.py (1)
127-133: Consider returning tuples for consistency with immutability approach.The helper methods
get_transitions_from_stateandget_transitions_to_statereturnlist[ModelFsmTransition], which is mutable. For consistency with the model's deep immutability design, consider returning tuples instead.-def get_transitions_from_state(self, state_name: str) -> list[ModelFsmTransition]: +def get_transitions_from_state(self, state_name: str) -> tuple[ModelFsmTransition, ...]: """Get all transitions from a specific state.""" - return [t for t in self.transitions if t.from_state == state_name] + return tuple(t for t in self.transitions if t.from_state == state_name) -def get_transitions_to_state(self, state_name: str) -> list[ModelFsmTransition]: +def get_transitions_to_state(self, state_name: str) -> tuple[ModelFsmTransition, ...]: """Get all transitions to a specific state.""" - return [t for t in self.transitions if t.to_state == state_name] + return tuple(t for t in self.transitions if t.to_state == state_name)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
src/omnibase_core/mixins/mixin_fsm_execution.py(6 hunks)src/omnibase_core/models/fsm/model_fsm_data_primary.py(2 hunks)src/omnibase_core/models/fsm/model_fsm_operation.py(1 hunks)src/omnibase_core/models/fsm/model_fsm_state.py(3 hunks)src/omnibase_core/models/fsm/model_fsm_state_snapshot.py(1 hunks)src/omnibase_core/models/fsm/model_fsm_transition.py(3 hunks)src/omnibase_core/models/fsm/model_fsm_transition_action.py(1 hunks)src/omnibase_core/models/fsm/model_fsm_transition_condition.py(1 hunks)src/omnibase_core/utils/fsm_executor.py(10 hunks)tests/unit/mixins/test_mixin_fsm_execution.py(1 hunks)tests/unit/models/fsm/test_model_fsm_data.py(12 hunks)tests/unit/models/fsm/test_model_fsm_state.py(8 hunks)tests/unit/models/fsm/test_model_fsm_transition.py(16 hunks)tests/unit/utils/test_fsm_executor.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/unit/utils/test_fsm_executor.py
- src/omnibase_core/models/fsm/model_fsm_transition_action.py
- tests/unit/models/fsm/test_model_fsm_state.py
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{py,txt}
📄 CodeRabbit inference engine (CLAUDE.md)
Always use Poetry for Python package management and task execution. Use
poetry runprefix for all commands (pytest, mypy, python, etc.) - never use pip or python directly
Files:
src/omnibase_core/models/fsm/model_fsm_transition.pysrc/omnibase_core/models/fsm/model_fsm_transition_condition.pysrc/omnibase_core/models/fsm/model_fsm_state.pytests/unit/models/fsm/test_model_fsm_data.pysrc/omnibase_core/mixins/mixin_fsm_execution.pysrc/omnibase_core/utils/fsm_executor.pytests/unit/mixins/test_mixin_fsm_execution.pysrc/omnibase_core/models/fsm/model_fsm_state_snapshot.pysrc/omnibase_core/models/fsm/model_fsm_data_primary.pytests/unit/models/fsm/test_model_fsm_transition.pysrc/omnibase_core/models/fsm/model_fsm_operation.py
src/omnibase_core/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/omnibase_core/**/*.py: Use protocol names for dependency injection, never concrete class names. Example:container.get_service('ProtocolEventBus')not concrete class names
Always use ModelOnexError for error handling, never generic Exception. Include error_code from EnumCoreErrorCode and context information
Use @standard_error_handling decorator to eliminate 6+ lines of try/catch boilerplate in async operations
Enforce 100% strict mypy compliance - all functions must have type annotations withdisallow_untyped_defs = true
Use EnumNodeKind for architectural classification (EFFECT, COMPUTE, REDUCER, ORCHESTRATOR, RUNTIME_HOST) in workflow routing
Use EnumNodeType for specific implementation type discovery and capability matching (TRANSFORMER, AGGREGATOR, COMPUTE_GENERIC, etc.)
Format code using Black and sort imports using isort - runpoetry run black src/ tests/andpoetry run isort src/ tests/before commit
Run all pre-commit hooks before pushing:pre-commit run --all-files- includes mypy strict, black, isort, ruff
Use duck typing with protocols - never use isinstance checks. Protocols use structural subtyping, not explicit inheritance
Process inter-service communication using ModelEventEnvelope pattern for event-driven architecture
Use thread-local storage or separate node instances for concurrent thread access - ModelComputeCache, ModelCircuitBreaker, NodeCompute/NodeEffect are NOT thread-safe
Files:
src/omnibase_core/models/fsm/model_fsm_transition.pysrc/omnibase_core/models/fsm/model_fsm_transition_condition.pysrc/omnibase_core/models/fsm/model_fsm_state.pysrc/omnibase_core/mixins/mixin_fsm_execution.pysrc/omnibase_core/utils/fsm_executor.pysrc/omnibase_core/models/fsm/model_fsm_state_snapshot.pysrc/omnibase_core/models/fsm/model_fsm_data_primary.pysrc/omnibase_core/models/fsm/model_fsm_operation.py
src/omnibase_core/models/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use Pydantic models for all data structures with v2.11+ compatibility, never use plain dataclasses or Python dicts for domain models
Files:
src/omnibase_core/models/fsm/model_fsm_transition.pysrc/omnibase_core/models/fsm/model_fsm_transition_condition.pysrc/omnibase_core/models/fsm/model_fsm_state.pysrc/omnibase_core/models/fsm/model_fsm_state_snapshot.pysrc/omnibase_core/models/fsm/model_fsm_data_primary.pysrc/omnibase_core/models/fsm/model_fsm_operation.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Maintain minimum 60% test coverage requirement - runpoetry run pytest tests/ --cov=src/omnibase_core --cov-report=term-missing
Run pytest with parallel workers for faster local testing:poetry run pytest tests/ -n 4(default: 4 workers, can disable with-n 0for debugging)
Use pytest markers to categorize tests: @pytest.mark.unit, @pytest.mark.integration, @pytest.mark.slow, @pytest.mark.smoke, @pytest.mark.performance
CI runs 20 parallel test splits with timeout protection (60s per test) - expected per-split runtime is 2m30s to 3m30s, critical alert if > 4m30s
Files:
tests/unit/models/fsm/test_model_fsm_data.pytests/unit/mixins/test_mixin_fsm_execution.pytests/unit/models/fsm/test_model_fsm_transition.py
🧠 Learnings (4)
📓 Common learnings
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-11T21:05:41.997Z
Learning: Applies to src/omnibase_core/nodes/node_reducer.py : Implement REDUCER nodes using NodeReducer base class with FSM-driven state transitions using ModelIntent pattern
📚 Learning: 2025-12-11T21:05:41.997Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-11T21:05:41.997Z
Learning: Applies to src/omnibase_core/models/**/*.py : Use Pydantic models for all data structures with v2.11+ compatibility, never use plain dataclasses or Python dicts for domain models
Applied to files:
src/omnibase_core/models/fsm/model_fsm_state_snapshot.py
📚 Learning: 2025-12-11T21:05:41.997Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-11T21:05:41.997Z
Learning: Applies to src/omnibase_core/**/*.py : Process inter-service communication using ModelEventEnvelope pattern for event-driven architecture
Applied to files:
src/omnibase_core/models/fsm/model_fsm_state_snapshot.py
📚 Learning: 2025-12-11T21:05:41.997Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-11T21:05:41.997Z
Learning: Applies to src/omnibase_core/**/*.py : Always use ModelOnexError for error handling, never generic Exception. Include error_code from EnumCoreErrorCode and context information
Applied to files:
src/omnibase_core/models/fsm/model_fsm_state_snapshot.py
🧬 Code graph analysis (7)
src/omnibase_core/models/fsm/model_fsm_transition.py (5)
src/omnibase_core/models/fsm/model_fsm_data_primary.py (1)
execute(169-177)src/omnibase_core/models/fsm/model_fsm_operation.py (1)
execute(85-112)src/omnibase_core/models/fsm/model_fsm_state.py (1)
execute(46-54)src/omnibase_core/models/fsm/model_fsm_transition_action.py (1)
execute(117-139)src/omnibase_core/models/fsm/model_fsm_transition_condition.py (1)
execute(125-142)
tests/unit/models/fsm/test_model_fsm_data.py (2)
src/omnibase_core/models/fsm/model_fsm_data_primary.py (3)
get_state_by_name(120-125)ModelFsmData(24-199)validate_instance(183-199)src/omnibase_core/models/fsm/model_fsm_state.py (2)
ModelFsmState(13-72)validate_instance(60-72)
src/omnibase_core/utils/fsm_executor.py (1)
src/omnibase_core/models/fsm/model_fsm_transition_condition.py (1)
ModelFSMTransitionCondition(21-213)
tests/unit/mixins/test_mixin_fsm_execution.py (1)
src/omnibase_core/mixins/mixin_fsm_execution.py (1)
get_fsm_state_history(158-169)
src/omnibase_core/models/fsm/model_fsm_data_primary.py (4)
src/omnibase_core/models/fsm/model_fsm_state.py (4)
ModelFsmState(13-72)execute(46-54)serialize(56-58)validate_instance(60-72)src/omnibase_core/models/fsm/model_fsm_transition.py (4)
ModelFsmTransition(14-81)execute(47-55)serialize(57-59)validate_instance(61-81)src/omnibase_core/models/fsm/model_fsm_operation.py (3)
execute(85-112)serialize(114-120)validate_instance(122-154)src/omnibase_core/models/fsm/model_fsm_transition_condition.py (3)
execute(125-142)serialize(144-150)validate_instance(152-213)
tests/unit/models/fsm/test_model_fsm_transition.py (1)
src/omnibase_core/models/fsm/model_fsm_transition.py (3)
ModelFsmTransition(14-81)execute(47-55)validate_instance(61-81)
src/omnibase_core/models/fsm/model_fsm_operation.py (5)
src/omnibase_core/models/fsm/model_fsm_data_primary.py (3)
execute(169-177)serialize(179-181)validate_instance(183-199)src/omnibase_core/models/fsm/model_fsm_state.py (3)
execute(46-54)serialize(56-58)validate_instance(60-72)src/omnibase_core/models/fsm/model_fsm_transition.py (3)
execute(47-55)serialize(57-59)validate_instance(61-81)src/omnibase_core/models/fsm/model_fsm_transition_action.py (3)
execute(117-139)serialize(141-147)validate_instance(149-179)src/omnibase_core/models/fsm/model_fsm_transition_condition.py (3)
execute(125-142)serialize(144-150)validate_instance(152-213)
🔇 Additional comments (16)
src/omnibase_core/models/fsm/model_fsm_transition_condition.py (2)
21-49: Well-structured model with proper protocol implementations.Good use of ModelOnexError with context, frozen config for thread safety, and clear documentation of v1.0 vs v1.1+ behavior. The reserved fields pattern is clean.
107-121: Expression parsing withsplit()is intentionally limited to single-word tokens per spec.The 3-token validation using
split()correctly enforces the spec requirement (CONTRACT_DRIVEN_NODEREDUCER_V1_0.md, lines 2022–2024): "v1.0 intentionally restricts expression grammar to eliminate ambiguity and reduce implementation surface area. Complex conditions must be encoded as precomputed fields in context."Expressions like
"name == 'John Doe'"or"status == pending approval"are out of scope for v1.0. The docstring examples and acceptance criteria confirm this is the intended design. If multi-word values are needed, they should be precomputed as context fields.src/omnibase_core/models/fsm/model_fsm_state.py (1)
60-72: Validation behavior differs from other FSM models.This
validate_instancereturnsFalseon failure, whileModelFSMTransitionCondition.validate_instanceraisesModelOnexError. If both implement the sameProtocolValidatableprotocol, consider aligning the behavior for consistency.tests/unit/mixins/test_mixin_fsm_execution.py (1)
176-180: Correctly updated assertion for tuple-based history.The test now expects
()instead of[], aligning with the immutability refactor whereget_fsm_state_history()returnstuple[str, ...].src/omnibase_core/models/fsm/model_fsm_transition.py (2)
32-37: Past review concern resolved: collections are now tuples.The
conditionsandactionsfields are nowtuple[str, ...]instead oflist[str], providing true immutability alongsidefrozen=True. This addresses the prior review feedback.
61-81: Solid validation combining Pydantic constraints and explicit checks.The combination of
min_length=1(blocks empty strings at construction) andstrip()checks invalidate_instance(catches whitespace-only strings) provides thorough validation coverage.tests/unit/models/fsm/test_model_fsm_data.py (2)
21-51: Tuple-based immutability assertions look consistent with the ModelFsmData “freeze on input” behavior.Also applies to: 81-86, 744-748
217-235:test_get_state_by_name_empty_statesrewrite is clearer given frozen models (searching for non-existent state instead of mutating).tests/unit/models/fsm/test_model_fsm_transition.py (1)
18-53: Coverage aligns well with tuple-based immutability andvalidate_instance()whitespace semantics.Also applies to: 138-176, 208-223, 250-252, 289-311, 416-429, 522-571
src/omnibase_core/mixins/mixin_fsm_execution.py (1)
14-15: Tuple-based history updates are clean and consistent with the broader immutability shift.Also applies to: 158-170, 107-114, 205-206
src/omnibase_core/models/fsm/model_fsm_operation.py (1)
1-14: No action required. The class nameModelFSMOperationis correct as-is.The search found zero usages of
ModelMvpFSMOperationacross the entire codebase, andModelFSMOperationis actively used in 80+ locations including tests, contracts, and exports. The codebase pattern uses "MVP" as a status indicator in docstrings (e.g., "MVP implementation with kind only") rather than as a class name prefix. The recent commit (PR #165) completed without introducing an MVP-prefixed variant, confirming the current naming is intentional and correct.The model also properly implements Pydantic v2 with
min_length=1constraints and correct protocol methods (execute,serialize,validate_instance).src/omnibase_core/models/fsm/model_fsm_data_primary.py (5)
8-14: LGTM! Deep immutability approach addresses previous concerns.The updated docstring clearly documents the deep immutability strategy using both
frozen=Trueand immutable types. This directly addresses the past review comment aboutfrozen=Trueonly preventing field reassignment but not in-place collection mutations.
24-47: LGTM! Excellent documentation of immutability guarantees.The class docstring thoroughly explains the deep immutability approach and provides practical usage examples for accessing the tuple-based dictionary fields. This will help developers understand the design trade-offs.
54-68: LGTM! Fields correctly implement deep immutability.The conversion from mutable collections (lists and dicts) to immutable types (tuples and tuple-of-tuples) ensures genuine immutability at all nesting levels. This is the correct approach for thread-safe, immutable data structures.
161-165: LGTM! Model config correctly enforces immutability.The
frozen=Trueconfiguration combined with immutable field types ensures complete immutability at all levels. The configuration is consistent with other FSM models in the codebase.
183-199: LGTM! Validation logic is correct and consistent.The
validate_instancemethod correctly validates thatstate_machine_nameandinitial_stateare non-empty and non-whitespace. The pattern of returningFalsefor validation failures is consistent with other FSM models (ModelFsmState,ModelFsmTransition,ModelFSMOperation) that don't have arequiredflag.
| @field_validator("states", mode="before") | ||
| @classmethod | ||
| def _convert_states_to_tuple( | ||
| cls, v: list[Any] | tuple[Any, ...] | Any | ||
| ) -> tuple[Any, ...]: | ||
| """Convert list of states to tuple for deep immutability.""" | ||
| if isinstance(v, list): | ||
| return tuple(v) | ||
| return v | ||
|
|
||
| @field_validator("transitions", mode="before") | ||
| @classmethod | ||
| def _convert_transitions_to_tuple( | ||
| cls, v: list[Any] | tuple[Any, ...] | Any | ||
| ) -> tuple[Any, ...]: | ||
| """Convert list of transitions to tuple for deep immutability.""" | ||
| if isinstance(v, list): | ||
| return tuple(v) | ||
| return v | ||
|
|
||
| @field_validator("global_actions", mode="before") | ||
| @classmethod | ||
| def _convert_global_actions_to_tuple( | ||
| cls, v: list[str] | tuple[str, ...] | Any | ||
| ) -> tuple[str, ...]: | ||
| """Convert list of global actions to tuple for deep immutability.""" | ||
| if isinstance(v, list): | ||
| return tuple(v) | ||
| return v | ||
|
|
||
| @field_validator("variables", mode="before") | ||
| @classmethod | ||
| def _convert_variables_to_frozen( | ||
| cls, v: dict[str, str] | tuple[tuple[str, str], ...] | Any | ||
| ) -> tuple[tuple[str, str], ...]: | ||
| """Convert dict to tuple of tuples for deep immutability.""" | ||
| if isinstance(v, dict): | ||
| return tuple(v.items()) | ||
| return v | ||
|
|
||
| @field_validator("metadata", mode="before") | ||
| @classmethod | ||
| def _convert_metadata_to_frozen( | ||
| cls, v: dict[str, str] | tuple[tuple[str, str], ...] | Any | ||
| ) -> tuple[tuple[str, str], ...]: | ||
| """Convert dict to tuple of tuples for deep immutability.""" | ||
| if isinstance(v, dict): | ||
| return tuple(v.items()) | ||
| return v |
There was a problem hiding this comment.
Tighten validator type hints for strict mypy compliance.
The field validators use Any in their type hints (e.g., list[Any] | tuple[Any, ...] | Any), which violates the project's strict mypy compliance requirement (disallow_untyped_defs = true). The trailing | Any also defeats type safety by allowing anything.
As per coding guidelines: "Enforce 100% strict mypy compliance - all functions must have type annotations with disallow_untyped_defs = true"
Consider these more specific type hints:
@field_validator("states", mode="before")
@classmethod
def _convert_states_to_tuple(
- cls, v: list[Any] | tuple[Any, ...] | Any
+ cls, v: list[ModelFsmState | dict[str, object]] | tuple[ModelFsmState, ...]
) -> tuple[Any, ...]:
"""Convert list of states to tuple for deep immutability."""
if isinstance(v, list):
return tuple(v)
return v
@field_validator("transitions", mode="before")
@classmethod
def _convert_transitions_to_tuple(
- cls, v: list[Any] | tuple[Any, ...] | Any
+ cls, v: list[ModelFsmTransition | dict[str, object]] | tuple[ModelFsmTransition, ...]
) -> tuple[Any, ...]:
"""Convert list of transitions to tuple for deep immutability."""
if isinstance(v, list):
return tuple(v)
return v
@field_validator("global_actions", mode="before")
@classmethod
def _convert_global_actions_to_tuple(
- cls, v: list[str] | tuple[str, ...] | Any
+ cls, v: list[str] | tuple[str, ...]
) -> tuple[str, ...]:
"""Convert list of global actions to tuple for deep immutability."""
if isinstance(v, list):
return tuple(v)
return v
@field_validator("variables", mode="before")
@classmethod
def _convert_variables_to_frozen(
- cls, v: dict[str, str] | tuple[tuple[str, str], ...] | Any
+ cls, v: dict[str, str] | tuple[tuple[str, str], ...]
) -> tuple[tuple[str, str], ...]:
"""Convert dict to tuple of tuples for deep immutability."""
if isinstance(v, dict):
return tuple(v.items())
return v
@field_validator("metadata", mode="before")
@classmethod
def _convert_metadata_to_frozen(
- cls, v: dict[str, str] | tuple[tuple[str, str], ...] | Any
+ cls, v: dict[str, str] | tuple[tuple[str, str], ...]
) -> tuple[tuple[str, str], ...]:
"""Convert dict to tuple of tuples for deep immutability."""
if isinstance(v, dict):
return tuple(v)
return vNote: The isinstance checks are acceptable in Pydantic validators where runtime type checking is necessary.
Committable suggestion skipped: line range outside the PR's diff.
Merged origin/main into feature branch, resolving conflicts in favor of FSMContextType for type consistency: - mixin_fsm_execution.py: Use FSMContextType instead of SerializedDict - model_fsm_state_snapshot.py: Keep FSMContextType and immutable tuple history - fsm_executor.py: Use FSMContextType for all context parameters The FSMContextType is purpose-built for FSM context and provides clearer semantics than the generic SerializedDict.
Change return type from list[str] to tuple[str, ...] to match the immutable tuple returned by MixinFSMExecution.get_fsm_state_history(). This maintains type consistency after merge conflict resolution.
Code Review - PR #165: MVP FSM Contract Models for NodeReducer v1.0Overall Assessment: ✅ APPROVE WITH MINOR OBSERVATIONS This is an excellent PR that adds foundational FSM contract models for NodeReducer v1.0. The implementation follows ONEX conventions rigorously, includes comprehensive test coverage (214 tests), and demonstrates strong architectural thinking with forward compatibility. 🎯 Strengths1. Exceptional Code Quality
2. Strong Type Safety
3. Excellent Test Coverage
4. Smart Architecture Decisions
📝 Key Observations1. ModelActionConfigParameter Missing Immutability
|
… type fixes ## CI Fixes - Fix ruff formatting issues in 14 files (mixins, models/common, models/configuration, models/core, models/infrastructure) ## FSM Deep Immutability - Convert list fields to tuple[T, ...] in model_fsm_state.py (entry_actions, exit_actions, properties) - Convert action_config from dict to tuple[tuple[str, ActionConfigValue], ...] in model_fsm_transition_action.py - Convert ModelFSMTransitionResult from plain class to frozen Pydantic model with tuple collections - Add field validators to auto-convert incoming lists/dicts to tuples - Remove redundant validate_assignment=True (frozen=True is sufficient) ## Type Fixes - Add missing requires_atomic_execution and supports_rollback fields to ModelFSMOperation - Update ModelReducerOutput.intents from list[ModelIntent] to tuple[ModelIntent, ...] - Update fsm_executor.py to use tuple conversions - Replace dict[str, Any] with typed alternatives in validators ## Test Updates - Update test_model_fsm_state.py assertions for tuple types - Update test_model_fsm_transition_action.py for tuple-based action_config All 309 FSM tests pass, mypy clean (0 errors in 2102 files) Note: Pre-existing single-class-per-file violations in configuration models are not addressed in this PR (separate cleanup task).
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
src/omnibase_core/models/configuration/model_throttling_behavior.py (1)
1-1: Group pydantic imports together.Both
FieldandBaseModelare imported frompydanticbut are split across lines 1 and 10. Grouping them at the top improves code organization.Apply this change to group the imports:
-from pydantic import Field +from pydantic import BaseModel, Field """ ModelThrottlingBehavior - Throttling behavior configuration for rate limiting Throttling behavior model for defining how to handle requests when rate limits are exceeded, including blocking, queuing, delay, and custom responses. """ -from pydantic import BaseModel -Also applies to: 10-10
src/omnibase_core/models/fsm/model_fsm_state.py (3)
56-70: PreferField(...)overField(default=...)for required fields.
It’s clearer/idiomatic in Pydantic v2 and avoids any ambiguity around required-ness.- name: str = Field(default=..., description="State name") + name: str = Field(..., description="State name")
72-101: Tighten validator typing (and consider avoidingisinstancehere if the repo enforces it strictly).
Inmode="before", returningobjectis typically a better fit (you’re transforming inputs, not guaranteeing the final field type yet). Also, if the “noisinstance” guideline is meant literally acrosssrc/omnibase_core/**, consider duck-typing for the mapping case. As per coding guidelines, avoidisinstancechecks.-from typing import Any +from collections.abc import Mapping @@ - def _convert_entry_actions_to_tuple( - cls, v: list[str] | tuple[str, ...] | Any - ) -> tuple[str, ...]: + def _convert_entry_actions_to_tuple( + cls, v: object + ) -> object: @@ - return v + return v @@ - def _convert_exit_actions_to_tuple( - cls, v: list[str] | tuple[str, ...] | Any - ) -> tuple[str, ...]: + def _convert_exit_actions_to_tuple( + cls, v: object + ) -> object: @@ - return v + return v @@ - def _convert_properties_to_frozen( - cls, v: dict[str, str] | tuple[tuple[str, str], ...] | Any - ) -> tuple[tuple[str, str], ...]: + def _convert_properties_to_frozen( + cls, v: object + ) -> object: @@ - if isinstance(v, dict): - return tuple(v.items()) + if isinstance(v, Mapping): + return tuple(v.items()) return v
92-101: Consider canonicalizing + validatingpropertieskeys (uniqueness/determinism).
With tuple-of-pairs, duplicates are representable (when input is already a tuple), and ordering can vary across producers; for contract models, it’s often worth enforcing unique keys (and optionally a canonical order) at the boundary.src/omnibase_core/models/fsm/model_fsm_transition_action.py (1)
29-29: Remove unused import.The
Anytype is imported but not used anywhere in the code.Apply this diff to remove the unused import:
-from typing import Any - from pydantic import BaseModel, ConfigDict, Field, field_validator
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
src/omnibase_core/mixins/mixin_node_executor.py(1 hunks)src/omnibase_core/mixins/mixin_workflow_execution.py(1 hunks)src/omnibase_core/models/common/model_dict_value_union.py(1 hunks)src/omnibase_core/models/common/model_discriminated_value.py(1 hunks)src/omnibase_core/models/common/model_value_union.py(1 hunks)src/omnibase_core/models/configuration/model_git_hub_issue_comment_event.py(1 hunks)src/omnibase_core/models/configuration/model_log_filter.py(2 hunks)src/omnibase_core/models/configuration/model_per_user_limits.py(1 hunks)src/omnibase_core/models/configuration/model_priority_metadata.py(1 hunks)src/omnibase_core/models/configuration/model_throttling_behavior.py(2 hunks)src/omnibase_core/models/core/model_tool_integration.py(1 hunks)src/omnibase_core/models/core/model_yaml_configuration.py(1 hunks)src/omnibase_core/models/fsm/model_fsm_operation.py(1 hunks)src/omnibase_core/models/fsm/model_fsm_state.py(3 hunks)src/omnibase_core/models/fsm/model_fsm_transition_action.py(1 hunks)src/omnibase_core/models/fsm/model_fsm_transition_result.py(1 hunks)
✅ Files skipped from review due to trivial changes (8)
- src/omnibase_core/models/configuration/model_log_filter.py
- src/omnibase_core/models/configuration/model_priority_metadata.py
- src/omnibase_core/models/configuration/model_per_user_limits.py
- src/omnibase_core/models/configuration/model_git_hub_issue_comment_event.py
- src/omnibase_core/models/common/model_value_union.py
- src/omnibase_core/models/common/model_dict_value_union.py
- src/omnibase_core/mixins/mixin_workflow_execution.py
- src/omnibase_core/models/core/model_yaml_configuration.py
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{py,txt}
📄 CodeRabbit inference engine (CLAUDE.md)
Always use Poetry for Python package management and task execution. Use
poetry runprefix for all commands (pytest, mypy, python, etc.) - never use pip or python directly
Files:
src/omnibase_core/mixins/mixin_node_executor.pysrc/omnibase_core/models/common/model_discriminated_value.pysrc/omnibase_core/models/core/model_tool_integration.pysrc/omnibase_core/models/configuration/model_throttling_behavior.pysrc/omnibase_core/models/fsm/model_fsm_transition_result.pysrc/omnibase_core/models/fsm/model_fsm_state.pysrc/omnibase_core/models/fsm/model_fsm_transition_action.pysrc/omnibase_core/models/fsm/model_fsm_operation.py
src/omnibase_core/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/omnibase_core/**/*.py: Use protocol names for dependency injection, never concrete class names. Example:container.get_service('ProtocolEventBus')not concrete class names
Always use ModelOnexError for error handling, never generic Exception. Include error_code from EnumCoreErrorCode and context information
Use @standard_error_handling decorator to eliminate 6+ lines of try/catch boilerplate in async operations
Enforce 100% strict mypy compliance - all functions must have type annotations withdisallow_untyped_defs = true
Use EnumNodeKind for architectural classification (EFFECT, COMPUTE, REDUCER, ORCHESTRATOR, RUNTIME_HOST) in workflow routing
Use EnumNodeType for specific implementation type discovery and capability matching (TRANSFORMER, AGGREGATOR, COMPUTE_GENERIC, etc.)
Format code using Black and sort imports using isort - runpoetry run black src/ tests/andpoetry run isort src/ tests/before commit
Run all pre-commit hooks before pushing:pre-commit run --all-files- includes mypy strict, black, isort, ruff
Use duck typing with protocols - never use isinstance checks. Protocols use structural subtyping, not explicit inheritance
Process inter-service communication using ModelEventEnvelope pattern for event-driven architecture
Use thread-local storage or separate node instances for concurrent thread access - ModelComputeCache, ModelCircuitBreaker, NodeCompute/NodeEffect are NOT thread-safe
Files:
src/omnibase_core/mixins/mixin_node_executor.pysrc/omnibase_core/models/common/model_discriminated_value.pysrc/omnibase_core/models/core/model_tool_integration.pysrc/omnibase_core/models/configuration/model_throttling_behavior.pysrc/omnibase_core/models/fsm/model_fsm_transition_result.pysrc/omnibase_core/models/fsm/model_fsm_state.pysrc/omnibase_core/models/fsm/model_fsm_transition_action.pysrc/omnibase_core/models/fsm/model_fsm_operation.py
src/omnibase_core/models/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use Pydantic models for all data structures with v2.11+ compatibility, never use plain dataclasses or Python dicts for domain models
Files:
src/omnibase_core/models/common/model_discriminated_value.pysrc/omnibase_core/models/core/model_tool_integration.pysrc/omnibase_core/models/configuration/model_throttling_behavior.pysrc/omnibase_core/models/fsm/model_fsm_transition_result.pysrc/omnibase_core/models/fsm/model_fsm_state.pysrc/omnibase_core/models/fsm/model_fsm_transition_action.pysrc/omnibase_core/models/fsm/model_fsm_operation.py
🧠 Learnings (3)
📓 Common learnings
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-11T21:05:41.997Z
Learning: Applies to src/omnibase_core/nodes/node_reducer.py : Implement REDUCER nodes using NodeReducer base class with FSM-driven state transitions using ModelIntent pattern
📚 Learning: 2025-12-11T21:05:41.997Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-11T21:05:41.997Z
Learning: Applies to src/omnibase_core/nodes/**/*.py : Import node types directly from omnibase_core.nodes: NodeCompute, NodeEffect, NodeReducer, NodeOrchestrator (v0.4.0+)
Applied to files:
src/omnibase_core/mixins/mixin_node_executor.py
📚 Learning: 2025-12-11T21:05:41.997Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-11T21:05:41.997Z
Learning: Applies to src/omnibase_core/models/**/*.py : Use Pydantic models for all data structures with v2.11+ compatibility, never use plain dataclasses or Python dicts for domain models
Applied to files:
src/omnibase_core/models/common/model_discriminated_value.py
🧬 Code graph analysis (4)
src/omnibase_core/models/fsm/model_fsm_transition_result.py (4)
src/omnibase_core/models/reducer/model_intent.py (1)
ModelIntent(76-140)src/omnibase_core/models/fsm/model_fsm_operation.py (2)
serialize(127-133)validate_instance(135-167)src/omnibase_core/models/fsm/model_fsm_state.py (2)
serialize(120-122)validate_instance(124-136)src/omnibase_core/models/fsm/model_fsm_transition_action.py (2)
serialize(184-190)validate_instance(192-222)
src/omnibase_core/models/fsm/model_fsm_state.py (5)
src/omnibase_core/models/fsm/model_fsm_operation.py (1)
execute(98-125)src/omnibase_core/models/fsm/model_fsm_transition_action.py (1)
execute(160-182)src/omnibase_core/models/fsm/model_fsm_data_primary.py (1)
execute(169-177)src/omnibase_core/models/fsm/model_fsm_transition.py (1)
execute(47-55)src/omnibase_core/models/fsm/model_fsm_transition_condition.py (1)
execute(125-142)
src/omnibase_core/models/fsm/model_fsm_transition_action.py (2)
src/omnibase_core/models/fsm/model_fsm_state.py (3)
execute(110-118)serialize(120-122)validate_instance(124-136)src/omnibase_core/models/fsm/model_fsm_transition_result.py (2)
serialize(125-131)validate_instance(133-152)
src/omnibase_core/models/fsm/model_fsm_operation.py (5)
src/omnibase_core/models/fsm/model_fsm_state.py (3)
execute(110-118)serialize(120-122)validate_instance(124-136)src/omnibase_core/models/fsm/model_fsm_transition_action.py (3)
execute(160-182)serialize(184-190)validate_instance(192-222)src/omnibase_core/models/fsm/model_fsm_transition.py (3)
execute(47-55)serialize(57-59)validate_instance(61-81)src/omnibase_core/models/fsm/model_fsm_transition_condition.py (3)
execute(125-142)serialize(144-150)validate_instance(152-213)src/omnibase_core/models/fsm/model_fsm_transition_result.py (2)
serialize(125-131)validate_instance(133-152)
🔇 Additional comments (13)
src/omnibase_core/models/core/model_tool_integration.py (1)
41-43: Formatting-only change is safe. TheField(...)reflow forresourcesdoesn’t alter types, defaults, or runtime behavior.src/omnibase_core/mixins/mixin_node_executor.py (1)
217-226: LGTM: local import formatting is fine; consider a one-line “avoid circular import” note if intentional.
This keeps the runtime dependency local (useful ifomnibase_core.types.typed_dict_mixin_typescan participate in cycles) and is Black-friendly.src/omnibase_core/models/common/model_discriminated_value.py (1)
5-5: LGTM: Cosmetic improvement removing no-op statement.Replacing the
passstatement with a descriptive comment improves code clarity without any functional impact.src/omnibase_core/models/configuration/model_throttling_behavior.py (2)
257-257: LGTM: Clean refactor to dict.fromkeys.The change from a dict comprehension to
dict.fromkeys()is functionally equivalent and provides a cleaner, more explicit way to create a dictionary with all keys set toTrue.
271-271: LGTM: Concise inline conditional.The inline conditional expression is clear and follows Python best practices for simple conditional assignments. The logic correctly returns
custom_response_bodyonly whencustom_response_enabledisTrue, otherwiseNone.src/omnibase_core/models/fsm/model_fsm_state.py (3)
3-14: Docstring/contract clarity is solid (deep immutability + access guidance).
The “tuple-of-tuples for properties + dict(...) to access” guidance is clear and should prevent most consumer confusion.Also applies to: 22-54
102-106: Frozen model + no-opexecute()matches the v1.0 pattern used elsewhere.
This is consistent with the other FSM models’ “immutable placeholder execution” approach.Also applies to: 110-118
124-136:validate_instance()name check + explicit__all__export look good.
The non-whitespace guard is a useful invariant beyond Pydantic’s “required string” check.Also applies to: 139-140
src/omnibase_core/models/fsm/model_fsm_operation.py (1)
69-167: LGTM! Well-structured reserved model with proper forward compatibility.The implementation correctly addresses past review feedback:
- Field constraints (
min_length=1) are present on required string fields- Unused
kwargsis explicitly marked with_ = kwargs- Consistent protocol implementations matching other FSM models
- Clear v1.1+ reservation boundaries in documentation
The frozen model with explicit whitespace validation provides robust immutability and input validation.
src/omnibase_core/models/fsm/model_fsm_transition_action.py (1)
103-150: Well-designed deep immutability with convenience conversion.The
action_configfield design effectively achieves deep immutability:
- Uses
tuple[tuple[str, ActionConfigValue], ...]instead of dict- Field validator automatically converts dict inputs to frozen tuples for user convenience
- Converts list values to tuples (line 147) for complete immutability
- Pydantic type validation ensures tuple contents match
ActionConfigValuetypesvalidate_instance()(lines 215-220) provides defensive runtime validationThis approach balances immutability requirements with developer ergonomics.
src/omnibase_core/models/fsm/model_fsm_transition_result.py (3)
125-132:serialize()matches the established pattern in other FSM models.
92-100: [rewritten comment]
[classification tag]
117-121:extra="ignore"is intentional for YAML forward-compatibility. The NodeReducer v1.0 contract specification (CONTRACT_DRIVEN_NODEREDUCER_V1_0.md) explicitly documents: "Allow extra fields from YAML contracts" and designates FSM models to useextra="ignore"to accept fields defined for future versions. This is a deliberate design choice for forward compatibility, not a validation gap. No change needed.
| Deep Immutability: | ||
| This model uses frozen=True for Pydantic immutability, and also uses | ||
| immutable types (tuple instead of list) for deep immutability. This | ||
| ensures that nested collections cannot be modified after construction. | ||
| """ |
There was a problem hiding this comment.
Make timestamp timezone-aware (UTC) to avoid naive/local-time ambiguity.
datetime.now().isoformat() is local-time + naive; results will vary across hosts and are hard to compare reliably.
-from datetime import datetime
+from datetime import datetime, timezone
@@
- timestamp: str = Field(
- default_factory=lambda: datetime.now().isoformat(),
+ timestamp: str = Field(
+ default_factory=lambda: datetime.now(timezone.utc).isoformat(),
description="ISO-format timestamp of result creation",
)Also applies to: 87-90
🤖 Prompt for AI Agents
In src/omnibase_core/models/fsm/model_fsm_transition_result.py around lines 7-11
(and also apply the same change at lines 87-90), the timestamp is generated as a
naive local datetime via datetime.now().isoformat(); change it to a
timezone-aware UTC timestamp by using datetime.now(tz=timezone.utc).isoformat()
(or datetime.utcnow().replace(tzinfo=timezone.utc).isoformat()) and add the
necessary import from datetime (timezone) so the stored ISO string is explicitly
UTC for reliable cross-host comparisons.
| new_state: str = Field( | ||
| default=..., | ||
| description="Resulting state name after transition", | ||
| ) | ||
| old_state: str = Field( | ||
| default=..., | ||
| description="Previous state name before transition", | ||
| ) |
There was a problem hiding this comment.
Enforce basic string constraints at the Pydantic layer + align validate_instance() with its own docstring.
Right now empty strings are only caught by validate_instance(), and success/error consistency isn’t validated.
new_state: str = Field(
default=...,
description="Resulting state name after transition",
+ min_length=1,
)
@@
old_state: str = Field(
default=...,
description="Previous state name before transition",
+ min_length=1,
)
@@
def validate_instance(self) -> bool:
@@
# Validate old_state is non-empty
if not self.old_state or not self.old_state.strip():
return False
+
+ # Enforce basic success/error consistency
+ if self.success and self.error:
+ return False
+ if (not self.success) and (not self.error or not self.error.strip()):
+ return False
@@
return TrueAlso applies to: 133-152
🤖 Prompt for AI Agents
In src/omnibase_core/models/fsm/model_fsm_transition_result.py around lines
63-70 and also lines 133-152, the model currently allows empty strings and
validate_instance() does not enforce the success/error consistency described in
its docstring; update the Field definitions for any string fields (including
new_state and old_state at 63-70 and the analogous fields in 133-152) to include
basic Pydantic constraints (e.g., min_length=1) so empty strings are rejected at
model validation time, and modify validate_instance() to enforce its docstring
rules: if success is True then error must be None/absent, if success is False
then error must be a non-empty string, and ensure any remaining instance-level
checks align with the new Field constraints (remove redundant empty-string
checks or make them consistent).
…models-omn-578-581
Code Review: FSM Contract Models for NodeReducer v1.0SummaryThis PR adds four new MVP FSM contract models for the NodeReducer v1.0 contract-driven architecture. The implementation is comprehensive, well-designed, and follows ONEX best practices. The PR demonstrates strong attention to type safety, immutability, and test coverage. ✅ Strengths1. Excellent Type Safety
2. Deep Immutability Implementation
Example of good immutability pattern: # In mixin_fsm_execution.py
previous_history = self._fsm_state.history if self._fsm_state else ()
new_history = previous_history + (result.old_state,) # Tuple concatenation
self._fsm_state = FSMState(
current_state=result.new_state,
context=context,
history=new_history, # Immutable tuple
)3. Comprehensive Test Coverage
4. Protocol-Driven Design
5. Model Organization
6. Error Handling
7. Documentation Quality
🔍 Observations & Minor Suggestions1. Expression Validation (ModelFSMTransitionCondition)The 3-token expression validator works well for simple expressions: tokens = self.expression.split() # "status == active" → ["status", "==", "active"]
if len(tokens) != 3:
raise ModelOnexError(...)Observation: This doesn't handle quoted strings with spaces (e.g., Recommendation: Add a docstring example showing valid expression formats: # Valid: "status == active", "count > 0", "name != empty"
# Invalid (v1.0): "name == 'John Doe'" # spaces in quoted strings2. ModelActionConfigParameter Default Value ValidationThe special-case handling for bool vs int is excellent: # Special case: bool is a subclass of int in Python, so we need explicit check
if self.type == "int" and isinstance(self.default, bool):
raise ModelOnexError(...)Observation: This is correct, but consider adding a test for 3. Immutability Trade-off DocumentationThe # Alternatives considered but not implemented:
# - types.MappingProxyType: Would provide read-only view but loses dict type
# - frozendict (third-party): Would require additional dependency
# - Pydantic model with frozen=True: Would change the class structureRecommendation: Consider linking to this decision in the architecture docs for future reference. 4. Unused Parameter PatternGood use of explicit unused parameter handling: def execute(self, **kwargs: object) -> bool:
_ = kwargs # Explicitly mark as unused for v1.0
return TrueAlternative (purely stylistic): You could use 🛡️ Security & Performance Considerations✅ No Security Concerns
✅ Performance
📊 Test Coverage AnalysisTest Distribution:
Total New Test Lines: ~3,663 lines for 4 models (~900 lines/model average) This is exceptionally thorough coverage. The tests follow a clear structure:
Recommendation: Ensure CI passes all 214 new tests. Based on the CLAUDE.md benchmark (2m30s-3m30s per split), this should add approximately 15-30 seconds to one split. 🎯 Code Quality Checklist
🚀 Final RecommendationAPPROVE - This PR is ready to merge pending CI checks. The implementation demonstrates:
Pre-Merge Checklist:
Great work on this implementation! The attention to immutability and type safety is particularly impressive. 🎉 Reviewed By: Claude Code PR Review Agent |
- Auto-fix I001 (import block unsorted) errors across 80 files - Split typed_dict_mixin_types.py into 21 separate TypedDict files in new types/typed_dict/ package for ONEX compliance - Split model_node_config_value.py by extracting ModelNodeConfigEntry to model_node_config_entry.py - Re-export types from original modules for API consistency - Update test to use ModelNodeConfigSchema model Resolves CI Code Quality check failures on PR #165.
Code Review - PR #165: FSM Contract Models for NodeReducer v1.0SummaryThis PR introduces four new MVP FSM contract models (OMN-578, OMN-579, OMN-580, OMN-581) for the NodeReducer v1.0 contract-driven architecture. The changes include comprehensive models for FSM transition conditions, actions, operations, and typed action config parameters. Overall, this is a well-structured, high-quality PR with excellent test coverage and adherence to ONEX patterns. ✅ Strengths1. Exceptional Code Quality & Architecture
2. Excellent Test Coverage
3. Documentation & Clarity
4. ONEX Pattern Adherence
🔍 Issues & Concerns1. CRITICAL: History Immutability Pattern ChangeLocation: # BEFORE (mutable list)
history = (self._fsm_state.history if self._fsm_state else []) + [result.old_state]
# AFTER (immutable tuple)
previous_history = self._fsm_state.history if self._fsm_state else ()
new_history = previous_history + (result.old_state,)Concern: While immutability is excellent for thread safety, this is a breaking change to the Questions:
Recommendation: ✅ APPROVE if pre-v0.4.0 (breaking changes allowed), otherwise document migration in 2. MODERATE: Import Ordering ConsistencyLocation: Multiple CLI model files ( Changes reorder imports to put # BEFORE
from omnibase_core.types.type_serializable_value import SerializedDict
from pydantic import BaseModel
# AFTER
from pydantic import BaseModel
from omnibase_core.types.type_serializable_value import SerializedDictIssue: ONEX doesn't seem to have a documented import ordering convention. This is cosmetic but may cause merge conflicts if inconsistent. Recommendation: Run 3. MINOR: ModelActionConfigParameter Type Validation Edge CaseLocation: The bool/int special case handling is correct but duplicates validation logic: # Special case: bool is a subclass of int in Python
if self.type == "int" and isinstance(self.default, bool):
raise ModelOnexError(...) # Duplicate error raising logicSuggestion: Extract error raising into a helper method to DRY: def _raise_type_mismatch(self, actual_type: str) -> None:
"""Helper to raise type mismatch error."""
raise ModelOnexError(
message=f"Default value for parameter '{self.name}' has type '{actual_type}', expected type '{self.type}'",
error_code=EnumCoreErrorCode.PARAMETER_TYPE_MISMATCH,
details=ModelErrorContext.with_context({...})
)
# Then use:
if self.type == "int" and isinstance(self.default, bool):
self._raise_type_mismatch("bool")Impact: Low - current code works fine, just slightly verbose. 4. MINOR: Reserved Field Documentation ConsistencyLocation: Multiple model docstrings Reserved fields (
Recommendation: Standardize format across all three models for consistency. 🎯 Performance Considerations✅ No performance concerns. Changes are model definitions with minimal runtime overhead:
🔒 Security Considerations✅ No security concerns:
🧪 Test Coverage AssessmentExcellent coverage based on review of test files:
Recommendation: ✅ Tests meet ONEX 60% coverage requirement (likely exceeds it for these modules) 📋 Checklist Against CLAUDE.md
🚀 Final RecommendationAPPROVE WITH MINOR SUGGESTIONS ✅ This is high-quality work that follows ONEX patterns excellently. The minor suggestions are optional improvements and should not block merging. Required Actions (Before Merge):
Optional Improvements (Future PRs):
💡 Excellent Patterns to HighlightFor other developers reviewing this PR:
Great work! 🎉 Reviewed by: Claude Sonnet 4.5 (claude-code) |
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (9)
src/omnibase_core/models/operations/model_workflow_instance_metadata.py (3)
45-52: Use timezone-aware timestamps forcreated_at/updated_at
Naivedatetime.now()is ambiguous and can cause subtle ordering/serialization bugs across services.+from datetime import datetime, timezone @@ - created_at: datetime = Field( - default_factory=datetime.now, + created_at: datetime = Field( + default_factory=lambda: datetime.now(timezone.utc), description="Creation timestamp", ) updated_at: datetime = Field( - default_factory=datetime.now, + default_factory=lambda: datetime.now(timezone.utc), description="Last update timestamp", )
89-110:get_id()never returnsworkflow_id/instance_id(likely always throws)
This model definesworkflow_idandinstance_id, but the lookup list omits both, soget_id()will raise even for valid instances.@@ def get_id(self) -> str: @@ for field in [ + "instance_id", + "workflow_id", "id", "uuid", "identifier", "node_id", "execution_id", "metadata_id", ]:
78-88: Don’t swallow exceptions; this hides validation failures and violates error-handling guidance
Withvalidate_assignment=True,setattr()can raise (e.g., Pydantic validation errors). CatchingExceptionand returningFalsedrops context and makes failures hard to debug. Consider catching specific exceptions and raisingModelOnexErrorwith context (or returningFalsebut at least capturing details).@@ -from pydantic import BaseModel +from pydantic import BaseModel, ValidationError @@ def execute(self, **kwargs: object) -> bool: @@ - except Exception: # fallback-ok: Protocol method - graceful fallback for optional implementation - return False + except (ValidationError, TypeError, AttributeError) as e: + raise ModelOnexError( + message="Failed to execute workflow metadata update", + error_code=EnumCoreErrorCode.VALIDATION_ERROR, + context={"kwargs": kwargs, "error": str(e)}, + ) from e @@ def validate_instance(self) -> bool: @@ - except Exception: # fallback-ok: Protocol method - graceful fallback for optional implementation - return False + except (ValidationError, TypeError, AttributeError) as e: + raise ModelOnexError( + message="Workflow instance metadata validation failed", + error_code=EnumCoreErrorCode.VALIDATION_ERROR, + context={"error": str(e)}, + ) from eAs per coding guidelines, avoid generic
Exceptionfor error handling.Also applies to: 115-123
src/omnibase_core/models/connections/model_connection_metrics.py (1)
71-83:configure()can overwrite BaseModel methods; restrict keys + avoidexcept Exception.
hasattr(self, key)is too permissive (e.g.,model_dump), andexcept Exceptionviolates repo guidance + can hide real bugs.from __future__ import annotations +from pydantic import ValidationError from pydantic import BaseModel, Field from omnibase_core.enums.enum_core_error_code import EnumCoreErrorCode from omnibase_core.models.errors.model_onex_error import ModelOnexError from omnibase_core.types import SerializedDict @@ def configure(self, **kwargs: object) -> bool: """Configure instance with provided parameters (Configurable protocol).""" try: for key, value in kwargs.items(): - if hasattr(self, key): - setattr(self, key, value) + if key in self.model_fields: + setattr(self, key, value) + else: + raise ModelOnexError( + message=f"Unknown configuration key: {key}", + error_code=EnumCoreErrorCode.VALIDATION_ERROR, + context={"key": key}, + ) return True - except Exception as e: + except (ValidationError, TypeError, AttributeError) as e: raise ModelOnexError( message=f"Operation failed: {e}", error_code=EnumCoreErrorCode.VALIDATION_ERROR, ) from esrc/omnibase_core/models/operations/model_computation_input_base.py (1)
1-12: Run isort to fix import organization and move module docstring.The imports don't follow PEP 8 grouping conventions:
- The module docstring (line 5) interrupts the import block and should appear at the top after
from __future__imports- Imports should be grouped: standard library (uuid), third-party (pydantic), then local (omnibase_core)
- Pydantic imports are split across lines 3 and 8 instead of being consolidated
As per coding guidelines, run
poetry run isort src/andpoetry run black src/before committing.Apply this diff to fix the import organization:
from __future__ import annotations + +""" +Base computation input model for discriminated union. + +Provides common interface for all computation input types. +Follows ONEX strong typing principles and one-model-per-file architecture. +""" + +from uuid import UUID + +from pydantic import BaseModel, Field + +from omnibase_core.enums.enum_computation_type import EnumComputationType +from omnibase_core.enums.enum_input_data_type import EnumInputDataType +from omnibase_core.models.common.model_schema_value import ModelSchemaValue - -from pydantic import Field - -"\nBase computation input model for discriminated union.\n\nProvides common interface for all computation input types.\nFollows ONEX strong typing principles and one-model-per-file architecture.\n" -from uuid import UUID - -from pydantic import BaseModel - -from omnibase_core.enums.enum_computation_type import EnumComputationType -from omnibase_core.enums.enum_input_data_type import EnumInputDataType -from omnibase_core.models.common.model_schema_value import ModelSchemaValuesrc/omnibase_core/models/infrastructure/model_result_dict.py (1)
51-77: Avoidexcept Exception+ constrainexecute/configureto real model fields.
CatchingExceptionviolates the “never generic Exception” guidance andhasattr/setattrcan mutate non-field attributes (or BaseModel internals). Prefer usingself.model_fields(Pydantic v2) and catching specific exceptions (e.g.,pydantic.ValidationError,AttributeError,TypeError), while enrichingModelOnexErrorcontext. Based on learnings / coding guidelines, useModelOnexErrorwithout genericException.@@ -from pydantic import BaseModel +from pydantic import BaseModel, ValidationError @@ class ModelResultDict(BaseModel): @@ def execute(self, **kwargs: object) -> bool: """Execute or update execution status (Executable protocol).""" try: - # Update any relevant execution fields - for key, value in kwargs.items(): - if hasattr(self, key): - setattr(self, key, value) + for key, value in kwargs.items(): + if key in self.model_fields: + setattr(self, key, value) return True - except Exception as e: + except (ValidationError, AttributeError, TypeError, ValueError) as e: raise ModelOnexError( error_code=EnumCoreErrorCode.VALIDATION_ERROR, - message=f"Operation failed: {e}", + message=f"ModelResultDict.execute failed: {e}", + context={"kwargs_keys": list(kwargs.keys())}, ) from e @@ def configure(self, **kwargs: object) -> bool: """Configure instance with provided parameters (Configurable protocol).""" try: for key, value in kwargs.items(): - if hasattr(self, key): - setattr(self, key, value) + if key in self.model_fields: + setattr(self, key, value) return True - except Exception as e: + except (ValidationError, AttributeError, TypeError, ValueError) as e: raise ModelOnexError( error_code=EnumCoreErrorCode.VALIDATION_ERROR, - message=f"Operation failed: {e}", + message=f"ModelResultDict.configure failed: {e}", + context={"kwargs_keys": list(kwargs.keys())}, ) from esrc/omnibase_core/models/operations/model_operation_payload.py (1)
94-114: Logic bug: get_id() doesn't check for the actual ID field.The method checks for
id,uuid,identifier,node_id,execution_id, andmetadata_id, but doesn't check foroperation_id, which is the actual UUID field defined at line 53. This causes the method to always raiseModelOnexErrordespite the instance having a valid identifier.Apply this diff to fix the issue:
def get_id(self) -> str: """Get unique identifier (Identifiable protocol).""" # Try common ID field patterns for field in [ + "operation_id", "id", "uuid", "identifier", "node_id", "execution_id", "metadata_id", ]:src/omnibase_core/models/operations/model_event_metadata.py (2)
103-121:get_id()never returns the model’s actual ID (event_id) → always raises.
ModelEventMetadatadefinesevent_id, butget_id()only checks"id","uuid", etc. This will raiseModelOnexErrorfor normal instances.Suggested fix:
def get_id(self) -> str: """Get unique identifier (Identifiable protocol).""" + # Primary ID for this model + return str(self.event_id) - # Try common ID field patterns - for field in [ - "id", - "uuid", - "identifier", - "node_id", - "execution_id", - "metadata_id", - ]: - if hasattr(self, field): - value = getattr(self, field) - if value is not None: - return str(value) - raise ModelOnexError( - message=f"{self.__class__.__name__} must have a valid ID field (type_id, id, uuid, identifier, etc.). Cannot generate stable ID without UUID field.", - error_code=EnumCoreErrorCode.VALIDATION_ERROR, - )
92-102: Restrict field updates to declared model fields and use ModelOnexError instead of swallowing exceptions.The
execute()method currently allows mutation of any attribute viahasattr()and silently suppresses errors by returningFalse. This violates the coding guideline requiringModelOnexErrorfor all error handling and is less secure than restricting updates to declared Pydantic fields.With
validate_assignment=Truealready configured, restrict updates toself.__class__.model_fieldsand propagate errors viaModelOnexErrorwithEnumCoreErrorCode.OPERATION_FAILEDcontext (similar to theget_id()method in the same file):def execute(self, **kwargs: object) -> bool: """Execute or update execution status (Executable protocol).""" try: allowed = self.__class__.model_fields for key, value in kwargs.items(): if key in allowed: setattr(self, key, value) return True except ModelOnexError: raise except Exception as e: raise ModelOnexError( message="Failed to execute ModelEventMetadata update.", error_code=EnumCoreErrorCode.OPERATION_FAILED, context={"kwargs_keys": list(kwargs.keys()), "cause": str(e)}, ) from e
🧹 Nitpick comments (15)
src/omnibase_core/models/cli/model_result_summary.py (1)
1-21: Import reorder is safe; consider normalizing module docstring/import layout for tooling + strict typing.
TheSerializedDictimport move (Line 20) is functionally no-op. However, the string at Lines 7-12 won’t be treated as the module docstring since imports precede it—consider moving it to the top and grouping all imports together. Also, for Pydantic v2 + strict mypy, consider usingpydantic.ConfigDictformodel_configto keep types tighter.src/omnibase_core/models/examples/model_fallback_strategy.py (1)
19-23: Import reorder is fine; consider consolidating Pydantic imports and moving module docstring to the top.
ReorderingSerializedDictwon’t change behavior, but since you’re editing imports: it’d be cleaner to combinefrom pydantic import BaseModel, Fieldand (optionally) place the module docstring immediately after the__future__import to align with standard tooling expectations (isort/doc parsers).src/omnibase_core/models/examples/model_fallback_metadata.py (1)
83-93: Consider simplifying the unnecessary exception handling.The try-except block wraps only
return True, which cannot raise an exception. The error handling code is unreachable and adds unnecessary complexity.Apply this diff to simplify:
def validate_instance(self) -> bool: """Validate instance integrity (ProtocolValidatable protocol).""" - try: - # Basic validation - ensure required fields exist - # Override in specific models for custom validation - return True - except Exception as e: - raise ModelOnexError( - error_code=EnumCoreErrorCode.VALIDATION_ERROR, - message=f"Operation failed: {e}", - ) from e + # Basic validation - ensure required fields exist + # Override in specific models for custom validation + return Truesrc/omnibase_core/models/connections/model_connection_security.py (1)
19-19: Import reordering has no functional impact.The repositioning of the
SerializedDictimport is harmless and does not affect runtime behavior or the public API.As a nitpick, the overall import order in this file could benefit from running
isortto align with PEP 8 conventions (stdlib → third-party → first-party). According to the coding guidelines,poetry run isort src/ tests/should be run before commit to ensure consistent import organization across the codebase.src/omnibase_core/models/cli/model_cli_execution_summary.py (1)
25-25: Consider running isort to standardize import organization.The import relocation is functionally correct, but the file has non-standard import organization with a module docstring placed between import blocks (lines 7-12). Running isort will group and order all imports according to project conventions.
poetry run isort src/omnibase_core/models/cli/model_cli_execution_summary.pyAs per coding guidelines: "Format code using Black and sort imports using isort - run
poetry run black src/ tests/andpoetry run isort src/ tests/before commit"src/omnibase_core/models/examples/model_typed_property.py (1)
20-20: LGTM! Import repositioning aligns with PR objectives.The import is correctly positioned to group local
omnibase_coreimports together. This change has no functional impact and aligns with the PR's goal of normalizing import ordering across FSM and configuration models.As per coding guidelines, ensure
poetry run isort src/ tests/has been run before committing to maintain consistent import ordering across the codebase.src/omnibase_core/models/examples/model_examples_collection_summary.py (1)
1-23: Reorganize imports and verify duplicate import claim.The import organization is non-standard with a docstring block (lines 7-12) interrupting the import statements. All imports should be grouped together at the top of the file before module-level docstrings or code.
Additionally, the AI summary mentions a duplicate
SerializedDictimport, but I only see one import on line 20. Please verify whether a duplicate exists.Apply this refactor to reorganize imports:
from __future__ import annotations +from datetime import datetime from pydantic import Field, BaseModel from omnibase_core.models.errors.model_onex_error import ModelOnexError - -""" -Examples collection summary model. - -Clean, strongly-typed replacement for the horrible union return type. -Follows ONEX one-model-per-file naming conventions. -""" - -from datetime import datetime - -from pydantic import BaseModel - from omnibase_core.enums.enum_core_error_code import EnumCoreErrorCode from omnibase_core.enums.enum_data_format import EnumDataFormat from omnibase_core.types.type_serializable_value import SerializedDict from .model_example_metadata_summary import ModelExampleMetadataSummary from .model_example_summary import ModelExampleSummary -# Removed Any import for ONEX compliance +""" +Examples collection summary model. + +Clean, strongly-typed replacement for the horrible union return type. +Follows ONEX one-model-per-file naming conventions. +"""Then run the following to ensure proper import formatting as per coding guidelines:
#!/bin/bash # Verify import organization with isort poetry run isort --check-only --diff src/omnibase_core/models/examples/model_examples_collection_summary.pysrc/omnibase_core/models/examples/model_example_summary.py (2)
61-76: Consider using Pydantic validation in configure method.The current implementation uses
setattrdirectly, which bypasses Pydantic's field validators even thoughvalidate_assignment=Trueis set in the model config. For example, callingconfigure(is_valid="not a bool")would not raise a validation error.Consider using Pydantic's validation:
def configure(self, **kwargs: object) -> bool: """Configure instance with provided parameters (Configurable protocol). Raises: ModelOnexError: If configuration fails with details about the failure """ try: - for key, value in kwargs.items(): - if hasattr(self, key): - setattr(self, key, value) + # Use Pydantic's validation by creating a new instance with updated values + updated_data = {**self.model_dump(), **kwargs} + validated = self.__class__.model_validate(updated_data) + for key, value in validated.model_dump().items(): + if hasattr(self, key): + object.__setattr__(self, key, value) return True except Exception as e:
82-96: Consider adding validation logic or clarifying the placeholder intent.The method currently returns
Truewithout performing any validation, despite the comment mentioning "ensure required fields exist". Since this is an example model, consider either:
- Adding a basic validation example (e.g., checking that required fields are not None)
- Updating the comment to explicitly state this is a no-op placeholder meant to be overridden
This would make the example clearer for developers using it as a reference.
src/omnibase_core/models/operations/model_operation_payload.py (1)
1-37: Reorganize imports to follow PEP 8 conventions.Imports are currently split before (lines 3-7) and after (lines 17-37) the module docstring. Standard Python convention and isort expect all imports to come after the module docstring, not split around it.
Recommended structure:
from __future__imports- Module docstring
- Standard library imports
- Third-party imports (e.g., pydantic)
- Local imports (omnibase_core)
Run
poetry run isort src/omnibase_core/models/operations/model_operation_payload.pyto automatically fix the import order, as per coding guidelines.#!/bin/bash # Verify import order compliance poetry run isort --check-only --diff src/omnibase_core/models/operations/model_operation_payload.pytests/unit/infrastructure/test_node_config_provider.py (1)
20-23: Prefer importingModelNodeConfigSchemafrom the public package export.
This reduces test brittleness when modules move.-from omnibase_core.models.configuration.model_node_config_value import ( - ModelNodeConfigSchema, -) +from omnibase_core.models.configuration import ModelNodeConfigSchemasrc/omnibase_core/models/configuration/model_node_config_value.py (1)
13-30: Consider aligningtypevsvalue_typenaming across config models.
Not blocking, but havingModelNodeConfigEntry.value_typeandModelNodeConfigSchema.typemakes generic handling awkward. A low-impact option is adding a read-only alias property (e.g.,value_type) toModelNodeConfigSchemawhile keeping the serialized field name astype.src/omnibase_core/models/operations/model_event_metadata.py (1)
8-22: Optional cleanup: module docstring position, import grouping, and timestamp timezone.
- The triple-quoted block at Line 8 won’t be treated as the module docstring because imports precede it.
- Import blocks look non-isort (stdlib/third-party/local).
timestampusesdatetime.now(local/naive). For event metadata, UTC-aware timestamps are usually safer.Also applies to: 50-53
src/omnibase_core/types/typed_dict/typed_dict_serialized_result.py (1)
1-14: LGTM! Simple TypedDict for serialized results.The minimal structure with
result: objectprovides maximum flexibility for serialized execution results. If serialization produces predictable types, consider tightening this in future iterations.src/omnibase_core/types/typed_dict_mixin_types.py (1)
9-31: Consider aligning import order with__all__for consistency.The imports have a slightly different order than
__all__(e.g.,TypedDictFilterCriteriaandTypedDictFSMContextare swapped). While this doesn't affect functionality, maintaining consistent alphabetical ordering in both locations improves readability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (105)
src/omnibase_core/models/cli/model_cli_command_option.py(1 hunks)src/omnibase_core/models/cli/model_cli_debug_info.py(1 hunks)src/omnibase_core/models/cli/model_cli_execution_config.py(1 hunks)src/omnibase_core/models/cli/model_cli_execution_context.py(1 hunks)src/omnibase_core/models/cli/model_cli_execution_input_data.py(1 hunks)src/omnibase_core/models/cli/model_cli_execution_summary.py(1 hunks)src/omnibase_core/models/cli/model_cli_output_data.py(1 hunks)src/omnibase_core/models/cli/model_cli_result.py(1 hunks)src/omnibase_core/models/cli/model_cli_result_metadata.py(1 hunks)src/omnibase_core/models/cli/model_result_summary.py(1 hunks)src/omnibase_core/models/cli/model_trace_data.py(1 hunks)src/omnibase_core/models/common/__init__.py(1 hunks)src/omnibase_core/models/configuration/__init__.py(1 hunks)src/omnibase_core/models/configuration/model_log_filter_config.py(1 hunks)src/omnibase_core/models/configuration/model_log_formatting.py(1 hunks)src/omnibase_core/models/configuration/model_node_config_entry.py(1 hunks)src/omnibase_core/models/configuration/model_node_config_value.py(1 hunks)src/omnibase_core/models/configuration/model_session_affinity.py(1 hunks)src/omnibase_core/models/connections/model_cloud_service_properties.py(1 hunks)src/omnibase_core/models/connections/model_connection_auth.py(1 hunks)src/omnibase_core/models/connections/model_connection_endpoint.py(1 hunks)src/omnibase_core/models/connections/model_connection_info.py(1 hunks)src/omnibase_core/models/connections/model_connection_metrics.py(1 hunks)src/omnibase_core/models/connections/model_connection_pool.py(1 hunks)src/omnibase_core/models/connections/model_connection_security.py(1 hunks)src/omnibase_core/models/connections/model_custom_connection_properties.py(1 hunks)src/omnibase_core/models/connections/model_database_properties.py(1 hunks)src/omnibase_core/models/connections/model_message_queue_properties.py(1 hunks)src/omnibase_core/models/connections/model_performance_properties.py(1 hunks)src/omnibase_core/models/examples/model_artifact_type_config.py(1 hunks)src/omnibase_core/models/examples/model_data_handling_declaration.py(1 hunks)src/omnibase_core/models/examples/model_environment_properties.py(1 hunks)src/omnibase_core/models/examples/model_environment_properties_collection.py(1 hunks)src/omnibase_core/models/examples/model_example.py(1 hunks)src/omnibase_core/models/examples/model_example_context_data.py(1 hunks)src/omnibase_core/models/examples/model_example_input_data.py(1 hunks)src/omnibase_core/models/examples/model_example_metadata.py(1 hunks)src/omnibase_core/models/examples/model_example_metadata_summary.py(1 hunks)src/omnibase_core/models/examples/model_example_output_data.py(1 hunks)src/omnibase_core/models/examples/model_example_summary.py(1 hunks)src/omnibase_core/models/examples/model_examples_collection_summary.py(1 hunks)src/omnibase_core/models/examples/model_fallback_metadata.py(1 hunks)src/omnibase_core/models/examples/model_fallback_strategy.py(1 hunks)src/omnibase_core/models/examples/model_namespace_config.py(1 hunks)src/omnibase_core/models/examples/model_property_collection.py(1 hunks)src/omnibase_core/models/examples/model_property_metadata.py(1 hunks)src/omnibase_core/models/examples/model_property_value.py(1 hunks)src/omnibase_core/models/examples/model_schema_example.py(1 hunks)src/omnibase_core/models/examples/model_typed_property.py(1 hunks)src/omnibase_core/models/infrastructure/__init__.py(1 hunks)src/omnibase_core/models/infrastructure/model_cli_result_data.py(1 hunks)src/omnibase_core/models/infrastructure/model_duration.py(1 hunks)src/omnibase_core/models/infrastructure/model_environment_variables.py(1 hunks)src/omnibase_core/models/infrastructure/model_error_value.py(1 hunks)src/omnibase_core/models/infrastructure/model_execution_summary.py(1 hunks)src/omnibase_core/models/infrastructure/model_metric.py(1 hunks)src/omnibase_core/models/infrastructure/model_metrics_data.py(1 hunks)src/omnibase_core/models/infrastructure/model_progress.py(1 hunks)src/omnibase_core/models/infrastructure/model_regex_flags.py(1 hunks)src/omnibase_core/models/infrastructure/model_result_dict.py(1 hunks)src/omnibase_core/models/infrastructure/model_retry_execution.py(1 hunks)src/omnibase_core/models/infrastructure/model_retry_failure_info.py(1 hunks)src/omnibase_core/models/infrastructure/model_retry_policy.py(1 hunks)src/omnibase_core/models/infrastructure/model_test_result.py(1 hunks)src/omnibase_core/models/infrastructure/model_test_results.py(1 hunks)src/omnibase_core/models/infrastructure/model_time_based.py(1 hunks)src/omnibase_core/models/infrastructure/model_timeout.py(1 hunks)src/omnibase_core/models/infrastructure/model_timeout_data.py(1 hunks)src/omnibase_core/models/infrastructure/progress/model_progress_core.py(1 hunks)src/omnibase_core/models/infrastructure/progress/model_progress_metrics.py(1 hunks)src/omnibase_core/models/infrastructure/progress/model_progress_milestones.py(1 hunks)src/omnibase_core/models/infrastructure/progress/model_progress_timing.py(1 hunks)src/omnibase_core/models/operations/model_computation_input_base.py(1 hunks)src/omnibase_core/models/operations/model_event_metadata.py(1 hunks)src/omnibase_core/models/operations/model_event_payload.py(1 hunks)src/omnibase_core/models/operations/model_operation_payload.py(1 hunks)src/omnibase_core/models/operations/model_operationparameters.py(1 hunks)src/omnibase_core/models/operations/model_system_metadata.py(1 hunks)src/omnibase_core/models/operations/model_workflow_instance_metadata.py(1 hunks)src/omnibase_core/models/operations/model_workflow_payload.py(1 hunks)src/omnibase_core/models/security/model_permission_scope.py(1 hunks)src/omnibase_core/types/typed_dict/__init__.py(1 hunks)src/omnibase_core/types/typed_dict/typed_dict_cache_stats.py(1 hunks)src/omnibase_core/types/typed_dict/typed_dict_discovery_extended_stats.py(1 hunks)src/omnibase_core/types/typed_dict/typed_dict_discovery_stats.py(1 hunks)src/omnibase_core/types/typed_dict/typed_dict_event_metadata.py(1 hunks)src/omnibase_core/types/typed_dict/typed_dict_executor_health.py(1 hunks)src/omnibase_core/types/typed_dict/typed_dict_filter_criteria.py(1 hunks)src/omnibase_core/types/typed_dict/typed_dict_fsm_context.py(1 hunks)src/omnibase_core/types/typed_dict/typed_dict_health_check_status.py(1 hunks)src/omnibase_core/types/typed_dict/typed_dict_introspection_data.py(1 hunks)src/omnibase_core/types/typed_dict/typed_dict_lazy_cache_stats.py(1 hunks)src/omnibase_core/types/typed_dict/typed_dict_metric_entry.py(1 hunks)src/omnibase_core/types/typed_dict/typed_dict_node_executor_health.py(1 hunks)src/omnibase_core/types/typed_dict/typed_dict_performance_profile.py(1 hunks)src/omnibase_core/types/typed_dict/typed_dict_redacted_data.py(1 hunks)src/omnibase_core/types/typed_dict/typed_dict_reducer_fsm_context.py(1 hunks)src/omnibase_core/types/typed_dict/typed_dict_registry_stats.py(1 hunks)src/omnibase_core/types/typed_dict/typed_dict_serialized_result.py(1 hunks)src/omnibase_core/types/typed_dict/typed_dict_service_health.py(1 hunks)src/omnibase_core/types/typed_dict/typed_dict_tool_execution_response.py(1 hunks)src/omnibase_core/types/typed_dict/typed_dict_tool_execution_result.py(1 hunks)src/omnibase_core/types/typed_dict/typed_dict_workflow_step_config.py(1 hunks)src/omnibase_core/types/typed_dict_mixin_types.py(1 hunks)tests/unit/infrastructure/test_node_config_provider.py(2 hunks)
✅ Files skipped from review due to trivial changes (40)
- src/omnibase_core/models/cli/model_cli_result.py
- src/omnibase_core/models/infrastructure/model_retry_policy.py
- src/omnibase_core/models/configuration/model_session_affinity.py
- src/omnibase_core/models/infrastructure/model_progress.py
- src/omnibase_core/models/cli/model_cli_command_option.py
- src/omnibase_core/models/examples/model_artifact_type_config.py
- src/omnibase_core/models/configuration/model_log_formatting.py
- src/omnibase_core/models/infrastructure/model_execution_summary.py
- src/omnibase_core/models/examples/model_property_collection.py
- src/omnibase_core/models/infrastructure/progress/model_progress_milestones.py
- src/omnibase_core/models/cli/model_cli_output_data.py
- src/omnibase_core/models/infrastructure/model_regex_flags.py
- src/omnibase_core/models/examples/model_schema_example.py
- src/omnibase_core/models/connections/model_connection_endpoint.py
- src/omnibase_core/models/infrastructure/model_timeout_data.py
- src/omnibase_core/models/infrastructure/model_time_based.py
- src/omnibase_core/models/cli/model_cli_execution_input_data.py
- src/omnibase_core/models/connections/model_performance_properties.py
- src/omnibase_core/models/cli/model_trace_data.py
- src/omnibase_core/models/infrastructure/model_timeout.py
- src/omnibase_core/models/infrastructure/model_cli_result_data.py
- src/omnibase_core/models/connections/model_custom_connection_properties.py
- src/omnibase_core/models/cli/model_cli_debug_info.py
- src/omnibase_core/models/operations/model_workflow_payload.py
- src/omnibase_core/models/examples/model_environment_properties_collection.py
- src/omnibase_core/models/operations/model_system_metadata.py
- src/omnibase_core/models/infrastructure/model_environment_variables.py
- src/omnibase_core/models/infrastructure/model_test_result.py
- src/omnibase_core/models/common/init.py
- src/omnibase_core/models/examples/model_property_value.py
- src/omnibase_core/models/examples/model_environment_properties.py
- src/omnibase_core/models/infrastructure/progress/model_progress_timing.py
- src/omnibase_core/models/connections/model_message_queue_properties.py
- src/omnibase_core/models/infrastructure/progress/model_progress_core.py
- src/omnibase_core/models/examples/model_example_context_data.py
- src/omnibase_core/models/operations/model_operationparameters.py
- src/omnibase_core/models/infrastructure/model_error_value.py
- src/omnibase_core/models/infrastructure/model_test_results.py
- src/omnibase_core/models/connections/model_cloud_service_properties.py
- src/omnibase_core/models/infrastructure/model_retry_failure_info.py
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{py,txt}
📄 CodeRabbit inference engine (CLAUDE.md)
Always use Poetry for Python package management and task execution. Use
poetry runprefix for all commands (pytest, mypy, python, etc.) - never use pip or python directly
Files:
src/omnibase_core/types/typed_dict/typed_dict_node_executor_health.pysrc/omnibase_core/types/typed_dict/typed_dict_fsm_context.pysrc/omnibase_core/types/typed_dict/typed_dict_discovery_extended_stats.pysrc/omnibase_core/types/typed_dict/typed_dict_service_health.pysrc/omnibase_core/types/typed_dict/typed_dict_introspection_data.pysrc/omnibase_core/types/typed_dict/typed_dict_discovery_stats.pysrc/omnibase_core/types/typed_dict/typed_dict_tool_execution_result.pysrc/omnibase_core/types/typed_dict/typed_dict_workflow_step_config.pysrc/omnibase_core/types/typed_dict/typed_dict_registry_stats.pysrc/omnibase_core/models/operations/model_event_payload.pysrc/omnibase_core/types/typed_dict/typed_dict_tool_execution_response.pysrc/omnibase_core/models/configuration/__init__.pysrc/omnibase_core/models/configuration/model_node_config_entry.pysrc/omnibase_core/models/examples/model_example_metadata.pysrc/omnibase_core/types/typed_dict/typed_dict_redacted_data.pysrc/omnibase_core/types/typed_dict/typed_dict_reducer_fsm_context.pysrc/omnibase_core/models/infrastructure/progress/model_progress_metrics.pysrc/omnibase_core/models/cli/model_cli_result_metadata.pysrc/omnibase_core/models/infrastructure/model_metric.pysrc/omnibase_core/models/connections/model_connection_pool.pysrc/omnibase_core/models/operations/model_workflow_instance_metadata.pysrc/omnibase_core/models/cli/model_cli_execution_config.pysrc/omnibase_core/types/typed_dict/typed_dict_executor_health.pysrc/omnibase_core/types/typed_dict/typed_dict_metric_entry.pysrc/omnibase_core/models/configuration/model_log_filter_config.pysrc/omnibase_core/models/examples/model_data_handling_declaration.pysrc/omnibase_core/models/cli/model_result_summary.pysrc/omnibase_core/types/typed_dict/typed_dict_filter_criteria.pysrc/omnibase_core/models/connections/model_connection_info.pysrc/omnibase_core/types/typed_dict/typed_dict_performance_profile.pysrc/omnibase_core/types/typed_dict/__init__.pysrc/omnibase_core/models/examples/model_example_summary.pysrc/omnibase_core/models/examples/model_examples_collection_summary.pysrc/omnibase_core/models/connections/model_database_properties.pysrc/omnibase_core/models/cli/model_cli_execution_summary.pysrc/omnibase_core/models/examples/model_fallback_strategy.pysrc/omnibase_core/types/typed_dict/typed_dict_cache_stats.pysrc/omnibase_core/models/infrastructure/model_retry_execution.pysrc/omnibase_core/models/examples/model_example.pysrc/omnibase_core/models/examples/model_property_metadata.pysrc/omnibase_core/models/operations/model_event_metadata.pysrc/omnibase_core/models/examples/model_typed_property.pysrc/omnibase_core/models/cli/model_cli_execution_context.pysrc/omnibase_core/models/examples/model_example_metadata_summary.pysrc/omnibase_core/models/infrastructure/model_duration.pysrc/omnibase_core/models/connections/model_connection_metrics.pysrc/omnibase_core/models/examples/model_namespace_config.pysrc/omnibase_core/models/operations/model_operation_payload.pysrc/omnibase_core/models/infrastructure/model_metrics_data.pysrc/omnibase_core/models/security/model_permission_scope.pysrc/omnibase_core/models/examples/model_fallback_metadata.pysrc/omnibase_core/models/connections/model_connection_auth.pysrc/omnibase_core/types/typed_dict/typed_dict_lazy_cache_stats.pysrc/omnibase_core/types/typed_dict/typed_dict_event_metadata.pysrc/omnibase_core/models/examples/model_example_output_data.pysrc/omnibase_core/models/connections/model_connection_security.pysrc/omnibase_core/models/operations/model_computation_input_base.pysrc/omnibase_core/models/infrastructure/model_result_dict.pysrc/omnibase_core/types/typed_dict_mixin_types.pysrc/omnibase_core/models/examples/model_example_input_data.pytests/unit/infrastructure/test_node_config_provider.pysrc/omnibase_core/types/typed_dict/typed_dict_serialized_result.pysrc/omnibase_core/types/typed_dict/typed_dict_health_check_status.pysrc/omnibase_core/models/configuration/model_node_config_value.pysrc/omnibase_core/models/infrastructure/__init__.py
src/omnibase_core/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/omnibase_core/**/*.py: Use protocol names for dependency injection, never concrete class names. Example:container.get_service('ProtocolEventBus')not concrete class names
Always use ModelOnexError for error handling, never generic Exception. Include error_code from EnumCoreErrorCode and context information
Use @standard_error_handling decorator to eliminate 6+ lines of try/catch boilerplate in async operations
Enforce 100% strict mypy compliance - all functions must have type annotations withdisallow_untyped_defs = true
Use EnumNodeKind for architectural classification (EFFECT, COMPUTE, REDUCER, ORCHESTRATOR, RUNTIME_HOST) in workflow routing
Use EnumNodeType for specific implementation type discovery and capability matching (TRANSFORMER, AGGREGATOR, COMPUTE_GENERIC, etc.)
Format code using Black and sort imports using isort - runpoetry run black src/ tests/andpoetry run isort src/ tests/before commit
Run all pre-commit hooks before pushing:pre-commit run --all-files- includes mypy strict, black, isort, ruff
Use duck typing with protocols - never use isinstance checks. Protocols use structural subtyping, not explicit inheritance
Process inter-service communication using ModelEventEnvelope pattern for event-driven architecture
Use thread-local storage or separate node instances for concurrent thread access - ModelComputeCache, ModelCircuitBreaker, NodeCompute/NodeEffect are NOT thread-safe
Files:
src/omnibase_core/types/typed_dict/typed_dict_node_executor_health.pysrc/omnibase_core/types/typed_dict/typed_dict_fsm_context.pysrc/omnibase_core/types/typed_dict/typed_dict_discovery_extended_stats.pysrc/omnibase_core/types/typed_dict/typed_dict_service_health.pysrc/omnibase_core/types/typed_dict/typed_dict_introspection_data.pysrc/omnibase_core/types/typed_dict/typed_dict_discovery_stats.pysrc/omnibase_core/types/typed_dict/typed_dict_tool_execution_result.pysrc/omnibase_core/types/typed_dict/typed_dict_workflow_step_config.pysrc/omnibase_core/types/typed_dict/typed_dict_registry_stats.pysrc/omnibase_core/models/operations/model_event_payload.pysrc/omnibase_core/types/typed_dict/typed_dict_tool_execution_response.pysrc/omnibase_core/models/configuration/__init__.pysrc/omnibase_core/models/configuration/model_node_config_entry.pysrc/omnibase_core/models/examples/model_example_metadata.pysrc/omnibase_core/types/typed_dict/typed_dict_redacted_data.pysrc/omnibase_core/types/typed_dict/typed_dict_reducer_fsm_context.pysrc/omnibase_core/models/infrastructure/progress/model_progress_metrics.pysrc/omnibase_core/models/cli/model_cli_result_metadata.pysrc/omnibase_core/models/infrastructure/model_metric.pysrc/omnibase_core/models/connections/model_connection_pool.pysrc/omnibase_core/models/operations/model_workflow_instance_metadata.pysrc/omnibase_core/models/cli/model_cli_execution_config.pysrc/omnibase_core/types/typed_dict/typed_dict_executor_health.pysrc/omnibase_core/types/typed_dict/typed_dict_metric_entry.pysrc/omnibase_core/models/configuration/model_log_filter_config.pysrc/omnibase_core/models/examples/model_data_handling_declaration.pysrc/omnibase_core/models/cli/model_result_summary.pysrc/omnibase_core/types/typed_dict/typed_dict_filter_criteria.pysrc/omnibase_core/models/connections/model_connection_info.pysrc/omnibase_core/types/typed_dict/typed_dict_performance_profile.pysrc/omnibase_core/types/typed_dict/__init__.pysrc/omnibase_core/models/examples/model_example_summary.pysrc/omnibase_core/models/examples/model_examples_collection_summary.pysrc/omnibase_core/models/connections/model_database_properties.pysrc/omnibase_core/models/cli/model_cli_execution_summary.pysrc/omnibase_core/models/examples/model_fallback_strategy.pysrc/omnibase_core/types/typed_dict/typed_dict_cache_stats.pysrc/omnibase_core/models/infrastructure/model_retry_execution.pysrc/omnibase_core/models/examples/model_example.pysrc/omnibase_core/models/examples/model_property_metadata.pysrc/omnibase_core/models/operations/model_event_metadata.pysrc/omnibase_core/models/examples/model_typed_property.pysrc/omnibase_core/models/cli/model_cli_execution_context.pysrc/omnibase_core/models/examples/model_example_metadata_summary.pysrc/omnibase_core/models/infrastructure/model_duration.pysrc/omnibase_core/models/connections/model_connection_metrics.pysrc/omnibase_core/models/examples/model_namespace_config.pysrc/omnibase_core/models/operations/model_operation_payload.pysrc/omnibase_core/models/infrastructure/model_metrics_data.pysrc/omnibase_core/models/security/model_permission_scope.pysrc/omnibase_core/models/examples/model_fallback_metadata.pysrc/omnibase_core/models/connections/model_connection_auth.pysrc/omnibase_core/types/typed_dict/typed_dict_lazy_cache_stats.pysrc/omnibase_core/types/typed_dict/typed_dict_event_metadata.pysrc/omnibase_core/models/examples/model_example_output_data.pysrc/omnibase_core/models/connections/model_connection_security.pysrc/omnibase_core/models/operations/model_computation_input_base.pysrc/omnibase_core/models/infrastructure/model_result_dict.pysrc/omnibase_core/types/typed_dict_mixin_types.pysrc/omnibase_core/models/examples/model_example_input_data.pysrc/omnibase_core/types/typed_dict/typed_dict_serialized_result.pysrc/omnibase_core/types/typed_dict/typed_dict_health_check_status.pysrc/omnibase_core/models/configuration/model_node_config_value.pysrc/omnibase_core/models/infrastructure/__init__.py
src/omnibase_core/models/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use Pydantic models for all data structures with v2.11+ compatibility, never use plain dataclasses or Python dicts for domain models
Files:
src/omnibase_core/models/operations/model_event_payload.pysrc/omnibase_core/models/configuration/__init__.pysrc/omnibase_core/models/configuration/model_node_config_entry.pysrc/omnibase_core/models/examples/model_example_metadata.pysrc/omnibase_core/models/infrastructure/progress/model_progress_metrics.pysrc/omnibase_core/models/cli/model_cli_result_metadata.pysrc/omnibase_core/models/infrastructure/model_metric.pysrc/omnibase_core/models/connections/model_connection_pool.pysrc/omnibase_core/models/operations/model_workflow_instance_metadata.pysrc/omnibase_core/models/cli/model_cli_execution_config.pysrc/omnibase_core/models/configuration/model_log_filter_config.pysrc/omnibase_core/models/examples/model_data_handling_declaration.pysrc/omnibase_core/models/cli/model_result_summary.pysrc/omnibase_core/models/connections/model_connection_info.pysrc/omnibase_core/models/examples/model_example_summary.pysrc/omnibase_core/models/examples/model_examples_collection_summary.pysrc/omnibase_core/models/connections/model_database_properties.pysrc/omnibase_core/models/cli/model_cli_execution_summary.pysrc/omnibase_core/models/examples/model_fallback_strategy.pysrc/omnibase_core/models/infrastructure/model_retry_execution.pysrc/omnibase_core/models/examples/model_example.pysrc/omnibase_core/models/examples/model_property_metadata.pysrc/omnibase_core/models/operations/model_event_metadata.pysrc/omnibase_core/models/examples/model_typed_property.pysrc/omnibase_core/models/cli/model_cli_execution_context.pysrc/omnibase_core/models/examples/model_example_metadata_summary.pysrc/omnibase_core/models/infrastructure/model_duration.pysrc/omnibase_core/models/connections/model_connection_metrics.pysrc/omnibase_core/models/examples/model_namespace_config.pysrc/omnibase_core/models/operations/model_operation_payload.pysrc/omnibase_core/models/infrastructure/model_metrics_data.pysrc/omnibase_core/models/security/model_permission_scope.pysrc/omnibase_core/models/examples/model_fallback_metadata.pysrc/omnibase_core/models/connections/model_connection_auth.pysrc/omnibase_core/models/examples/model_example_output_data.pysrc/omnibase_core/models/connections/model_connection_security.pysrc/omnibase_core/models/operations/model_computation_input_base.pysrc/omnibase_core/models/infrastructure/model_result_dict.pysrc/omnibase_core/models/examples/model_example_input_data.pysrc/omnibase_core/models/configuration/model_node_config_value.pysrc/omnibase_core/models/infrastructure/__init__.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Maintain minimum 60% test coverage requirement - runpoetry run pytest tests/ --cov=src/omnibase_core --cov-report=term-missing
Run pytest with parallel workers for faster local testing:poetry run pytest tests/ -n 4(default: 4 workers, can disable with-n 0for debugging)
Use pytest markers to categorize tests: @pytest.mark.unit, @pytest.mark.integration, @pytest.mark.slow, @pytest.mark.smoke, @pytest.mark.performance
CI runs 20 parallel test splits with timeout protection (60s per test) - expected per-split runtime is 2m30s to 3m30s, critical alert if > 4m30s
Files:
tests/unit/infrastructure/test_node_config_provider.py
🧠 Learnings (7)
📓 Common learnings
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-11T21:05:41.997Z
Learning: Applies to src/omnibase_core/nodes/node_reducer.py : Implement REDUCER nodes using NodeReducer base class with FSM-driven state transitions using ModelIntent pattern
📚 Learning: 2025-12-11T21:05:41.997Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-11T21:05:41.997Z
Learning: Applies to src/omnibase_core/**/*.py : Process inter-service communication using ModelEventEnvelope pattern for event-driven architecture
Applied to files:
src/omnibase_core/models/operations/model_event_payload.pysrc/omnibase_core/models/operations/model_event_metadata.py
📚 Learning: 2025-12-11T21:05:41.997Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-11T21:05:41.997Z
Learning: Applies to src/omnibase_core/**/*.py : Always use ModelOnexError for error handling, never generic Exception. Include error_code from EnumCoreErrorCode and context information
Applied to files:
src/omnibase_core/models/operations/model_event_payload.pysrc/omnibase_core/models/examples/model_example_metadata.pysrc/omnibase_core/models/configuration/model_log_filter_config.pysrc/omnibase_core/models/examples/model_data_handling_declaration.pysrc/omnibase_core/models/cli/model_result_summary.pysrc/omnibase_core/models/examples/model_fallback_strategy.pysrc/omnibase_core/models/infrastructure/model_duration.pysrc/omnibase_core/models/examples/model_fallback_metadata.pysrc/omnibase_core/models/infrastructure/model_result_dict.py
📚 Learning: 2025-12-11T21:05:41.997Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-11T21:05:41.997Z
Learning: Applies to src/omnibase_core/nodes/**/*.py : Import node types directly from omnibase_core.nodes: NodeCompute, NodeEffect, NodeReducer, NodeOrchestrator (v0.4.0+)
Applied to files:
src/omnibase_core/models/configuration/__init__.py
📚 Learning: 2025-12-11T21:05:41.997Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-11T21:05:41.997Z
Learning: Applies to src/omnibase_core/nodes/**/*.py : Use ModelONEXContainer for dependency injection in node constructors. Never confuse it with ModelContainer[T] which is a generic value wrapper
Applied to files:
src/omnibase_core/models/configuration/model_node_config_entry.pysrc/omnibase_core/models/configuration/model_node_config_value.py
📚 Learning: 2025-12-11T21:05:41.997Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-11T21:05:41.997Z
Learning: Applies to src/omnibase_core/nodes/node_reducer.py : Implement REDUCER nodes using NodeReducer base class with FSM-driven state transitions using ModelIntent pattern
Applied to files:
src/omnibase_core/types/typed_dict/typed_dict_reducer_fsm_context.py
📚 Learning: 2025-12-11T21:05:41.997Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-11T21:05:41.997Z
Learning: Applies to src/omnibase_core/models/**/*.py : Use Pydantic models for all data structures with v2.11+ compatibility, never use plain dataclasses or Python dicts for domain models
Applied to files:
src/omnibase_core/models/cli/model_cli_result_metadata.pysrc/omnibase_core/models/examples/model_data_handling_declaration.pysrc/omnibase_core/models/connections/model_database_properties.pysrc/omnibase_core/models/examples/model_fallback_strategy.pysrc/omnibase_core/models/examples/model_property_metadata.pysrc/omnibase_core/models/examples/model_typed_property.pysrc/omnibase_core/models/infrastructure/model_duration.pysrc/omnibase_core/models/examples/model_fallback_metadata.pysrc/omnibase_core/models/examples/model_example_output_data.pysrc/omnibase_core/models/connections/model_connection_security.pysrc/omnibase_core/models/infrastructure/model_result_dict.pysrc/omnibase_core/models/examples/model_example_input_data.py
🧬 Code graph analysis (14)
src/omnibase_core/types/typed_dict/typed_dict_node_executor_health.py (1)
src/omnibase_core/models/metadata/model_metadata_field_info.py (1)
uuid(161-180)
src/omnibase_core/types/typed_dict/typed_dict_fsm_context.py (1)
src/omnibase_core/infrastructure/node_base.py (1)
current_state(762-770)
src/omnibase_core/types/typed_dict/typed_dict_service_health.py (1)
src/omnibase_core/models/metadata/model_metadata_field_info.py (1)
uuid(161-180)
src/omnibase_core/types/typed_dict/typed_dict_introspection_data.py (1)
src/omnibase_core/models/node_metadata/model_node_metadata_info.py (1)
capabilities(153-155)
src/omnibase_core/types/typed_dict/typed_dict_discovery_stats.py (2)
src/omnibase_core/protocols/types/protocol_service_instance.py (1)
is_available(41-43)src/omnibase_core/models/node_metadata/model_node_metadata_info.py (1)
capabilities(153-155)
src/omnibase_core/models/configuration/__init__.py (2)
src/omnibase_core/models/configuration/model_node_config_entry.py (1)
ModelNodeConfigEntry(9-35)src/omnibase_core/models/configuration/model_node_config_value.py (1)
ModelNodeConfigSchema(13-30)
src/omnibase_core/models/configuration/model_node_config_entry.py (2)
src/omnibase_core/protocols/event_bus/protocol_event_message.py (1)
key(28-30)src/omnibase_core/models/core/model_extension_data.py (1)
value_type(92-108)
src/omnibase_core/types/typed_dict/typed_dict_filter_criteria.py (1)
src/omnibase_core/models/node_metadata/model_node_metadata_info.py (1)
capabilities(153-155)
src/omnibase_core/types/typed_dict/__init__.py (18)
src/omnibase_core/types/typed_dict/typed_dict_cache_stats.py (1)
TypedDictCacheStats(8-13)src/omnibase_core/types/typed_dict/typed_dict_discovery_extended_stats.py (1)
TypedDictDiscoveryExtendedStats(8-19)src/omnibase_core/types/typed_dict/typed_dict_event_metadata.py (1)
TypedDictEventMetadata(8-14)src/omnibase_core/types/typed_dict/typed_dict_executor_health.py (1)
TypedDictExecutorHealth(8-17)src/omnibase_core/types/typed_dict/typed_dict_filter_criteria.py (1)
TypedDictFilterCriteria(8-14)src/omnibase_core/types/typed_dict/typed_dict_health_check_status.py (1)
TypedDictHealthCheckStatus(8-15)src/omnibase_core/types/typed_dict/typed_dict_introspection_data.py (1)
TypedDictIntrospectionData(8-16)src/omnibase_core/types/typed_dict/typed_dict_lazy_cache_stats.py (1)
TypedDictLazyCacheStats(8-15)src/omnibase_core/types/typed_dict/typed_dict_metric_entry.py (1)
TypedDictMetricEntry(8-12)src/omnibase_core/types/typed_dict/typed_dict_node_executor_health.py (1)
TypedDictNodeExecutorHealth(9-21)src/omnibase_core/types/typed_dict/typed_dict_performance_profile.py (1)
TypedDictPerformanceProfile(8-13)src/omnibase_core/types/typed_dict/typed_dict_redacted_data.py (1)
TypedDictRedactedData(8-12)src/omnibase_core/types/typed_dict/typed_dict_registry_stats.py (1)
TypedDictRegistryStats(8-15)src/omnibase_core/types/typed_dict/typed_dict_serialized_result.py (1)
TypedDictSerializedResult(8-11)src/omnibase_core/types/typed_dict/typed_dict_service_health.py (1)
TypedDictServiceHealth(9-21)src/omnibase_core/types/typed_dict/typed_dict_tool_execution_response.py (1)
TypedDictToolExecutionResponse(8-16)src/omnibase_core/types/typed_dict/typed_dict_tool_execution_result.py (1)
TypedDictToolExecutionResult(8-14)src/omnibase_core/types/typed_dict/typed_dict_workflow_step_config.py (1)
TypedDictWorkflowStepConfig(8-14)
src/omnibase_core/types/typed_dict/typed_dict_event_metadata.py (1)
src/omnibase_core/protocols/event_bus/protocol_event_bus_headers.py (1)
event_type(55-57)
src/omnibase_core/models/operations/model_computation_input_base.py (1)
src/omnibase_core/models/common/model_schema_value.py (1)
ModelSchemaValue(22-347)
src/omnibase_core/types/typed_dict_mixin_types.py (20)
src/omnibase_core/types/typed_dict/typed_dict_cache_stats.py (1)
TypedDictCacheStats(8-13)src/omnibase_core/types/typed_dict/typed_dict_discovery_extended_stats.py (1)
TypedDictDiscoveryExtendedStats(8-19)src/omnibase_core/types/typed_dict/typed_dict_event_metadata.py (1)
TypedDictEventMetadata(8-14)src/omnibase_core/types/typed_dict/typed_dict_executor_health.py (1)
TypedDictExecutorHealth(8-17)src/omnibase_core/types/typed_dict/typed_dict_filter_criteria.py (1)
TypedDictFilterCriteria(8-14)src/omnibase_core/types/typed_dict/typed_dict_fsm_context.py (1)
TypedDictFSMContext(9-15)src/omnibase_core/types/typed_dict/typed_dict_health_check_status.py (1)
TypedDictHealthCheckStatus(8-15)src/omnibase_core/types/typed_dict/typed_dict_introspection_data.py (1)
TypedDictIntrospectionData(8-16)src/omnibase_core/types/typed_dict/typed_dict_lazy_cache_stats.py (1)
TypedDictLazyCacheStats(8-15)src/omnibase_core/types/typed_dict/typed_dict_metric_entry.py (1)
TypedDictMetricEntry(8-12)src/omnibase_core/types/typed_dict/typed_dict_node_executor_health.py (1)
TypedDictNodeExecutorHealth(9-21)src/omnibase_core/types/typed_dict/typed_dict_performance_profile.py (1)
TypedDictPerformanceProfile(8-13)src/omnibase_core/types/typed_dict/typed_dict_redacted_data.py (1)
TypedDictRedactedData(8-12)src/omnibase_core/types/typed_dict/typed_dict_reducer_fsm_context.py (1)
TypedDictReducerFSMContext(8-13)src/omnibase_core/types/typed_dict/typed_dict_registry_stats.py (1)
TypedDictRegistryStats(8-15)src/omnibase_core/types/typed_dict/typed_dict_serialized_result.py (1)
TypedDictSerializedResult(8-11)src/omnibase_core/types/typed_dict/typed_dict_service_health.py (1)
TypedDictServiceHealth(9-21)src/omnibase_core/types/typed_dict/typed_dict_tool_execution_response.py (1)
TypedDictToolExecutionResponse(8-16)src/omnibase_core/types/typed_dict/typed_dict_tool_execution_result.py (1)
TypedDictToolExecutionResult(8-14)src/omnibase_core/types/typed_dict/typed_dict_workflow_step_config.py (1)
TypedDictWorkflowStepConfig(8-14)
tests/unit/infrastructure/test_node_config_provider.py (1)
src/omnibase_core/models/configuration/model_node_config_value.py (1)
ModelNodeConfigSchema(13-30)
src/omnibase_core/models/configuration/model_node_config_value.py (1)
src/omnibase_core/models/configuration/model_node_config_entry.py (1)
ModelNodeConfigEntry(9-35)
⏰ 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: claude-review
- GitHub Check: Smoke Tests
| from omnibase_core.types.type_serializable_value import SerializedDict | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd "model_example" .Repository: OmniNode-ai/omnibase_core
Length of output: 1030
🏁 Script executed:
find . -name "model_example.py" 2>/dev/nullRepository: OmniNode-ai/omnibase_core
Length of output: 121
🏁 Script executed:
git status | head -20
git log --oneline -5Repository: OmniNode-ai/omnibase_core
Length of output: 210
🏁 Script executed:
cat -n src/omnibase_core/models/examples/model_example.py | head -70Repository: OmniNode-ai/omnibase_core
Length of output: 2391
🏁 Script executed:
grep -n "SerializedDict" src/omnibase_core/models/examples/model_example.pyRepository: OmniNode-ai/omnibase_core
Length of output: 189
🏁 Script executed:
cat -n src/omnibase_core/models/examples/model_example.py | tail -50Repository: OmniNode-ai/omnibase_core
Length of output: 1932
🏁 Script executed:
cd /tmp && git show HEAD:src/omnibase_core/models/examples/model_example.py | head -30Repository: OmniNode-ai/omnibase_core
Length of output: 137
🏁 Script executed:
poetry run isort --diff src/omnibase_core/models/examples/model_example.py 2>&1 | head -100Repository: OmniNode-ai/omnibase_core
Length of output: 113
🏁 Script executed:
python3 << 'EOF'
# Simulate what proper import organization should look like per PEP 8
content = """from __future__ import annotations
\"\"\"
Example model for examples collection.
This module provides the ModelExample class for strongly typed
example data with comprehensive fields and validation.
\"\"\"
from datetime import UTC, datetime
from uuid import UUID, uuid4
from pydantic import BaseModel, Field
from omnibase_core.types.type_serializable_value import SerializedDict
from .model_example_context_data import ModelExampleContextData
from .model_example_data import ModelExampleInputData, ModelExampleOutputData
"""
print("PROPER IMPORT ORDER (per PEP 8 and isort):")
print(content)
print("\n" + "="*60)
print("CURRENT STATE (IMPROPER):")
print("Lines 1-5: First import block")
print("Lines 7-12: Module docstring (BETWEEN imports - WRONG)")
print("Lines 15-23: Second import block")
print("\nThe docstring should come IMMEDIATELY after 'from __future__ import annotations'")
print("and BEFORE all other imports.")
EOFRepository: OmniNode-ai/omnibase_core
Length of output: 155
Run isort to fix import organization and docstring placement.
The SerializedDict import at line 20 is correctly positioned, but the module docstring (lines 7-12) is currently between import blocks, which violates PEP 8. Per coding guidelines, run poetry run isort src/omnibase_core/models/examples/model_example.py to reorganize imports with the docstring properly placed immediately after from __future__ import annotations and before all other imports.
🤖 Prompt for AI Agents
In src/omnibase_core/models/examples/model_example.py around lines 20-21, the
module docstring is incorrectly placed between import blocks violating PEP8; run
the project isort command to reorder imports and move the module docstring
immediately after "from __future__ import annotations". Execute: poetry run
isort src/omnibase_core/models/examples/model_example.py (or run isort as
configured in repo) so imports are grouped correctly and the docstring sits
before other imports.
| from typing import NotRequired, TypedDict | ||
|
|
||
|
|
||
| class TypedDictWorkflowStepConfig(TypedDict, total=False): | ||
| """TypedDict for workflow step configuration from YAML.""" | ||
|
|
||
| step_name: str | ||
| step_type: str | ||
| timeout_ms: NotRequired[int] | ||
| depends_on: NotRequired[list[str]] | ||
|
|
There was a problem hiding this comment.
total=False makes step_name/step_type optional (likely unintended). Drop total=False and keep optional keys as NotRequired[...] (or use Required[...] explicitly).
-from typing import NotRequired, TypedDict
+from typing import NotRequired, Required, TypedDict
-class TypedDictWorkflowStepConfig(TypedDict, total=False):
+class TypedDictWorkflowStepConfig(TypedDict):
"""TypedDict for workflow step configuration from YAML."""
- step_name: str
- step_type: str
+ step_name: Required[str]
+ step_type: Required[str]
timeout_ms: NotRequired[int]
depends_on: NotRequired[list[str]]🤖 Prompt for AI Agents
In src/omnibase_core/types/typed_dict/typed_dict_workflow_step_config.py around
lines 5 to 15, the TypedDict is declared with total=False which inadvertently
makes step_name and step_type optional; remove total=False and make optional
fields explicit using NotRequired[...] (or mark required fields with
Required[...] if preferred) so step_name and step_type remain required and
timeout_ms/depends_on remain optional.
PR Review Issues Addressed: 🟠 MAJOR: - FSM deep immutability: Add validators to convert lists → tuples (model_fsm_transition.py: conditions/actions) - String constraints: Add min_length=1 to action_name, action_type, new_state, old_state fields - Timezone-aware timestamps: datetime.now() → datetime.now(UTC) - Metadata sorting: Sort keys for deterministic hashing - ConfigValue union ordering: bool | int | float | str (bool before int) - TypedDict fixes: - typed_dict_node_executor_health: str|UUID → UUID - typed_dict_redacted_data: Add meaningful fields (was empty) - typed_dict_tool_execution_response: execution_time → execution_time_ms - typed_dict_workflow_step_config: Remove total=False - typed_dict_discovery_extended_stats: Add None-ability 🟡 MINOR: - Import organization in model_example.py Tests: - Add comprehensive test_model_fsm_transition_result.py (48 tests) - Update test_model_fsm_transition_action.py for min_length=1 behavior - All 357 FSM tests passing
PR Review Issues Addressed: 🟠 MAJOR: - FSM deep immutability: Add validators to convert lists → tuples (model_fsm_transition.py: conditions/actions) - String constraints: Add min_length=1 to action_name, action_type, new_state, old_state fields - Timezone-aware timestamps: datetime.now() → datetime.now(UTC) - Metadata sorting: Sort keys for deterministic hashing - ConfigValue union ordering: bool | int | float | str (bool before int) - TypedDict fixes: - typed_dict_node_executor_health: str|UUID → UUID - typed_dict_redacted_data: Add meaningful fields (was empty) - typed_dict_tool_execution_response: execution_time → execution_time_ms - typed_dict_workflow_step_config: Remove total=False - typed_dict_discovery_extended_stats: Add None-ability 🟡 MINOR: - Import organization in model_example.py Tests: - Add comprehensive test_model_fsm_transition_result.py (48 tests) - Update test_model_fsm_transition_action.py for min_length=1 behavior - All 357 FSM tests passing
…gParameter - model_fsmtransitionaction.py: Fix attribute access from parameter_name to name to match ModelActionConfigParameter's actual field name - test_model_fsm_transition_action.py: Update tests to use correct model schema: - Use name/type/required/default fields instead of version/parameter_name/parameter_value - Fix default value assertion (list not tuple) These fixes resolve mypy type errors and test collection failures.
4d1e004 to
b7ceafc
Compare
PR #165 Review: MVP FSM Contract Models for NodeReducer v1.0Executive SummaryOverall Assessment: ✅ APPROVE with minor suggestions This PR introduces four new MVP FSM contract models that are well-architected, thoroughly tested, and align excellently with ONEX patterns. The implementation demonstrates strong type safety, deep immutability, and comprehensive test coverage. Key Metrics:
1. New MVP FSM Models Review✅ ModelFSMTransitionCondition (
|
Changes: - FSM models: Move module docstrings to top of files - FSM models: Mark unused **kwargs as **_kwargs for linting - FSM models: Remove unused import in model_fsm_transition_action.py - FSM models: Add operator documentation to ModelFSMTransitionCondition - ModelActionConfigParameter: Add ConfigDict(frozen=True) for immutability - TypedDicts: Change str|UUID to str in typed_dict_node_executor_health.py - TypedDicts: Fix execution_time_ms type consistency (float -> int) - mixin_node_executor: Add str() conversion for node_id UUID - Rewrite test_model_action_config_parameter.py to match actual schema All 70 tests pass for ModelActionConfigParameter.
PR #165 Review: MVP FSM Contract Models for NodeReducer v1.0Executive SummaryRecommendation: ✅ APPROVE with minor observations This PR adds 4 new FSM contract models with excellent code quality, comprehensive testing (3,690+ lines of tests), and strong architectural alignment with ONEX patterns. The changes improve type safety through immutability patterns (tuple usage) and comply with ONEX anti-pattern rules by replacing 1. Code Quality ✅ EXCELLENTStrengthsStrict Type Safety:
Protocol Implementation:
Field Validation:
Minor Observations
2. Type Safety ✅ EXCELLENTMypy ComplianceAll CI checks passing including mypy strict mode validation. Type ImprovementsBefore (dict soup): action_config: dict[str, Any] # Allows anythingAfter (strictly typed): ActionConfigValue = str | int | float | bool | tuple[str, ...] | None
action_config: tuple[tuple[str, ActionConfigValue], ...] # Restricted types onlyImmutability via TuplesThe PR consistently replaces # Before:
history: list[str] = field(default_factory=list)
# After:
history: tuple[str, ...] = ()Impact: This prevents accidental mutation and enforces functional programming patterns in FSM execution. 3. Testing ✅ EXCELLENTTest CoverageTotal Test Lines: 3,690 lines across 4 test files
Test QualityComprehensive Coverage:
Tests even document Python runtime behavior for future maintainers - excellent attention to detail. 4. Architecture ✅ EXCELLENTONEX Four-Node Architecture Alignment
ONEX Anti-Pattern ComplianceBefore (Anti-pattern): action_config: dict[str, Any] # ONEX anti-patternAfter (Compliant): ActionConfigValue = str | int | float | bool | tuple[str, ...] | NoneThe PR explicitly addresses this: "ActionConfigValue type alias replaces dict[str, Any] to comply with ONEX anti-pattern rules" 5. Security 🔒 NO CONCERNSInput Validation✅ Strong validation boundaries:
Immutability Security✅ Frozen models prevent tampering:
Reserved Fields (v1.1+)✅ No security risk - Future fields are parsed but NOT executed in v1.0 6. Breaking Changes 🟡 MINOR (Documented)API ChangesHistory field type change: # Before:
def get_fsm_state_history(self) -> list[str]
# After:
def get_fsm_state_history(self) -> tuple[str, ...]Impact: Low - Tuples support same iteration/indexing as lists. Most code should work unchanged. Versioning ContextPer CLAUDE.md: "Until v0.4.0 is released, this project does NOT maintain backwards compatibility." Verdict: Changes are acceptable under pre-release policy. The tuple change is an improvement (immutability) that should be retained in v0.4.0. 7. Documentation ✅ EXCELLENTAll models include:
Tests include clear names, docstrings explaining runtime behavior, and edge case justifications. 8. Summary & RecommendationsStrengths
Recommendations
Final Verdict: ✅ APPROVEThis PR demonstrates exemplary code quality with strong type safety improvements, comprehensive test coverage, clear architectural alignment, and excellent documentation. The changes move omnibase_core toward stronger immutability guarantees and better type safety, which are both highly valuable for FSM-driven state management in production systems. Reviewed by: Claude Code (Sonnet 4.5) |
Resolved conflicts: - model_node_config_value.py: Keep imports from separate module - model_example.py: Keep UTC and datetime imports - model_fallback_metadata.py: Keep Field and ModelOnexError imports - model_property_value.py: Keep Field, model_validator, ModelOnexError imports - model_initialization_metadata.py: Use proper ternary formatting - typed_dict_mixin_types.py: Keep imports from typed_dict subpackage - test_node_config_provider.py: Keep isinstance check for schema
PR #165 Review: FSM Contract Models for NodeReducer v1.0SummaryThis PR adds comprehensive FSM contract models for NodeReducer v1.0, implementing a declarative contract-driven architecture. The implementation demonstrates strong adherence to ONEX patterns with excellent test coverage (4,600+ new test lines across 14 test files). ✅ Strengths1. Excellent Architecture & Design
2. Code Quality
3. Testing
4. Documentation
5. Refactoring Quality
🟡 Minor Observations1. Unused Parameter Marking (NITPICK)Good practice marking unused kwargs with # ✅ Good (model_fsm_transition_condition.py:145)
def execute(self, **_kwargs: object) -> bool:
return True2. validate_instance() ImplementationThe validate_instance() methods now have proper validation logic instead of just returning True. This is excellent for catching invalid states at runtime. 3. Type Consistency
🟢 Security & PerformanceSecurity
Performance
🟢 Test Coverage AnalysisBased on file additions (+7,386) and deletions (-1,443):
🎯 Recommendations1. Consider Adding Runtime Type Checks (OPTIONAL)For ActionConfigValue tuple validation, consider adding a comment explaining why runtime checks are needed (tuple[str, ...] type hints can't be validated without iteration). 2. Document Deep Immutability Pattern (NICE-TO-HAVE)Consider adding a guide in
3. CI Performance Monitoring (FYI)With 4,600+ new test lines, monitor CI split performance:
🎉 ConclusionOverall Assessment: EXCELLENT ⭐⭐⭐⭐⭐ This PR demonstrates exemplary software engineering:
The FSM contract models provide a solid foundation for NodeReducer v1.0's declarative architecture. The deep immutability pattern, comprehensive validation, and protocol compliance make these models production-ready. Recommendation: APPROVE ✅ No blocking issues. The minor observations are nitpicks and optional enhancements. This is high-quality work that follows all CLAUDE.md guidelines. Reviewed by: Claude Code |
- Fix ModelFSMTransitionAction import in test_fsm_executor.py (use FSM module instead of subcontracts) - Remove invalid 'version' parameters from FSM models that don't have them - Update error code assertions in test_safe_yaml_loader.py to expect more specific codes (NOT_FOUND, CONVERSION_ERROR) - Add pydantic.ValidationError handling in contracts.py validation - Fix exception type in test_contracts_extended.py (OSError vs RuntimeError) All 133 affected tests now pass.
Code Review for PR #165: FSM Contract Models for NodeReducer v1.0SummaryThis PR introduces four new MVP FSM contract models (ModelFSMTransitionCondition, ModelFSMTransitionAction, ModelFSMOperation, ModelActionConfigParameter) with comprehensive test coverage (214 tests). The implementation follows ONEX architecture patterns, consolidates FSM models, and implements deep immutability for thread safety. ✅ Strengths1. Excellent Architecture & Design
2. Strong Type Safety
3. Comprehensive Testing
4. Documentation Quality
🟡 Minor Issues & Suggestions1. Validator Convenience vs Immutability (Minor)File: The
2. Expression Validation Could Be Stricter (Enhancement)File: Current validation only checks token count (3 tokens), but doesn't validate:
Recommendation: Consider adding operator validation in a future PR: VALID_OPERATORS = {'==', '\!=', '<', '>', '<=', '>=', 'in', 'not_in', 'contains', 'matches'}
tokens = self.expression.split()
if tokens[1] not in VALID_OPERATORS:
raise ModelOnexError(...)3. Timeout Constraint Inconsistency (Nitpick)Files:
Question: Should timeout_ms=0 be allowed? If not, ModelFSMTransitionCondition should use Recommendation: Use 4. Type Alias Location (Organizational)File: The Recommendation: Consider moving 🟢 Security & PerformanceSecurity ✅
Performance ✅
📊 Test CoverageTest Metrics
Coverage Gaps (Future Work)
🎯 Code Quality Metrics
🔍 Specific Code Patterns Review✅ Correct Patterns
✅ ONEX Compliance
📝 Recommendations SummaryMust Fix (None) ✅All critical issues have been addressed in the commits. Should Fix (Optional Improvements)
Nice to Have
🎉 Final VerdictAPPROVED ✅This is excellent work that demonstrates:
Impact Assessment
CI Health
🙏 AcknowledgmentsGreat job addressing all review feedback across 17 commits:
Ready to merge! 🚀 Review conducted per CLAUDE.md guidelines. All checks performed against omnibase_core v0.4.0 standards. |
Code Review - PR #165: FSM Contract Models for NodeReducer v1.0Overall AssessmentStatus: ✅ Excellent work with minor recommendations This is a comprehensive, well-architected PR that adds MVP FSM contract models for the NodeReducer v1.0 contract-driven architecture. The code quality is exceptional, following ONEX patterns rigorously. Summary:
✅ Strengths1. Architectural Excellence
2. Type Safety
3. Code Quality
4. Validation Logic
🟡 Minor Issues & Recommendations1. Type Alias Removal (Low Priority)The PR removes many type aliases to inline Union types per ONEX standards. While this improves compliance, it reduces readability in complex signatures. Example (model_fsm_transition_action.py:98-100): action_config: tuple[
tuple[str, str | int | float | bool | tuple[str, ...] | None], ...
] = Field(...)Recommendation: Consider if a limited set of well-named type aliases (properly documented) might improve maintainability without violating standards. This is a design tradeoff worth discussing with the team. 2. FSMContextType Design DecisionThe new Concern: This becomes a "escape hatch" that could propagate through the codebase. Recommendation:
3. Validator Ordering in Immutable CollectionsField validators convert Minor Concern: The validators use def _convert_states_to_tuple(
cls, v: list[Any] | tuple[Any, ...] | Any
) -> tuple[Any, ...]:Recommendation: This is acceptable since Pydantic validates types after validators run, but consider adding a comment explaining why 4. Reserved Field DocumentationThe models excellently document reserved fields (e.g., Recommendation: ✅ Perfect as-is. Consider adding a migration guide when v1.1 ships to help users enable these features. 🟢 Best Practices Observed
🔍 Security & PerformanceSecurity: ✅ No concerns
Performance: ✅ No concerns
Thread Safety: ✅ Excellent
📊 Test Coverage AssessmentBased on the PR description (214 tests) and test file examination: Coverage: ✅ Excellent
Recommendation: Consider adding:
📝 DocumentationDocumentation Quality: ✅ Excellent Highlights:
Recommendation: Consider adding a migration guide for users upgrading from pre-v0.4.0 FSM contracts. 🎯 Adherence to CLAUDE.md Standards
✅ Final RecommendationLGTM with Minor Suggestions This PR is ready to merge. The code quality is exceptional, tests are comprehensive, and it follows ONEX architectural patterns rigorously. The minor recommendations above are for future consideration, not blockers. Action Items (Optional, Post-Merge):
Excellent work on this implementation! The deep immutability, comprehensive validation, and forward-compatible design are exemplary. Reviewed by: Claude Code (Sonnet 4.5) |
…N-581] FSM Model Improvements: - Change timeout_ms constraint from ge=0 to gt=0 (0ms timeout invalid) - Add operator whitelist validation in validate_expression_format() - Valid operators: ==, !=, <, >, <=, >=, in, not_in, contains, matches Type Alias Removal (inline all Union types per ONEX standards): - model_fsm_transition_action.py: Remove ActionConfigValue - compute_transformations.py: Remove JsonValue, JsonType, TransformationHandler - model_effect_result_types.py: Remove EffectResultUnion - model_custom_filters.py: Remove FilterType - model_metadata_tool_collection.py: Remove ToolDataType + compatibility aliases - model_action_payload_types.py: Remove SpecificActionPayload - model_action_config_value.py: Remove ModelActionConfigValue - model_security_utils.py: Remove JSONValue (use object for recursive type) - model_node_configuration_value.py: Remove ModelNodeConfigurationValue - model_node_status_types.py: Remove NodeStatusUnion All 143 FSM tests pass, mypy strict mode clean.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (8)
src/omnibase_core/models/node_metadata/model_node_configuration_value.py (1)
95-115: Bug:from_value(True/False)will hit theintbranch (bool is subclass of int).This currently coerces
True -> 1/False -> 0, contradicting the doc/comment and the repo’s established pattern.def from_value( value: object, ) -> Union[ModelNodeConfigurationStringValue, ModelNodeConfigurationNumericValue]: @@ - if isinstance(value, str): + if isinstance(value, bool): + # No boolean variant in this model; preserve intent without int-coercion. + return from_string("true" if value else "false") + if isinstance(value, str): return from_string(value) if isinstance(value, int): return from_int(value) if isinstance(value, float): return from_float(value) # Fallback to string representation for bool and other types return from_string(str(value))src/omnibase_core/models/operations/model_effect_result_types.py (1)
22-47: Remove unusedEffectResultDiscriminatorandget_effect_result_discriminator, or wire the discriminator and replaceisinstance(v, dict)with duck typing.Currently,
EffectResultDiscriminatorandget_effect_result_discriminator()are dead code—the function is never invoked becauseModelEffectResultusesField(discriminator="result_type")instead. Additionally,get_effect_result_discriminatorusesisinstance(v, dict), which violates the duck-typing guideline ("never use isinstance checks").Option A (simplest): Delete both
EffectResultDiscriminatorandget_effect_result_discriminatorsinceField(discriminator="result_type")already works correctly with Pydantic v2.12.5 and the model variants have properLiteraltype definitions forresult_type.Option B (if custom error context is needed): Keep
EffectResultDiscriminator, wire it intoModelEffectResultby replacingField(discriminator="result_type")withEffectResultDiscriminator, and fix theisinstancecheck using duck typing (e.g.,hasattr(v, "get")for mapping protocol detection).src/omnibase_core/models/core/model_action_payload_types.py (3)
1-13: String literal at Lines 6-10 isn’t a module docstring (dead statement).
Move it to the top of the file (or convert to#comments) so tooling picks it up and it doesn’t execute as a no-op at import time.-from typing import Union - -from omnibase_core.enums.enum_core_error_code import EnumCoreErrorCode -from omnibase_core.models.errors.model_onex_error import ModelOnexError - -""" -Action Payload Type Hierarchies. - -Re-exports all payload types from their individual files and provides factory functions. -""" - -from typing import Any +""" +Action Payload Type Hierarchies. + +Re-exports all payload types from their individual files and provides factory functions. +""" + +from typing import Any, Union + +from omnibase_core.enums.enum_core_error_code import EnumCoreErrorCode +from omnibase_core.models.errors.model_onex_error import ModelOnexError
81-119: Add context information toModelOnexErrorwhen raising unknown action type error.The error should include action name and category as keyword arguments for debugging. Pass
action_nameandcategoryas kwargs to the error constructor instead of acontextdict parameter (which is not supported).msg = f"Unknown action type: {action_type.name}" raise ModelOnexError( error_code=EnumCoreErrorCode.VALIDATION_ERROR, message=msg, + action_name=action_type.name, + category=action_type.category, )
46-108: Remove invalid"list[Any]"action name and refactor hardcoded routing to use action categories.The
"list[Any]"string at line 97 is not a valid action name—it contains brackets that violateModelNodeActionType.validate_name_format(), which requires lowercase alphanumeric + underscores only. This check is unreachable dead code.More critically, hardcoded name checks (lines 95–105) override the category-based routing design. For example,
"query"is explicitly hardcoded toModelDataActionPayload, but theQUERYcategory maps toModelMonitoringActionPayload(line 83). SinceModelNodeActionTypealready has acategoryfield with corresponding mappings, route all actions through the category map instead of maintaining a parallel hardcoded name list. This eliminates the maintenance risk of name–routing mismatches and aligns with the architectural intent of the category registry.src/omnibase_core/models/core/model_metadata_tool_collection.py (1)
725-737: Critical: ToolCollection alias not exported from model_tool_collection.py causing import failures.The review comment correctly identifies a public API surface issue, but the actual problem is in
model_tool_collection.py, notmodel_metadata_tool_collection.py.Two files currently import
ToolCollectionfrommodel_tool_collection.py:
src/omnibase_core/models/configuration/model_onex_metadata.py:28src/omnibase_core/models/configuration/model_metadata_block.py:24However,
ToolCollection(defined as an alias inmodel_enhanced_tool_collection.py) is not included inmodel_tool_collection.py's__all__list. The__all__exports onlyModelToolCollectionbut not theToolCollectionalias, which will causeImportErrorat runtime for the downstream imports.Add
"ToolCollection"tomodel_tool_collection.py's__all__to resolve the breaking change.src/omnibase_core/models/node_metadata/model_node_status_types.py (1)
22-28: Missingstatus_typedefaulting to"active"can mask invalid payloads; also avoidisinstance.Right now, any dict/object missing
status_typeis treated as"active", which can let malformed inputs validate asModelNodeStatusActiveinstead of hitting your custom discriminator error. Also,isinstance(v, dict)conflicts with the “no isinstance; use duck typing” guideline. As per coding guidelines, prefer duck typing with try/except.Consider something like:
def get_node_status_discriminator(v: Any) -> str: """Extract discriminator value for node status union.""" - if isinstance(v, dict): - status_type = v.get("status_type", "active") - return str(status_type) # Ensure string type - return str(getattr(v, "status_type", "active")) # Ensure string type + try: + status_type = v.get("status_type") # duck-typed Mapping-like + except Exception: + status_type = getattr(v, "status_type", None) + + # If status_type is missing, return a sentinel to force discriminator validation failure + if status_type is None: + return "__missing_status_type__" + return str(status_type)If you intend backward-compat behavior (“missing => active”), please add an explicit comment explaining that choice and its implications.
src/omnibase_core/models/security/model_security_utils.py (1)
129-164: Return typelist[object]causes type safety issues downstream.The
list[object]return type mirrorsmask_dict_credentials()returningdict[str, object], but this design forces type-suppression in actual callers. Inmodel_external_service_config.py(line 155), the result requirestype: ignore[arg-type]to pass type checking, despite the comment confirming the content is semantically simple (str, int, bool, or recursively-masked structures).Use
list[Any]/dict[str, Any]instead to reflect the heterogeneous, JSON-like nature of masked output and eliminate type-checker friction for downstream code.
♻️ Duplicate comments (1)
src/omnibase_core/models/fsm/model_fsm_data_primary.py (1)
70-118: Validator type hints still useAny, violating strict mypy compliance.The field validators continue to use
Anyin their type hints (e.g.,list[Any] | tuple[Any, ...] | Any), which was flagged in previous reviews as violating the project's strict mypy compliance requirement.As per coding guidelines: "Enforce 100% strict mypy compliance - all functions must have type annotations with
disallow_untyped_defs = true"Based on learnings: The previous review at lines 70-118 provided specific type hint suggestions to remove the trailing
| Anyand use more specific types likelist[ModelFsmState | dict[str, object]] | tuple[ModelFsmState, ...].
🧹 Nitpick comments (12)
tests/unit/utils/test_safe_yaml_loader.py (1)
574-578: Make the malformed-YAML test more specific thanerror_codealone.
Right now it only assertsCONVERSION_ERROR, which could pass for unrelated conversion failures; adding a lightweight message assertion would reduce false positives and improve debuggability.with pytest.raises(ModelOnexError) as exc_info: extract_example_from_schema(schema_file) assert exc_info.value.error_code == EnumCoreErrorCode.CONVERSION_ERROR +assert "YAML parsing" in exc_info.value.messagesrc/omnibase_core/types/typed_dict/typed_dict_redacted_data.py (1)
8-16: Well-structured TypedDict addresses previous review concern.The TypedDict now provides meaningful type safety with three well-defined fields for redaction metadata. The previous concern about an empty TypedDict has been properly resolved.
The
redactedbool field has semantic value beyond being derivable from the count—it indicates whether redaction processing was applied, which is distinct from whether any fields were actually redacted.Optional enhancement for future versions: Consider adding
redacted_field_names: list[str]to track which specific fields were redacted. This would aid debugging and audit trails, but is not essential for MVP metadata tracking.src/omnibase_core/models/core/model_custom_filters.py (2)
69-83: Prefer reusing the same (private) union type forget_filterto avoid drift.
Once you have a single_CustomFilterValue(above),get_filtercan stay consistent and shorter.- def get_filter( - self, name: str - ) -> ( - Union[ - ModelStringFilter, - ModelNumericFilter, - ModelDateTimeFilter, - ModelListFilter, - ModelMetadataFilter, - ModelStatusFilter, - ModelComplexFilter, - ] - | None - ): + def get_filter(self, name: str) -> _CustomFilterValue | None: """Get a filter by name.""" return self.filters.get(name)
99-111:from_dictlocal annotation should also reuse the shared union type.
Keeps the three sites in lockstep and avoids subtle mismatches.- filters: dict[ - str, - Union[ - ModelStringFilter, - ModelNumericFilter, - ModelDateTimeFilter, - ModelListFilter, - ModelMetadataFilter, - ModelStatusFilter, - ModelComplexFilter, - ], - ] = {} + filters: dict[str, _CustomFilterValue] = {}src/omnibase_core/models/configuration/model_node_config_entry.py (1)
5-8: PreferTypeAliasforConfigValue(mypy/exports clarity).This is minor, but helps keep strict typing consistent across the repo.
-from pydantic import BaseModel, Field +from typing import TypeAlias + +from pydantic import BaseModel, Field @@ -ConfigValue = bool | int | float | str +ConfigValue: TypeAlias = bool | int | float | strsrc/omnibase_core/models/node_metadata/model_node_capability.py (1)
3-6:example_configtyping update looks consistent; optional: dropUnion+ considerMapping.If you want to simplify (Python 3.10+), this is equivalent and removes the
Unionimport:-from typing import Union +from collections.abc import Mapping @@ - example_config: ( - dict[ - str, - Union[ - ModelNodeConfigurationStringValue, ModelNodeConfigurationNumericValue - ], - ] - | None - ) = Field( + example_config: ( + Mapping[str, ModelNodeConfigurationStringValue | ModelNodeConfigurationNumericValue] + | None + ) = Field( default=None, description="Example configuration for this capability", )Also applies to: 23-26, 96-107
src/omnibase_core/models/fsm/model_fsm_transition_result.py (1)
143-162: Consider enforcing success/error consistency mentioned in docstringThe docstring (line 149) states "If success is False, error should typically be set", but
validate_instance()doesn't enforce this. Consider adding validation logic to match the documented behavior:def validate_instance(self) -> bool: """Validate instance integrity (ProtocolValidatable protocol). Performs validation to ensure required fields have valid values: - new_state must be a non-empty, non-whitespace string - old_state must be a non-empty, non-whitespace string - If success is False, error should typically be set Returns: bool: True if validation passed, False otherwise """ # Validate new_state is non-empty if not self.new_state or not self.new_state.strip(): return False # Validate old_state is non-empty if not self.old_state or not self.old_state.strip(): return False + + # Validate success/error consistency + if not self.success and not self.error: + return False return TrueAlternatively, update the docstring to clarify that success/error consistency is not enforced at the validation layer.
src/omnibase_core/models/fsm/model_fsm_transition_condition.py (1)
225-241: Redundant expression validation in frozen model.Since
model_validatoralready validates the 3-token format at construction time and the model is frozen (immutable), this check invalidate_instance()will never fail for a valid instance. Consider removing it or adding a comment explaining why it's kept for defensive purposes.- # Expression format is already validated by model_validator - # but we check token count here for explicit validation calls - tokens = self.expression.split() - if len(tokens) != 3: - if self.required: - raise ModelOnexError( - message=( - f"Expression must have exactly 3 tokens, " - f"got {len(tokens)}: '{self.expression}'" - ), - error_code=EnumCoreErrorCode.VALIDATION_ERROR, - context={ - "expression": self.expression, - "token_count": len(tokens), - }, - ) - return False - + # Expression format already validated by model_validator at construction. + # Model is frozen, so re-validation is unnecessary.src/omnibase_core/models/contracts/model_action_config_parameter.py (1)
99-103: Consider documenting theAnytype usage.The
default: Any | Nonefield type appears to contradict the docstring stating "No Any types allowed in public interface". This is pragmatic given the dynamic type validation, but consider adding a brief comment explaining whyAnyis necessary here (to accept all supported parameter types) while maintaining runtime type safety via the validator.tests/unit/models/contracts/subcontracts/test_model_fsm_transition_action.py (1)
167-167: Update comment to reflect immutable tuple type.The comment references "empty list" but should reference "empty tuple" to align with the deep immutability refactor where
action_configis now a tuple.Apply this diff:
- assert action.action_config == [] # Default is empty list + assert action.action_config == [] # Default is empty tuplesrc/omnibase_core/models/core/model_action_payload_types.py (1)
109-114: Inlineresult(optional) and consider asetfor membership checks.
resultis fine, but it’s one extra name; if you don’t need a debugger anchor, you canreturn payload_class(...). Also, the earliername in [...]checks are better assetliterals to avoid repeated linear scans (tiny, but cheap win).src/omnibase_core/models/core/model_metadata_tool_collection.py (1)
383-399: Tightened return types are good; consider a small typing cleanup to reduce duplication.
Both methods repeatdict[str, Union[ModelFunctionTool, dict[str, object]]]and still have comments referring to “ToolDataType” (which appears to be removed). Suggest introducing a local alias and using|to simplify, and update the stale comment.-from typing import Any, Union +from typing import Any +ToolData = ModelFunctionTool | dict[str, object] ... def get_tools_by_type( self, tool_type: EnumMetadataToolType -) -> dict[str, Union[ModelFunctionTool, dict[str, object]]]: +) -> dict[str, ToolData]: """Get all tools of a specific type.""" - # Tool data values can be ModelFunctionTool or raw dict - tools: dict[str, Union[ModelFunctionTool, dict[str, object]]] = {} + # Tool entries are either ModelFunctionTool or a raw dict fallback. + tools: dict[str, ToolData] = {} ... def get_tools_by_status( self, status: EnumMetadataToolStatus -) -> dict[str, Union[ModelFunctionTool, dict[str, object]]]: +) -> dict[str, ToolData]: """Get all tools with a specific status.""" - # Tool data values can be ModelFunctionTool or raw dict - tools: dict[str, Union[ModelFunctionTool, dict[str, object]]] = {} + # Tool entries are either ModelFunctionTool or a raw dict fallback. + tools: dict[str, ToolData] = {}Also applies to: 401-417
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (41)
src/omnibase_core/mixins/mixin_node_executor.py(1 hunks)src/omnibase_core/models/configuration/model_node_config_entry.py(1 hunks)src/omnibase_core/models/contracts/__init__.py(2 hunks)src/omnibase_core/models/contracts/model_action_config_parameter.py(1 hunks)src/omnibase_core/models/contracts/subcontracts/model_fsmtransitionaction.py(2 hunks)src/omnibase_core/models/core/model_action_config_value.py(2 hunks)src/omnibase_core/models/core/model_action_payload_types.py(3 hunks)src/omnibase_core/models/core/model_custom_filters.py(3 hunks)src/omnibase_core/models/core/model_metadata_tool_collection.py(3 hunks)src/omnibase_core/models/core/model_tool_collection.py(1 hunks)src/omnibase_core/models/examples/model_example.py(1 hunks)src/omnibase_core/models/fsm/__init__.py(1 hunks)src/omnibase_core/models/fsm/model_fsm_data.py(1 hunks)src/omnibase_core/models/fsm/model_fsm_data_primary.py(2 hunks)src/omnibase_core/models/fsm/model_fsm_operation.py(1 hunks)src/omnibase_core/models/fsm/model_fsm_state.py(3 hunks)src/omnibase_core/models/fsm/model_fsm_transition.py(2 hunks)src/omnibase_core/models/fsm/model_fsm_transition_action.py(1 hunks)src/omnibase_core/models/fsm/model_fsm_transition_condition.py(1 hunks)src/omnibase_core/models/fsm/model_fsm_transition_result.py(1 hunks)src/omnibase_core/models/node_metadata/model_node_capability.py(3 hunks)src/omnibase_core/models/node_metadata/model_node_configuration_value.py(1 hunks)src/omnibase_core/models/node_metadata/model_node_status_types.py(2 hunks)src/omnibase_core/models/operations/model_effect_result_types.py(1 hunks)src/omnibase_core/models/security/model_security_utils.py(5 hunks)src/omnibase_core/types/typed_dict/typed_dict_discovery_extended_stats.py(1 hunks)src/omnibase_core/types/typed_dict/typed_dict_node_executor_health.py(1 hunks)src/omnibase_core/types/typed_dict/typed_dict_redacted_data.py(1 hunks)src/omnibase_core/types/typed_dict/typed_dict_tool_execution_response.py(1 hunks)src/omnibase_core/types/typed_dict/typed_dict_workflow_step_config.py(1 hunks)src/omnibase_core/utils/compute_transformations.py(3 hunks)src/omnibase_core/validation/contracts.py(2 hunks)tests/unit/models/contracts/subcontracts/test_model_action_config_parameter.py(4 hunks)tests/unit/models/contracts/subcontracts/test_model_fsm_transition_action.py(6 hunks)tests/unit/models/fsm/test_model_fsm_transition_action.py(1 hunks)tests/unit/models/fsm/test_model_fsm_transition_condition.py(1 hunks)tests/unit/models/fsm/test_model_fsm_transition_result.py(1 hunks)tests/unit/utils/test_fsm_executor.py(2 hunks)tests/unit/utils/test_safe_yaml_loader.py(2 hunks)tests/unit/validation/test_contracts_extended.py(1 hunks)tests/unit/validation/test_fsm_analysis.py(0 hunks)
💤 Files with no reviewable changes (1)
- tests/unit/validation/test_fsm_analysis.py
🚧 Files skipped from review as they are similar to previous changes (8)
- src/omnibase_core/types/typed_dict/typed_dict_node_executor_health.py
- src/omnibase_core/models/contracts/init.py
- src/omnibase_core/types/typed_dict/typed_dict_workflow_step_config.py
- src/omnibase_core/types/typed_dict/typed_dict_discovery_extended_stats.py
- src/omnibase_core/types/typed_dict/typed_dict_tool_execution_response.py
- src/omnibase_core/models/fsm/init.py
- src/omnibase_core/models/fsm/model_fsm_transition_action.py
- src/omnibase_core/models/examples/model_example.py
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{py,txt}
📄 CodeRabbit inference engine (CLAUDE.md)
Always use Poetry for Python package management and task execution. Use
poetry runprefix for all commands (pytest, mypy, python, etc.) - never use pip or python directly
Files:
tests/unit/models/fsm/test_model_fsm_transition_result.pysrc/omnibase_core/models/fsm/model_fsm_operation.pysrc/omnibase_core/models/contracts/model_action_config_parameter.pysrc/omnibase_core/models/security/model_security_utils.pysrc/omnibase_core/types/typed_dict/typed_dict_redacted_data.pysrc/omnibase_core/mixins/mixin_node_executor.pysrc/omnibase_core/models/fsm/model_fsm_transition_condition.pytests/unit/models/fsm/test_model_fsm_transition_condition.pytests/unit/models/fsm/test_model_fsm_transition_action.pysrc/omnibase_core/models/operations/model_effect_result_types.pytests/unit/models/contracts/subcontracts/test_model_action_config_parameter.pytests/unit/utils/test_safe_yaml_loader.pytests/unit/validation/test_contracts_extended.pysrc/omnibase_core/models/fsm/model_fsm_state.pysrc/omnibase_core/models/fsm/model_fsm_data_primary.pysrc/omnibase_core/models/contracts/subcontracts/model_fsmtransitionaction.pysrc/omnibase_core/utils/compute_transformations.pysrc/omnibase_core/models/core/model_metadata_tool_collection.pysrc/omnibase_core/models/core/model_action_config_value.pysrc/omnibase_core/validation/contracts.pysrc/omnibase_core/models/configuration/model_node_config_entry.pysrc/omnibase_core/models/fsm/model_fsm_transition.pysrc/omnibase_core/models/node_metadata/model_node_status_types.pysrc/omnibase_core/models/core/model_custom_filters.pysrc/omnibase_core/models/core/model_tool_collection.pysrc/omnibase_core/models/node_metadata/model_node_capability.pytests/unit/utils/test_fsm_executor.pytests/unit/models/contracts/subcontracts/test_model_fsm_transition_action.pysrc/omnibase_core/models/node_metadata/model_node_configuration_value.pysrc/omnibase_core/models/fsm/model_fsm_transition_result.pysrc/omnibase_core/models/fsm/model_fsm_data.pysrc/omnibase_core/models/core/model_action_payload_types.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Maintain minimum 60% test coverage requirement - runpoetry run pytest tests/ --cov=src/omnibase_core --cov-report=term-missing
Run pytest with parallel workers for faster local testing:poetry run pytest tests/ -n 4(default: 4 workers, can disable with-n 0for debugging)
Use pytest markers to categorize tests: @pytest.mark.unit, @pytest.mark.integration, @pytest.mark.slow, @pytest.mark.smoke, @pytest.mark.performance
CI runs 20 parallel test splits with timeout protection (60s per test) - expected per-split runtime is 2m30s to 3m30s, critical alert if > 4m30s
Files:
tests/unit/models/fsm/test_model_fsm_transition_result.pytests/unit/models/fsm/test_model_fsm_transition_condition.pytests/unit/models/fsm/test_model_fsm_transition_action.pytests/unit/models/contracts/subcontracts/test_model_action_config_parameter.pytests/unit/utils/test_safe_yaml_loader.pytests/unit/validation/test_contracts_extended.pytests/unit/utils/test_fsm_executor.pytests/unit/models/contracts/subcontracts/test_model_fsm_transition_action.py
src/omnibase_core/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/omnibase_core/**/*.py: Use protocol names for dependency injection, never concrete class names. Example:container.get_service('ProtocolEventBus')not concrete class names
Always use ModelOnexError for error handling, never generic Exception. Include error_code from EnumCoreErrorCode and context information
Use @standard_error_handling decorator to eliminate 6+ lines of try/catch boilerplate in async operations
Enforce 100% strict mypy compliance - all functions must have type annotations withdisallow_untyped_defs = true
Use EnumNodeKind for architectural classification (EFFECT, COMPUTE, REDUCER, ORCHESTRATOR, RUNTIME_HOST) in workflow routing
Use EnumNodeType for specific implementation type discovery and capability matching (TRANSFORMER, AGGREGATOR, COMPUTE_GENERIC, etc.)
Format code using Black and sort imports using isort - runpoetry run black src/ tests/andpoetry run isort src/ tests/before commit
Run all pre-commit hooks before pushing:pre-commit run --all-files- includes mypy strict, black, isort, ruff
Use duck typing with protocols - never use isinstance checks. Protocols use structural subtyping, not explicit inheritance
Process inter-service communication using ModelEventEnvelope pattern for event-driven architecture
Use thread-local storage or separate node instances for concurrent thread access - ModelComputeCache, ModelCircuitBreaker, NodeCompute/NodeEffect are NOT thread-safe
Files:
src/omnibase_core/models/fsm/model_fsm_operation.pysrc/omnibase_core/models/contracts/model_action_config_parameter.pysrc/omnibase_core/models/security/model_security_utils.pysrc/omnibase_core/types/typed_dict/typed_dict_redacted_data.pysrc/omnibase_core/mixins/mixin_node_executor.pysrc/omnibase_core/models/fsm/model_fsm_transition_condition.pysrc/omnibase_core/models/operations/model_effect_result_types.pysrc/omnibase_core/models/fsm/model_fsm_state.pysrc/omnibase_core/models/fsm/model_fsm_data_primary.pysrc/omnibase_core/models/contracts/subcontracts/model_fsmtransitionaction.pysrc/omnibase_core/utils/compute_transformations.pysrc/omnibase_core/models/core/model_metadata_tool_collection.pysrc/omnibase_core/models/core/model_action_config_value.pysrc/omnibase_core/validation/contracts.pysrc/omnibase_core/models/configuration/model_node_config_entry.pysrc/omnibase_core/models/fsm/model_fsm_transition.pysrc/omnibase_core/models/node_metadata/model_node_status_types.pysrc/omnibase_core/models/core/model_custom_filters.pysrc/omnibase_core/models/core/model_tool_collection.pysrc/omnibase_core/models/node_metadata/model_node_capability.pysrc/omnibase_core/models/node_metadata/model_node_configuration_value.pysrc/omnibase_core/models/fsm/model_fsm_transition_result.pysrc/omnibase_core/models/fsm/model_fsm_data.pysrc/omnibase_core/models/core/model_action_payload_types.py
src/omnibase_core/models/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use Pydantic models for all data structures with v2.11+ compatibility, never use plain dataclasses or Python dicts for domain models
Files:
src/omnibase_core/models/fsm/model_fsm_operation.pysrc/omnibase_core/models/contracts/model_action_config_parameter.pysrc/omnibase_core/models/security/model_security_utils.pysrc/omnibase_core/models/fsm/model_fsm_transition_condition.pysrc/omnibase_core/models/operations/model_effect_result_types.pysrc/omnibase_core/models/fsm/model_fsm_state.pysrc/omnibase_core/models/fsm/model_fsm_data_primary.pysrc/omnibase_core/models/contracts/subcontracts/model_fsmtransitionaction.pysrc/omnibase_core/models/core/model_metadata_tool_collection.pysrc/omnibase_core/models/core/model_action_config_value.pysrc/omnibase_core/models/configuration/model_node_config_entry.pysrc/omnibase_core/models/fsm/model_fsm_transition.pysrc/omnibase_core/models/node_metadata/model_node_status_types.pysrc/omnibase_core/models/core/model_custom_filters.pysrc/omnibase_core/models/core/model_tool_collection.pysrc/omnibase_core/models/node_metadata/model_node_capability.pysrc/omnibase_core/models/node_metadata/model_node_configuration_value.pysrc/omnibase_core/models/fsm/model_fsm_transition_result.pysrc/omnibase_core/models/fsm/model_fsm_data.pysrc/omnibase_core/models/core/model_action_payload_types.py
🧠 Learnings (8)
📓 Common learnings
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-11T21:05:41.997Z
Learning: Applies to src/omnibase_core/nodes/node_reducer.py : Implement REDUCER nodes using NodeReducer base class with FSM-driven state transitions using ModelIntent pattern
📚 Learning: 2025-12-11T21:05:41.997Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-11T21:05:41.997Z
Learning: Applies to src/omnibase_core/**/*.py : Always use ModelOnexError for error handling, never generic Exception. Include error_code from EnumCoreErrorCode and context information
Applied to files:
tests/unit/utils/test_safe_yaml_loader.pysrc/omnibase_core/models/node_metadata/model_node_capability.pytests/unit/utils/test_fsm_executor.py
📚 Learning: 2025-12-11T21:05:41.997Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-11T21:05:41.997Z
Learning: Applies to src/omnibase_core/**/*.py : Enforce 100% strict mypy compliance - all functions must have type annotations with `disallow_untyped_defs = true`
Applied to files:
src/omnibase_core/models/fsm/model_fsm_data_primary.py
📚 Learning: 2025-12-11T21:05:41.997Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-11T21:05:41.997Z
Learning: Applies to src/omnibase_core/nodes/**/*.py : Use ModelONEXContainer for dependency injection in node constructors. Never confuse it with ModelContainer[T] which is a generic value wrapper
Applied to files:
src/omnibase_core/models/configuration/model_node_config_entry.pysrc/omnibase_core/models/node_metadata/model_node_status_types.pysrc/omnibase_core/models/node_metadata/model_node_capability.py
📚 Learning: 2025-12-11T21:05:41.997Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-11T21:05:41.997Z
Learning: Applies to src/omnibase_core/**/*.py : Use EnumNodeType for specific implementation type discovery and capability matching (TRANSFORMER, AGGREGATOR, COMPUTE_GENERIC, etc.)
Applied to files:
src/omnibase_core/models/node_metadata/model_node_status_types.pysrc/omnibase_core/models/node_metadata/model_node_capability.py
📚 Learning: 2025-12-11T21:05:41.997Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-11T21:05:41.997Z
Learning: Applies to src/omnibase_core/**/*.py : Use EnumNodeKind for architectural classification (EFFECT, COMPUTE, REDUCER, ORCHESTRATOR, RUNTIME_HOST) in workflow routing
Applied to files:
src/omnibase_core/models/node_metadata/model_node_status_types.py
📚 Learning: 2025-12-11T21:05:41.997Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-11T21:05:41.997Z
Learning: Applies to src/omnibase_core/models/**/*.py : Use Pydantic models for all data structures with v2.11+ compatibility, never use plain dataclasses or Python dicts for domain models
Applied to files:
src/omnibase_core/models/node_metadata/model_node_capability.py
📚 Learning: 2025-12-11T21:05:41.997Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-11T21:05:41.997Z
Learning: Applies to src/omnibase_core/nodes/node_reducer.py : Implement REDUCER nodes using NodeReducer base class with FSM-driven state transitions using ModelIntent pattern
Applied to files:
src/omnibase_core/models/fsm/model_fsm_transition_result.py
🧬 Code graph analysis (21)
tests/unit/models/fsm/test_model_fsm_transition_result.py (1)
src/omnibase_core/models/fsm/model_fsm_transition_result.py (3)
ModelFSMTransitionResult(24-162)validate_instance(143-162)serialize(135-141)
src/omnibase_core/models/fsm/model_fsm_operation.py (6)
src/omnibase_core/protocols/event_bus/protocol_event_bus_headers.py (1)
operation_name(85-87)src/omnibase_core/models/fsm/model_fsm_transition_action.py (3)
execute(159-180)serialize(182-188)validate_instance(190-220)src/omnibase_core/models/fsm/model_fsm_transition_condition.py (3)
execute(155-172)serialize(174-180)validate_instance(182-243)src/omnibase_core/models/fsm/model_fsm_state.py (3)
execute(110-118)serialize(120-122)validate_instance(124-136)src/omnibase_core/models/fsm/model_fsm_transition.py (3)
execute(82-90)serialize(92-94)validate_instance(96-116)src/omnibase_core/models/fsm/model_fsm_transition_result.py (2)
serialize(135-141)validate_instance(143-162)
src/omnibase_core/models/contracts/model_action_config_parameter.py (4)
src/omnibase_core/enums/enum_core_error_code.py (1)
EnumCoreErrorCode(9-163)src/omnibase_core/models/common/model_error_context.py (2)
ModelErrorContext(20-149)with_context(60-84)src/omnibase_core/models/common/model_schema_value.py (1)
ModelSchemaValue(22-347)src/omnibase_core/models/errors/model_onex_error.py (3)
ModelOnexError(51-356)message(250-252)error_code(255-257)
src/omnibase_core/mixins/mixin_node_executor.py (2)
src/omnibase_core/mixins/mixin_event_driven_node.py (1)
node_id(125-127)src/omnibase_core/mixins/mixin_node_setup.py (1)
node_id(67-68)
src/omnibase_core/models/fsm/model_fsm_transition_condition.py (5)
src/omnibase_core/enums/enum_core_error_code.py (1)
EnumCoreErrorCode(9-163)src/omnibase_core/models/errors/model_onex_error.py (2)
ModelOnexError(51-356)error_code(255-257)src/omnibase_core/protocols/event_bus/protocol_event_bus_headers.py (1)
retry_count(105-107)src/omnibase_core/models/fsm/model_fsm_state.py (3)
execute(110-118)serialize(120-122)validate_instance(124-136)src/omnibase_core/models/fsm/model_fsm_transition.py (3)
execute(82-90)serialize(92-94)validate_instance(96-116)
tests/unit/models/fsm/test_model_fsm_transition_action.py (1)
src/omnibase_core/models/fsm/model_fsm_transition_action.py (4)
ModelFSMTransitionAction(32-220)validate_instance(190-220)execute(159-180)serialize(182-188)
tests/unit/models/contracts/subcontracts/test_model_action_config_parameter.py (3)
src/omnibase_core/enums/enum_core_error_code.py (1)
EnumCoreErrorCode(9-163)src/omnibase_core/models/contracts/model_action_config_parameter.py (1)
ModelActionConfigParameter(42-199)src/omnibase_core/models/errors/model_onex_error.py (5)
ModelOnexError(51-356)error_code(255-257)message(250-252)model_dump_json(304-306)model_json_schema(349-356)
tests/unit/validation/test_contracts_extended.py (1)
src/omnibase_core/validation/contracts.py (1)
validate_yaml_file(61-128)
src/omnibase_core/models/fsm/model_fsm_state.py (5)
src/omnibase_core/models/fsm/model_fsm_transition_action.py (1)
execute(159-180)src/omnibase_core/models/fsm/model_fsm_transition_condition.py (1)
execute(155-172)src/omnibase_core/models/fsm/model_fsm_data_primary.py (1)
execute(169-177)src/omnibase_core/models/fsm/model_fsm_operation.py (1)
execute(98-124)src/omnibase_core/models/fsm/model_fsm_transition.py (1)
execute(82-90)
src/omnibase_core/models/contracts/subcontracts/model_fsmtransitionaction.py (1)
src/omnibase_core/models/contracts/model_action_config_parameter.py (1)
ModelActionConfigParameter(42-199)
src/omnibase_core/utils/compute_transformations.py (1)
src/omnibase_core/enums/enum_transformation_type.py (1)
EnumTransformationType(11-34)
src/omnibase_core/models/core/model_metadata_tool_collection.py (1)
src/omnibase_core/models/core/model_function_tool.py (1)
ModelFunctionTool(15-56)
src/omnibase_core/models/core/model_action_config_value.py (4)
src/omnibase_core/models/node_metadata/model_node_configuration_value.py (1)
from_value(95-114)src/omnibase_core/models/core/model_action_config_string_value.py (1)
ModelActionConfigStringValue(20-57)src/omnibase_core/models/core/model_action_config_numeric_value.py (1)
ModelActionConfigNumericValue(19-52)src/omnibase_core/models/core/model_action_config_boolean_value.py (1)
ModelActionConfigBooleanValue(17-46)
src/omnibase_core/models/configuration/model_node_config_entry.py (2)
src/omnibase_core/protocols/event_bus/protocol_event_message.py (1)
key(28-30)src/omnibase_core/models/core/model_extension_data.py (1)
value_type(92-108)
src/omnibase_core/models/fsm/model_fsm_transition.py (5)
src/omnibase_core/models/fsm/model_fsm_transition_action.py (1)
execute(159-180)src/omnibase_core/models/fsm/model_fsm_transition_condition.py (1)
execute(155-172)src/omnibase_core/models/fsm/model_fsm_data_primary.py (1)
execute(169-177)src/omnibase_core/models/fsm/model_fsm_operation.py (1)
execute(98-124)src/omnibase_core/models/fsm/model_fsm_state.py (1)
execute(110-118)
src/omnibase_core/models/core/model_tool_collection.py (1)
src/omnibase_core/models/core/model_metadata_tool_collection.py (1)
ModelMetadataToolCollection(44-714)
src/omnibase_core/models/node_metadata/model_node_capability.py (2)
src/omnibase_core/models/metadata/model_metadata_field_info.py (1)
uuid(161-180)src/omnibase_core/models/node_metadata/model_node_configuration_value.py (1)
ModelNodeConfigurationStringValue(27-54)
tests/unit/models/contracts/subcontracts/test_model_fsm_transition_action.py (3)
src/omnibase_core/models/contracts/model_action_config_parameter.py (1)
ModelActionConfigParameter(42-199)src/omnibase_core/models/contracts/subcontracts/model_fsmtransitionaction.py (1)
ModelFSMTransitionAction(23-101)src/omnibase_core/models/primitives/model_semver.py (1)
ModelSemVer(24-143)
src/omnibase_core/models/node_metadata/model_node_configuration_value.py (1)
src/omnibase_core/models/core/model_action_config_value.py (1)
from_value(73-99)
src/omnibase_core/models/fsm/model_fsm_transition_result.py (3)
src/omnibase_core/models/fsm/model_fsm_transition_condition.py (2)
serialize(174-180)validate_instance(182-243)src/omnibase_core/models/fsm/model_fsm_state.py (2)
serialize(120-122)validate_instance(124-136)src/omnibase_core/models/fsm/model_fsm_transition.py (2)
serialize(92-94)validate_instance(96-116)
src/omnibase_core/models/core/model_action_payload_types.py (1)
src/omnibase_core/models/core/model_node_action_type.py (1)
ModelNodeActionType(22-196)
⏰ 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: Smoke Tests
- GitHub Check: claude-review
🔇 Additional comments (47)
tests/unit/utils/test_safe_yaml_loader.py (1)
556-560: NOT_FOUND expectation for missing schema file is correct.
Good semantic tightening vs treating missing resources as INTERNAL_ERROR. Based on learnings, aligning onModelOnexError+EnumCoreErrorCodeis the right direction.src/omnibase_core/types/typed_dict/typed_dict_redacted_data.py (2)
1-6: LGTM: Clean imports and clear module docstring.The module structure follows Python best practices with future annotations for type hint compatibility and a clear, concise docstring.
19-19: LGTM: Correct public API export.The
__all__declaration properly exposes the TypedDict for public use, following Python module best practices.src/omnibase_core/utils/compute_transformations.py (2)
39-41: Import change is fine; keep typing consistent project-wide.
131-136: Docstring update is consistent with the new public typing surface.src/omnibase_core/mixins/mixin_node_executor.py (1)
246-246: LGTM: Consistent string coercion for node_idThe explicit
str()coercion ensures thenode_idfield in health reporting is always a string, which aligns with theTypedDictNodeExecutorHealthtype definition. This prevents type inconsistencies when_node_idis a UUID or other type across different node implementations.src/omnibase_core/validation/contracts.py (1)
20-20: LGTM: Enhanced Pydantic validation error handlingThe addition of
ValidationErrorhandling provides better error reporting for contract validation failures. The error message format is consistent with existing handlers, and the placement in the exception chain is appropriate.Also applies to: 111-113
tests/unit/validation/test_contracts_extended.py (1)
920-927: LGTM: Test updated to match implementationThe change from
RuntimeErrortoOSErrorcorrectly aligns with the actual error handling invalidate_yaml_file()(seecontracts.pylines 121-123), which catchesOSErrorfor file reading errors. The assertion now properly expects "OS error reading file" in the error message.src/omnibase_core/models/fsm/model_fsm_operation.py (1)
1-170: LGTM: Well-designed forward-compatibility modelThe
ModelFSMOperationimplementation follows ONEX patterns consistently:
- Previous review feedback properly addressed (min_length constraints, unused kwargs marking)
- Frozen model configuration ensures thread safety
- Protocol implementations (execute, serialize, validate_instance) align with other FSM models
- Documentation clearly indicates v1.1+ reserved status
src/omnibase_core/models/fsm/model_fsm_data.py (1)
8-8: LGTM: Import placement correctedThe
from __future__ import annotationsstatement is now correctly placed after the module docstring, following Python best practices.src/omnibase_core/models/contracts/subcontracts/model_fsmtransitionaction.py (1)
16-18: LGTM: Import path and attribute name aligned with refactorThe changes correctly align with the broader refactor:
- Import path updated to reference the relocated
ModelActionConfigParametermodule- Duplicate detection now uses
param.name(the public attribute) instead ofparam.parameter_nameBased on the relevant code snippets,
ModelActionConfigParameterusesnameas the field identifier (lines 41-198 in model_action_config_parameter.py).Also applies to: 86-88
tests/unit/models/fsm/test_model_fsm_transition_condition.py (1)
1-1120: LGTM: Comprehensive test coverage for ModelFSMTransitionConditionExcellent test suite covering:
- ✅ Instantiation with minimal/full data
- ✅ Field validation and type checking
- ✅ Protocol implementations (execute, serialize, validate_instance)
- ✅ Edge cases (whitespace, empty strings, special characters, operators)
- ✅ Serialization roundtrips (dict and JSON)
- ✅ Immutability and equality behaviors
- ✅ Import pathway verification
The test organization is clear with well-named test classes, and the whitespace/operator validation tests are particularly thorough.
src/omnibase_core/models/fsm/model_fsm_transition_condition.py (3)
1-19: LGTM!Clean module docstring with specification reference and appropriate imports. Uses
ModelOnexErrorandEnumCoreErrorCodeas per coding guidelines.
21-101: LGTM!Well-documented model with clear v1.0/v1.1+ distinction for reserved fields. Field constraints (
ge=0for retry_count,gt=0for timeout_ms) are appropriate, andVALID_OPERATORSas afrozensetensures immutability.
103-151: Expression parsing limitation with whitespace in values.The
split()method will fail for expressions with whitespace in values (e.g.,"name == 'hello world'"splits into 4+ tokens). If this is an intentional v1.0 limitation, consider documenting it explicitly in the docstring.Is this 3-token constraint intentional for v1.0, requiring values without whitespace? If quoted strings with spaces should be supported in future, consider reserving this for v1.1+.
tests/unit/models/fsm/test_model_fsm_transition_result.py (5)
1-26: LGTM!Excellent module docstring documenting the deep immutability pattern. Proper imports and use of
@pytest.mark.unitmarker as per coding guidelines.
27-99: LGTM!Thorough instantiation tests covering minimal, full, and failed transition scenarios. Correctly verifies immutable tuple defaults and metadata conversion.
101-329: LGTM!Comprehensive validation test coverage including Pydantic validation errors, type checking, and the
validate_instanceprotocol method for whitespace handling.
331-491: LGTM!Solid protocol and serialization tests with proper roundtrip verification.
493-714: LGTM!Excellent edge case coverage including special characters, very long strings, metadata sorting for deterministic hashing, frozen config validation, and import path verification.
src/omnibase_core/models/contracts/model_action_config_parameter.py (3)
1-40: LGTM!Well-structured module with clear documentation and appropriate type definitions. The
_TYPE_MAPPINGcorrectly handles thefloatacceptingintcase while maintaining strict typing for other types.
109-192: LGTM!Excellent type validation with proper handling of Python's
boolbeing a subclass ofint. The error context usingModelErrorContext.with_contextandModelSchemaValue.from_valuefollows established patterns.
194-202: LGTM!Appropriate model configuration with
frozen=Truefor immutability andvalidate_assignment=Truefor assignment validation. Clean__all__export.src/omnibase_core/models/core/model_action_config_value.py (3)
38-44: LGTM!Good use of Pydantic's
Discriminatorwith custom error handling for the discriminated union. Theget_action_config_discriminator_valuefunction properly handles both dict and model instance inputs.
73-99: LGTM!Correct implementation with proper type checking order (bool before int) and explicit Union return type. The fallback to string for unsupported types is a sensible default.
102-114: LGTM!Complete
__all__export list including all factory functions and the newModelActionConfigValueUnion.tests/unit/utils/test_fsm_executor.py (4)
18-24: LGTM!Import paths correctly updated to reflect model relocation to
omnibase_core.models.fsmmodule. AddedModelOnexErrorimport for error handling tests.
134-140: LGTM!
ModelFSMTransitionConditioncorrectly instantiated withoutversionfield, aligning with the MVP model pattern that doesn't requireModelSemVer.
185-191: LGTM!
ModelFSMTransitionActioncorrectly instantiated withoutversionfield, consistent with the MVP model pattern.
671-690: LGTM!
FSMState.historycorrectly updated to use tuples (()and("idle",)) instead of lists, aligning with the deep immutability pattern across FSM models.tests/unit/models/contracts/subcontracts/test_model_fsm_transition_action.py (1)
11-21: LGTM! Import path and version handling updated correctly.The import path change for
ModelActionConfigParameteraligns with the module restructuring, and introducing theDEFAULT_VERSIONconstant improves test clarity and consistency.src/omnibase_core/models/fsm/model_fsm_state.py (3)
7-70: Excellent deep immutability implementation with clear documentation.The enhanced documentation clearly explains the deep immutability pattern, provides usage examples for converting tuples back to dicts, and the field type changes are consistent throughout. The validator-based approach for automatic conversion is developer-friendly.
72-100: Validators correctly implement automatic immutability conversion.The validators properly convert incoming lists and dicts to immutable tuples, enabling a developer-friendly API where users can pass mutable types that are automatically frozen. The pattern is consistent and correct.
102-136: Protocol implementations are correct and well-documented.The
execute()method correctly implements v1.0 no-op behavior with clear documentation, andvalidate_instance()properly validates non-empty, non-whitespace names. The frozen configuration enforces immutability.src/omnibase_core/models/fsm/model_fsm_transition.py (2)
7-72: Deep immutability correctly implemented - past review concern resolved.The conversion to tuple-based fields addresses the previous review comment about mutable lists undermining the frozen guarantee. The implementation is consistent with
ModelFsmStateand properly documented. Themin_length=1constraints on string fields provide additional validation at construction time.
96-116: Validation correctly catches whitespace-only strings.The
validate_instance()implementation properly validates all three required string fields for non-whitespace content, complementing Pydantic'smin_length=1constraint to prevent whitespace-only values.src/omnibase_core/models/fsm/model_fsm_data_primary.py (2)
7-68: Field types correctly updated for deep immutability.The conversion of all collection fields to tuples is consistent with the broader immutability refactor. The documentation provides helpful examples for converting frozen fields back to dicts when needed for lookups.
120-199: Helper methods and protocol implementations are well-designed.The helper methods provide useful FSM querying capabilities with clear return types, and
validate_fsm_structure()offers comprehensive validation that returns actionable error messages. Protocol implementations are consistent with other FSM models.tests/unit/models/contracts/subcontracts/test_model_action_config_parameter.py (1)
1-913: Excellent comprehensive test coverage for ModelActionConfigParameter.The test suite is well-organized with logical grouping into 8 test classes covering:
- All parameter types (string, int, bool, float, list, dict)
- Field validation and constraints
- Default value type matching with proper error handling
- Immutability (frozen behavior)
- Serialization/deserialization round-trips
- Edge cases (unicode, special chars, nested structures)
- Documentation and schema validation
- Real-world use cases
The tests properly validate the new field schema (name, type, required, default) and ensure ModelOnexError is raised with appropriate error codes for type mismatches.
tests/unit/models/fsm/test_model_fsm_transition_action.py (2)
23-621: Comprehensive testing of deep immutability and protocol implementations.The tests thoroughly validate the deep immutability implementation:
- Confirms
action_configdefaults to empty tuple()- Verifies dict-to-tuple conversion (lines 60-64, 147-151)
- Tests frozen model behavior where
execute()accepts kwargs but doesn't mutate (lines 499-533)- Uses
model_construct()to bypass validation for edge case testing (lines 432-480)Protocol implementations are well-tested with both minimal and complex scenarios.
623-1105: Thorough edge case and API surface testing.The remaining test classes provide excellent coverage:
- Serialization round-trips (dict and JSON) with equality checks
- Edge cases: empty strings, special characters, negative values, large numbers
- Constraint validation (execution_order≥0, timeout_ms>0)
- Reserved fields for v1.1+ compatibility
- Import path verification from both package and module
- Documentation presence validation
The test suite achieves comprehensive coverage with clear test organization and proper use of pytest markers.
src/omnibase_core/models/core/model_metadata_tool_collection.py (1)
36-41: Import addition looks consistent with the compatibility aliases at the bottom.
ModelMetadataToolUsageMetricsis now imported and then re-exported/aliased later; this wiring looks coherent.src/omnibase_core/models/core/model_tool_collection.py (1)
22-34: Re-export façade looks consistent; verify consumer imports after removing legacy symbols.
The updated imports and__all__align with the “redirect to enhanced models” intent, and keeping bothMetadataTool*aliases andModelMetadataTool*types supports gradual migration. The main risk is downstream imports of removed names (handled by the repo-wide verification suggested above).Also applies to: 36-59
src/omnibase_core/models/node_metadata/model_node_status_types.py (2)
38-38:__all__ = []is correct; the symbols in this module are unused.The
NodeStatusDiscriminatorandget_node_status_discriminatorare defined in this module but are never imported or used anywhere in the codebase. While the file's structure suggests a discriminated union pattern was planned, it was never integrated—the status models (ModelNodeStatusActive,ModelNodeStatusError,ModelNodeStatusMaintenance) are used directly throughout the codebase and are properly exported at the package level innode_metadata/__init__.py. Exporting unused symbols would be poor practice. The empty__all__correctly reflects that this module is an internal helper and should not expose unintegrated code.
30-36: Thecustom_error_*parameters are supported in pydantic 2.12.5.Pydantic v2.12 exposes
custom_error_type,custom_error_message, andcustom_error_contexton both theField()andDiscriminator()constructors for discriminated unions. The code is correct for the pinned version.src/omnibase_core/models/security/model_security_utils.py (2)
5-5: Import change looks fine (keepsAnyavailable for public signatures).
80-126: Return typedict[str, object]is intentional and correct; the non-str key concern is theoretical and should be addressed differently.
Return type change is appropriate: Changing from
dict[str, Any]todict[str, object]is intentional per the FSM type alias removal commit and correctly represents a recursive masking result. The caller inmodel_external_service_config.pyalready acknowledges this narrower type and handles the mismatch explicitly with a type comment andtype: ignore[arg-type].Non-str key concern is theoretical: While technically possible to pass a
Mappingwith non-str keys at runtime (violating the type contract), the actual caller passesdict[str, Any]frommodel_dump(). The function correctly enforces its type contract via the parameter annotationMapping[str, Any].Suggested fix violates guidelines: The proposed
isinstance(key, str)check contradicts the project's duck-typing guideline ("never use isinstance checks"). Defensive validation for type-contract violations should use type-safe approaches instead—either enforce the contract with a concretedict[str, Any]parameter, or rely on the existing type annotations to catch violations at static analysis time.No changes needed; the implementation is correct as-is.
| class ModelNodeConfigEntry(BaseModel): | ||
| """ | ||
| Strongly-typed model for a node configuration entry. | ||
|
|
||
| This model represents a single configuration entry with its key, | ||
| type information, and default value. | ||
|
|
||
| Attributes: | ||
| key: Configuration key (e.g., "compute.max_parallel_workers") | ||
| value_type: Type name of the value ('int', 'float', 'bool', 'str') | ||
| default: Default value for this configuration | ||
| """ | ||
|
|
||
| key: str = Field( | ||
| ..., | ||
| description="Configuration key (e.g., 'compute.max_parallel_workers')", | ||
| ) | ||
| value_type: str = Field( | ||
| ..., | ||
| description="Type name of the value ('int', 'float', 'bool', 'str')", | ||
| ) | ||
| default: ConfigValue = Field( | ||
| ..., | ||
| description="Default value for this configuration", | ||
| ) | ||
|
|
||
| model_config = {"frozen": True} |
There was a problem hiding this comment.
value_type: str is too permissive; enforce allowed values + validate default matches.
Right now any string is accepted for value_type, and nothing prevents value_type="int" with default="abc".
-from pydantic import BaseModel, Field
+from typing import Literal, TypeAlias
+
+from pydantic import BaseModel, Field, model_validator
@@
-ConfigValue = bool | int | float | str
+ConfigValue: TypeAlias = bool | int | float | str
+NodeConfigValueType: TypeAlias = Literal["int", "float", "bool", "str"]
@@
- value_type: str = Field(
+ value_type: NodeConfigValueType = Field(
...,
description="Type name of the value ('int', 'float', 'bool', 'str')",
)
@@
model_config = {"frozen": True}
+
+ @model_validator(mode="after")
+ def _validate_default_matches_value_type(self) -> "ModelNodeConfigEntry":
+ vt = self.value_type
+ dv = self.default
+ ok = (
+ (vt == "bool" and isinstance(dv, bool))
+ or (vt == "int" and isinstance(dv, int) and not isinstance(dv, bool))
+ or (vt == "float" and isinstance(dv, float))
+ or (vt == "str" and isinstance(dv, str))
+ )
+ if not ok:
+ raise ValueError(
+ f"default type mismatch: value_type={vt!r}, default={dv!r} ({type(dv).__name__})"
+ )
+ return selfCommittable suggestion skipped: line range outside the PR's diff.
| # union-ok: discriminated_model_union - All filter types share filter_type discriminator field | ||
| filters: dict[ | ||
| str, | ||
| Union[ | ||
| ModelStringFilter, | ||
| ModelNumericFilter, | ||
| ModelDateTimeFilter, | ||
| ModelListFilter, | ||
| ModelMetadataFilter, | ||
| ModelStatusFilter, | ||
| ModelComplexFilter, | ||
| ], | ||
| ] = Field(default_factory=dict, description="Named custom filters") |
There was a problem hiding this comment.
Discriminator is mentioned but not enforced (Union validation can be ambiguous).
The comment says this is a discriminated union on filter_type, but the field type doesn’t declare a Pydantic discriminator; if any caller passes raw dicts into ModelCustomFilters(filters=...), union selection may become order-/shape-dependent.
-from typing import Any, Union
+from typing import Any, Annotated, TypeAlias, Union
from pydantic import BaseModel, Field
@@
class ModelCustomFilters(BaseModel):
@@
- # union-ok: discriminated_model_union - All filter types share filter_type discriminator field
- filters: dict[
- str,
- Union[
- ModelStringFilter,
- ModelNumericFilter,
- ModelDateTimeFilter,
- ModelListFilter,
- ModelMetadataFilter,
- ModelStatusFilter,
- ModelComplexFilter,
- ],
- ] = Field(default_factory=dict, description="Named custom filters")
+ _CustomFilterValue: TypeAlias = Annotated[
+ Union[
+ ModelStringFilter,
+ ModelNumericFilter,
+ ModelDateTimeFilter,
+ ModelListFilter,
+ ModelMetadataFilter,
+ ModelStatusFilter,
+ ModelComplexFilter,
+ ],
+ Field(discriminator="filter_type"),
+ ]
+
+ # union-ok: discriminated_model_union - All filter types share filter_type discriminator field
+ filters: dict[str, _CustomFilterValue] = Field(
+ default_factory=dict, description="Named custom filters"
+ )🤖 Prompt for AI Agents
In src/omnibase_core/models/core/model_custom_filters.py around lines 25-37, the
filters field comments claim a discriminated union but the Union lacks an
enforced Pydantic discriminator, so union resolution can be ambiguous; change
the field annotation to a Pydantic discriminated union (e.g., use
typing.Annotated[Union[...], Field(discriminator="filter_type")] or equivalent)
and import Annotated, and ensure each filter model declares a concrete Literal
filter_type value so Pydantic can unambiguously select the correct subtype at
validation time.
| # Transformation registry mapping type to handler | ||
| # TransformationHandler used because handlers have varying signatures | ||
| TRANSFORMATION_REGISTRY: dict[EnumTransformationType, TransformationHandler] = { | ||
| # Callable[..., Any] used because handlers have varying signatures | ||
| TRANSFORMATION_REGISTRY: dict[EnumTransformationType, Callable[..., Any]] = { | ||
| EnumTransformationType.IDENTITY: transform_identity, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, read the registry definition area
wc -l src/omnibase_core/utils/compute_transformations.pyRepository: OmniNode-ai/omnibase_core
Length of output: 123
🏁 Script executed:
#!/bin/bash
# Read the registry definition and surrounding code
head -n 500 src/omnibase_core/utils/compute_transformations.py | tail -n 100Repository: OmniNode-ai/omnibase_core
Length of output: 3825
🏁 Script executed:
#!/bin/bash
# Search for all handler function definitions
rg "^def transform_" src/omnibase_core/utils/compute_transformations.py -A 3Repository: OmniNode-ai/omnibase_core
Length of output: 964
🏁 Script executed:
#!/bin/bash
# Find execute_transformation function
rg "def execute_transformation" src/omnibase_core/utils/compute_transformations.py -A 15Repository: OmniNode-ai/omnibase_core
Length of output: 766
🏁 Script executed:
#!/bin/bash
# Check for mypy configuration
find . -name "pyproject.toml" -o -name "mypy.ini" -o -name ".mypy.ini" | head -5Repository: OmniNode-ai/omnibase_core
Length of output: 96
🏁 Script executed:
#!/bin/bash
# Check mypy configuration
cat mypy.iniRepository: OmniNode-ai/omnibase_core
Length of output: 2383
🏁 Script executed:
#!/bin/bash
# Read full execute_transformation implementation
rg "def execute_transformation" src/omnibase_core/utils/compute_transformations.py -A 40Repository: OmniNode-ai/omnibase_core
Length of output: 1969
🏁 Script executed:
#!/bin/bash
# Check how TRANSFORMATION_REGISTRY is used throughout the file
rg "TRANSFORMATION_REGISTRY" src/omnibase_core/utils/compute_transformations.py -B 2 -A 2Repository: OmniNode-ai/omnibase_core
Length of output: 803
🏁 Script executed:
#!/bin/bash
# Check if there are any other usages of TRANSFORMATION_REGISTRY in the codebase
rg "TRANSFORMATION_REGISTRY" --type py | grep -v "^src/omnibase_core/utils/compute_transformations.py"Repository: OmniNode-ai/omnibase_core
Length of output: 51
🏁 Script executed:
#!/bin/bash
# Get full execute_transformation with the actual handler invocation
sed -n '480,550p' src/omnibase_core/utils/compute_transformations.pyRepository: OmniNode-ai/omnibase_core
Length of output: 3178
🏁 Script executed:
#!/bin/bash
# Get more lines to see the actual handler invocation
sed -n '540,572p' src/omnibase_core/utils/compute_transformations.pyRepository: OmniNode-ai/omnibase_core
Length of output: 1263
Tighten registry handler typing to enforce uniform (data, config) call shape and correct comment accuracy.
Current Callable[..., Any] allows handlers with incompatible arity without mypy catching it, despite all handlers following a strict (data, config) call pattern. The comment claiming "varying signatures" is inaccurate—signatures vary only in config type, not call shape. With strict mypy enforcement enabled (disallow_untyped_defs = True, disallow_any_generics = True), tighten the registry typing to:
# Transformation registry mapping type to handler.
# Handlers share a uniform (data, config) call shape; config types vary by transformation.
TRANSFORMATION_REGISTRY: dict[EnumTransformationType, Callable[[Any, Any], Any]] = {
EnumTransformationType.IDENTITY: transform_identity,
EnumTransformationType.REGEX: transform_regex,
EnumTransformationType.CASE_CONVERSION: transform_case,
EnumTransformationType.TRIM: transform_trim,
EnumTransformationType.NORMALIZE_UNICODE: transform_unicode,
EnumTransformationType.JSON_PATH: transform_json_path,
}This enforces the 2-arg contract at the registry level and prevents silent drift. Individual handler implementations already have proper type specificity, so the Any placeholders in the registry type are acceptable for this dispatch table pattern.
🤖 Prompt for AI Agents
In src/omnibase_core/utils/compute_transformations.py around lines 471 to 474,
the TRANSFORMATION_REGISTRY uses a too-permissive Callable[..., Any] and an
incorrect comment claiming handlers have varying signatures; tighten the
registry typing to enforce the uniform two-argument call shape by changing the
type to Callable[[Any, Any], Any] and update the comment to state "Handlers
share a uniform (data, config) call shape; config types vary by transformation."
Also ensure the registry includes all transformation enum keys (IDENTITY, REGEX,
CASE_CONVERSION, TRIM, NORMALIZE_UNICODE, JSON_PATH) mapped to their handler
functions so the dispatch table matches the new typing contract.
87d1f98 to
417e52a
Compare
PR Review: FSM Contract Models for NodeReducer v1.0SummaryThis PR adds four new FSM contract models for NodeReducer v1.0 architecture with comprehensive test coverage (3,600+ tests added). The implementation demonstrates strong adherence to ONEX patterns including deep immutability, type safety, and protocol-driven design. ✅ Strengths1. Excellent Deep Immutability Implementation
2. Strong Type Safety
3. Comprehensive Test Coverage
4. Protocol Compliance
5. Code Organization
🟡 Minor Issues1. Validation ConsistencyLocation: The
Suggestion: Consider whether 2. Operator Validation PlacementLocation: The operator whitelist validation is in a 3. Field Validator Type HintsLocation: Multiple files Field validators like # Current (line 131-136 in model_fsm_transition_action.py)
def _convert_action_config_to_frozen(
cls,
v: (
dict[str, str | int | float | bool | tuple[str, ...] | None]
| tuple[tuple[str, str | int | float | bool | tuple[str, ...] | None], ...]
| None
),
) -> tuple[tuple[str, str | int | float | bool | tuple[str, ...] | None], ...]:
# Could use type alias at module level for readability
ActionConfigItem = tuple[str, str | int | float | bool | tuple[str, ...] | None]
ActionConfigInput = dict[str, ...] | tuple[ActionConfigItem, ...] | NoneThough I understand this conflicts with the "no type aliases" rule. Perhaps the rule should allow internal/private type aliases for complex signatures? 4. UTC vs timezone-aware datetimeGood catch in commit 8dbcbb9: Changed 🟢 Best Practices Followed
🔍 Security & PerformanceNo concerns identified:
📊 CI/CD Considerations
📝 DocumentationExcellent documentation:
🎯 RecommendationAPPROVE with minor suggestions This is high-quality work that follows ONEX patterns meticulously. The deep immutability implementation is particularly well done. The minor issues noted are truly minor and don't block merging. Suggested follow-ups (can be separate PRs):
Review completed: 2025-12-13 |
…578, OMN-581] Add support for standard operators (==, !=, >, <, >=, <=, in, not_in, contains, matches) in fsm_executor.py to match ModelFSMTransitionCondition validator whitelist. Remove unused Sized import. Update test fixture to use valid >= operator instead of invalid min_length.
Code Review - PR #165: FSM Contract Models for NodeReducer v1.0This is a substantial and well-executed PR that adds foundational FSM contract models for the NodeReducer v1.0 architecture. The code quality is excellent overall, with comprehensive testing and adherence to ONEX patterns. Below is my detailed review. ✅ Strengths1. Excellent Code Quality
2. Comprehensive Testing
3. Strong Validation Logic
4. Excellent Documentation
5. Proper Protocol Implementations
6. Code Organization
🟡 Minor Issues (Low Priority)1. ConfigDict Usage ✅ ADDRESSED
2. Timezone-Aware Timestamps ✅ ADDRESSED
3. Metadata Sorting ✅ ADDRESSED
4. Union Type Ordering ✅ ADDRESSED
📊 Test Coverage AnalysisTotal Tests: 309 FSM tests (from PR description: 214 initially, expanded to 309) Test Distribution:
Coverage Quality: Excellent
🔒 Security ConsiderationsNo security issues identified:
🚀 Performance ConsiderationsExcellent performance characteristics:
Potential optimization (future consideration):
📋 Architecture AlignmentExcellent alignment with ONEX patterns:
Code Quality Standards:
🎯 Recommendations1. Merge StatusREADY TO MERGE ✅ All issues have been addressed in the latest commits. The PR demonstrates:
2. Future Enhancements (Not blocking for merge)v1.1+ Considerations:
Documentation:
📝 Commit Quality AssessmentExcellent commit history:
Notable commits:
🏆 Final VerdictScore: 9.5/10 ⭐⭐⭐⭐⭐ This is an exemplary PR that sets a high standard for the codebase. The author has:
Minor deductions:
Recommendation: ✅ APPROVE AND MERGE Great work, @jonahgabriel! This PR significantly strengthens the FSM contract foundation for NodeReducer v1.0. The attention to detail, comprehensive testing, and adherence to ONEX patterns is outstanding. Review generated by Claude Code (Sonnet 4.5) on 2025-12-13 |
1 participant
… OMN-579, OMN-580, OMN-581]
Add four new MVP FSM contract models for the NodeReducer v1.0 contract-driven architecture. Models use 'Mvp' prefix to differentiate from existing subcontract models which have different structure (require version: ModelSemVer).
New models:
Technical notes:
Summary by CodeRabbit
New Features
Improvements
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.