Repository navigation
feat(contracts): upgrade ModelContractReducer to v1.5.0 schema [OMN-648] - #148
jonahgabriel merged 6 commits into
Conversation
- Add `fingerprint` field with regex validation (<semver>:<sha256_12>) - Rename `state_transitions` to `state_machine` with alias support - Add `frozen=True` to ModelContractReducer for thread safety - Add `frozen=True` to ModelFSMSubcontract for immutability - Add `populate_by_name=True` for YAML alias flexibility - Update INTERFACE_VERSION to 1.5.0 for both models - Add comprehensive test suite (50 tests) for v1.5.0 features This is a prerequisite for OMN-260 (contract upgrades) and OMN-257 (validation).
WalkthroughAdded contract fingerprinting and renamed FSM field to Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
Code Review - PR #148: ModelContractReducer v1.5.0 Schema UpgradeThank you for this well-structured PR! This is a solid implementation of the v1.5.0 schema upgrade. Below is my comprehensive review: ✅ Strengths1. Excellent Test Coverage
2. Backwards Compatibility
3. Thread Safety & Immutability
4. Clear Documentation
|
| Item | Status | Notes |
|---|---|---|
| Follows CLAUDE.md conventions | ✅ | Correct use of Poetry, ModelOnexError, etc. |
| Type hints complete | ✅ | All signatures properly typed |
| Error handling | ✅ | Uses ModelOnexError with proper error codes |
| Documentation | ✅ | Clear docstrings and comments |
| Test coverage | ✅ | 50 tests covering all changes |
| Backwards compatibility | ✅ | Alias pattern preserves existing contracts |
| Thread safety | ✅ | frozen=True applied correctly |
| Mypy strict compliance | Needs verification (PR says passes) |
🎯 Recommendations Summary
Must Fix:
- Restore
validate_assignment=TrueinModelFSMSubcontract.model_configfor consistency withModelContractReducer
Should Consider:
- Clarify fingerprint description to say "12-char-sha256-prefix" for precision
- Verify mypy passes on modified files (as claimed in PR description)
Nice to Have:
- Consider simplifying test method names (remove "fingerprint" redundancy in class
TestModelContractReducerFingerprint)
✅ Overall Assessment
This is high-quality work that correctly implements the v1.5.0 schema requirements:
- ✅ Fingerprint validation with proper regex
- ✅ Backwards-compatible state_machine alias
- ✅ Thread-safe frozen models
- ✅ Comprehensive test coverage
With the validate_assignment fix, this PR is ready to merge.
📚 References
- CLAUDE.md: Thread Safety section (confirms frozen=True requirement)
- ModelContractFingerprint: Shows fingerprint format standards
- Project conventions: Consistent with existing contract models
Great work on this foundational upgrade! 🚀
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/omnibase_core/models/contracts/model_contract_reducer.py (1)
199-205: state_machine alias handling and subcontract validation path look correct; minor wording nitUsing
state_machine: ModelFSMSubcontract | Nonewithalias="state_transitions"pluspopulate_by_name=Truegives you the desired backwards compatibility for existing YAML while keeping the new field name in Python code. The updated checks invalidate_node_specific_configandvalidate_subcontract_constraintscorrectly gate FSM validation onstate_machineand accept either"state_machine"or"state_transitions"in incoming contract data, and_validate_fsm_subcontractnow safely handles aNoneFSM.One minor nit: the docstring comment above
validate_subcontract_constraintsstill mentions “state_transitions subcontracts”, which could now be updated to say “state_machine (alias: state_transitions) subcontracts” for consistency with the new field name and error messages.Based on learnings, this keeps the reducer contract purely declarative and does not mix in FSM transition logic, so it remains aligned with the ModelIntent-based FSM pattern in reducer nodes.
Also applies to: 330-336, 358-369, 400-402
tests/unit/models/contracts/test_model_contract_reducer_v150.py (2)
169-310: state_machine alias and populate_by_name behavior are well-covered; optional from_yaml checkThe alias-focused tests (
state_machinevsstate_transitions) and the_create_minimal_fsm_subcontracthelper do a good job verifying that both the field name and alias populatestate_machinecorrectly and that FSM configuration is preserved.If you want extra safety around the YAML boundary, consider adding a small test that runs a reducer contract with only
state_transitionsthroughModelContractReducer.from_yaml(...)to assert that alias-based population also works via the YAML helper, not justmodel_validate. That would fully exercise the intended backwards-compatibility path.
311-407: Frozen behavior tests are thorough; hashability assertion may be brittle against library changesThe frozen-behavior tests around field mutation (including
correlation_idandnode_name) give solid coverage that the immutability contract is enforced at runtime.The one potentially brittle spot is
test_frozen_model_not_hashable_with_nested_mutable_objects, which asserts aTypeErroronhash(contract)based on current library behavior and an implementation detail (nested mutable objects). Unless “must not be hashable” is an explicit part of your public contract, you might consider loosening or dropping that assertion so the test suite doesn’t start failing if the underlying model library later makes frozen models hashable without breaking immutability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/omnibase_core/models/contracts/model_contract_reducer.py(7 hunks)src/omnibase_core/models/contracts/subcontracts/model_fsm_subcontract.py(2 hunks)tests/unit/models/contracts/test_model_contract_reducer_v150.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Always use Poetry for all Python package management and task execution - never use pip directly or execute python/pytest withoutpoetry runprefix
Format all Python code with black and sort imports with isort - maintain consistent code formatting across the project
Files:
src/omnibase_core/models/contracts/subcontracts/model_fsm_subcontract.pytests/unit/models/contracts/test_model_contract_reducer_v150.pysrc/omnibase_core/models/contracts/model_contract_reducer.py
src/omnibase_core/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/omnibase_core/**/*.py: Use structured error handling withModelOnexErrorandEnumCoreErrorCode- never raise genericException
Use the@standard_error_handlingdecorator to eliminate 6+ lines of try/catch boilerplate in async operations
Use protocol-based dependency injection withcontainer.get_service("ProtocolName")- never use concrete class names or isinstance checks with protocols
Type check all Python code with strict mypy (100% strict mode compliance required) - all functions must have type annotations and disallow Any return types
Use ModelEventEnvelope for inter-service event-driven communication - wrap event data in standardized envelope pattern
All source files must achieve 100% strict mypy compliance - no Any types allowed, all function parameters and return types must be explicitly annotated
Document all public APIs, classes, and functions with docstrings explaining purpose, parameters, return values, and usage examples
Never use generic Exception - always use ModelOnexError with specific EnumCoreErrorCode for error handling consistency
Use EnumNodeKind for high-level architectural routing (EFFECT, COMPUTE, REDUCER, ORCHESTRATOR, RUNTIME_HOST) - use EnumNodeType for specific implementation discovery
Files:
src/omnibase_core/models/contracts/subcontracts/model_fsm_subcontract.pysrc/omnibase_core/models/contracts/model_contract_reducer.py
src/omnibase_core/models/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use Pydantic 2.11+ models for all data structures with strict validation enabled
Files:
src/omnibase_core/models/contracts/subcontracts/model_fsm_subcontract.pysrc/omnibase_core/models/contracts/model_contract_reducer.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Write comprehensive test coverage with minimum 60% coverage requirement - organize tests in unit/ and integration/ directories with appropriate markers
Use appropriate pytest markers (@pytest.mark.unit, @pytest.mark.integration, @pytest.mark.slow, @pytest.mark.performance) to categorize tests
Files:
tests/unit/models/contracts/test_model_contract_reducer_v150.py
src/omnibase_core/**/*reducer*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/omnibase_core/**/*reducer*.py: NodeReducer and NodeOrchestrator are the primary implementations for v0.4.0+ - import fromomnibase_core.nodes(not deprecated 'Declarative' versions)
Implement FSM transitions using ModelIntent pattern in REDUCER nodes - keep state transition logic separate from side effects
Files:
src/omnibase_core/models/contracts/model_contract_reducer.py
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-10T19:23:31.049Z
Learning: Applies to src/omnibase_core/**/*reducer*.py : Implement FSM transitions using ModelIntent pattern in REDUCER nodes - keep state transition logic separate from side effects
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-10T19:23:31.049Z
Learning: Applies to src/omnibase_core/**/*reducer*.py : NodeReducer and NodeOrchestrator are the primary implementations for v0.4.0+ - import from `omnibase_core.nodes` (not deprecated 'Declarative' versions)
📚 Learning: 2025-12-10T19:23:31.049Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-10T19:23:31.049Z
Learning: Applies to src/omnibase_core/**/*reducer*.py : Implement FSM transitions using ModelIntent pattern in REDUCER nodes - keep state transition logic separate from side effects
Applied to files:
src/omnibase_core/models/contracts/model_contract_reducer.py
🧬 Code graph analysis (2)
tests/unit/models/contracts/test_model_contract_reducer_v150.py (3)
src/omnibase_core/models/contracts/model_contract_reducer.py (1)
to_yaml(480-495)src/omnibase_core/models/contracts/subcontracts/model_fsm_subcontract.py (1)
ModelFSMSubcontract(35-279)src/omnibase_core/models/primitives/model_semver.py (1)
ModelSemVer(24-143)
src/omnibase_core/models/contracts/model_contract_reducer.py (2)
src/omnibase_core/models/primitives/model_semver.py (1)
ModelSemVer(24-143)src/omnibase_core/models/contracts/subcontracts/model_fsm_subcontract.py (1)
ModelFSMSubcontract(35-279)
⏰ 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 (5)
src/omnibase_core/models/contracts/subcontracts/model_fsm_subcontract.py (1)
47-47: Interface version bump and frozen config align with v1.5.0 reducer usage
INTERFACE_VERSIONset to1.5.0andmodel_configupdated withfrozen=Trueare consistent with the reducer contract’s new interface version and immutability semantics; they also match the new tests that assert FSM subcontract immutability and version. No changes requested.Also applies to: 275-278
src/omnibase_core/models/contracts/model_contract_reducer.py (2)
77-77: Fingerprint field and interface version upgrade are coherent and well-scopedThe
INTERFACE_VERSIONbump to1.5.0and the new optionalfingerprintfield with the<semver>:<sha256_12>pattern are narrowly scoped and match the documented format and tests. The regex cleanly enforces the expected shape while allowingNonefor backwards compatibility. No changes requested.Also applies to: 91-96
470-478: Model config flags (frozen + populate_by_name) match the intended contract semanticsThe
model_configupdates (frozen=Truefor immutability andpopulate_by_name=Trueto accept bothstate_machineand its alias) line up with the new tests and with the goal of thread-safe, read-only contract instances that can still ingest legacy YAML. The other config values (extra="forbid",use_enum_values=False,validate_assignment,str_strip_whitespace,validate_default) remain consistent with strict validation expectations. No changes requested.tests/unit/models/contracts/test_model_contract_reducer_v150.py (2)
42-167: Helper factories and fingerprint tests give strong coverage of the new field
get_minimal_reducer_contract_data/create_minimal_reducer_contractkeep the tests DRY and focused, and the fingerprint test class exercises a wide range of valid and invalid patterns (including edge cases like empty strings, uppercase hex, wrong semver shape, and missing colon). This should make regressions in the regex or field optionality very visible. No changes requested.
409-531: Version, model_config, and serialization tests accurately pin the v1.5.0 contract surfaceThe remaining test classes (interface version checks, FSM subcontract version and frozen config,
extra="forbid",populate_by_name, enum retention, and fingerprint presence inmodel_dump/to_yaml) neatly lock in the intended public surface for v1.5.0. Together they provide good guardrails against accidental interface or config drift. No changes requested.
…648] - Enhance ModelContractReducer module and class docstrings with v1.5.0 details - Document fingerprint field format (<semver>:<sha256_12>) and purpose - Document state_machine alias mechanism for YAML flexibility - Explain frozen=True thread safety and immutability guarantees - Enhance ModelFSMSubcontract docstrings with immutability documentation - Add Google-style docstrings to all model validators Addresses PR #148 review feedback for clear documentation.
Code Review: ModelContractReducer v1.5.0 Schema Upgrade✅ Overall AssessmentThis is a well-executed schema upgrade with excellent documentation, comprehensive testing, and careful attention to backwards compatibility. The changes align well with the omnibase_core architecture and follow project conventions. 🎯 Strengths1. Comprehensive Test Coverage (50 tests)
2. Documentation Quality
3. Backwards Compatibility
4. Type Safety
5. Thread Safety
🔍 Code Quality ObservationsModelContractReducer Changes: ✅ Fingerprint Field (lines 114-124) fingerprint: str | None = Field(
default=None,
pattern=r"^\d+\.\d+\.\d+:[a-f0-9]{12}$",
...
)
✅ State Machine Alias (lines 230-240) state_machine: ModelFSMSubcontract | None = Field(
default=None,
alias="state_transitions",
...
)
✅ Model Config (lines 512-520) model_config = ConfigDict(
extra="forbid",
frozen=True, # v1.5.0
populate_by_name=True, # v1.5.0
...
)
✅ Validation Logic Updated (lines 365-367, 395-404)
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/omnibase_core/models/contracts/model_contract_reducer.py (1)
512-520: Clarify the interaction betweenfrozen=Trueandvalidate_assignment=True.With
frozen=True, any field assignment attempt raises aValidationErrorimmediately, makingvalidate_assignment=Trueeffectively redundant. The comment on line 516 acknowledges this ("triggers error if frozen"), but the combination may confuse future maintainers.Consider removing
validate_assignment=Truesincefrozen=Truealready prevents all mutations, or updating the comment to explicitly note the redundancy is intentional for defensive purposes.model_config = ConfigDict( extra="forbid", # Strict validation - reject unknown fields frozen=True, # Thread safety and immutability - instances cannot be modified use_enum_values=False, # Keep enum objects, don't convert to strings - validate_assignment=True, # Validate on any assignment attempt (triggers error if frozen) str_strip_whitespace=True, # Clean string inputs validate_default=True, # Validate default values at model definition time populate_by_name=True, # Allow both field name and alias for YAML flexibility )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/omnibase_core/models/contracts/model_contract_reducer.py(8 hunks)src/omnibase_core/models/contracts/subcontracts/model_fsm_subcontract.py(6 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Always use Poetry for all Python package management and task execution - never use pip directly or execute python/pytest withoutpoetry runprefix
Format all Python code with black and sort imports with isort - maintain consistent code formatting across the project
Files:
src/omnibase_core/models/contracts/model_contract_reducer.pysrc/omnibase_core/models/contracts/subcontracts/model_fsm_subcontract.py
src/omnibase_core/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/omnibase_core/**/*.py: Use structured error handling withModelOnexErrorandEnumCoreErrorCode- never raise genericException
Use the@standard_error_handlingdecorator to eliminate 6+ lines of try/catch boilerplate in async operations
Use protocol-based dependency injection withcontainer.get_service("ProtocolName")- never use concrete class names or isinstance checks with protocols
Type check all Python code with strict mypy (100% strict mode compliance required) - all functions must have type annotations and disallow Any return types
Use ModelEventEnvelope for inter-service event-driven communication - wrap event data in standardized envelope pattern
All source files must achieve 100% strict mypy compliance - no Any types allowed, all function parameters and return types must be explicitly annotated
Document all public APIs, classes, and functions with docstrings explaining purpose, parameters, return values, and usage examples
Never use generic Exception - always use ModelOnexError with specific EnumCoreErrorCode for error handling consistency
Use EnumNodeKind for high-level architectural routing (EFFECT, COMPUTE, REDUCER, ORCHESTRATOR, RUNTIME_HOST) - use EnumNodeType for specific implementation discovery
Files:
src/omnibase_core/models/contracts/model_contract_reducer.pysrc/omnibase_core/models/contracts/subcontracts/model_fsm_subcontract.py
src/omnibase_core/**/*reducer*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/omnibase_core/**/*reducer*.py: NodeReducer and NodeOrchestrator are the primary implementations for v0.4.0+ - import fromomnibase_core.nodes(not deprecated 'Declarative' versions)
Implement FSM transitions using ModelIntent pattern in REDUCER nodes - keep state transition logic separate from side effects
Files:
src/omnibase_core/models/contracts/model_contract_reducer.py
src/omnibase_core/models/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use Pydantic 2.11+ models for all data structures with strict validation enabled
Files:
src/omnibase_core/models/contracts/model_contract_reducer.pysrc/omnibase_core/models/contracts/subcontracts/model_fsm_subcontract.py
🧠 Learnings (3)
📓 Common learnings
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-10T19:23:31.049Z
Learning: Applies to src/omnibase_core/**/*reducer*.py : Implement FSM transitions using ModelIntent pattern in REDUCER nodes - keep state transition logic separate from side effects
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-10T19:23:31.049Z
Learning: Applies to src/omnibase_core/**/*reducer*.py : NodeReducer and NodeOrchestrator are the primary implementations for v0.4.0+ - import from `omnibase_core.nodes` (not deprecated 'Declarative' versions)
📚 Learning: 2025-12-10T19:23:31.049Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-10T19:23:31.049Z
Learning: Applies to src/omnibase_core/**/*reducer*.py : Implement FSM transitions using ModelIntent pattern in REDUCER nodes - keep state transition logic separate from side effects
Applied to files:
src/omnibase_core/models/contracts/model_contract_reducer.pysrc/omnibase_core/models/contracts/subcontracts/model_fsm_subcontract.py
📚 Learning: 2025-12-10T19:23:31.049Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-10T19:23:31.049Z
Learning: Applies to src/omnibase_core/**/*reducer*.py : NodeReducer and NodeOrchestrator are the primary implementations for v0.4.0+ - import from `omnibase_core.nodes` (not deprecated 'Declarative' versions)
Applied to files:
src/omnibase_core/models/contracts/model_contract_reducer.py
🧬 Code graph analysis (2)
src/omnibase_core/models/contracts/model_contract_reducer.py (2)
src/omnibase_core/models/primitives/model_semver.py (1)
ModelSemVer(24-143)src/omnibase_core/models/contracts/subcontracts/model_fsm_subcontract.py (1)
ModelFSMSubcontract(39-330)
src/omnibase_core/models/contracts/subcontracts/model_fsm_subcontract.py (1)
src/omnibase_core/models/primitives/model_semver.py (1)
ModelSemVer(24-143)
⏰ 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 (6)
src/omnibase_core/models/contracts/subcontracts/model_fsm_subcontract.py (2)
47-64: LGTM! Clean documentation of immutability semantics.The docstring correctly explains that validators run during construction before the instance is frozen, and the INTERFACE_VERSION is properly bumped to 1.5.0 to align with the schema upgrade.
326-329: LGTM! Appropriate use of frozen=True for thread safety.The
frozen=Trueconfiguration correctly provides immutability guarantees after construction. Note that this removes the ability to usevalidate_assignment, but since frozen models reject all assignments anyway, this is the correct design choice for thread-safe immutable contracts.src/omnibase_core/models/contracts/model_contract_reducer.py (4)
110-124: LGTM! Well-designed fingerprint field for drift detection.The regex pattern
^\d+\.\d+\.\d+:[a-f0-9]{12}$enforces a canonical format with lowercase hex characters. The optional nature (Nonedefault) appropriately supports both new contracts without fingerprints and versioned contracts with drift detection enabled.
227-240: LGTM! Good backward compatibility approach with field aliasing.The rename from
state_transitionstostate_machinewithalias="state_transitions"combined withpopulate_by_name=Truein model_config enables YAML contracts to use either field name. This maintains backward compatibility while establishing the preferred v1.5.0 naming convention.
393-404: LGTM! Correct handling of both field name and alias in validation.The check for both
"state_machine"and"state_transitions"incontract_datacorrectly handles:
- Raw YAML input using the legacy
state_transitionskeymodel_dump()output which always uses the field namestate_machineThis ensures subcontract constraint validation works regardless of the source.
435-437: LGTM! Correctly updated to use the new field name.The
_validate_fsm_subcontractmethod now reads fromself.state_machinewhich aligns with the renamed field. The early return onNoneis appropriate.
…648] Document why ModelFSMSubcontract omits validate_assignment while ModelContractReducer includes it - both have identical behavior due to frozen=True, but the explicit flag is kept for documentation clarity.
Code Review - PR #148: ModelContractReducer v1.5.0 Schema UpgradeOverviewThis PR upgrades ✅ Strengths1. Excellent Test Coverage
2. Backwards Compatibility
3. Thread Safety & Immutability
4. Fingerprint Validation
5. Documentation Quality
|
| Category | Rating | Notes |
|---|---|---|
| Architecture | ⭐⭐⭐⭐⭐ | Follows ONEX patterns perfectly |
| Type Safety | ⭐⭐⭐⭐⭐ | Strict typing, mypy compliant |
| Testing | ⭐⭐⭐⭐⭐ | 50 tests, excellent coverage |
| Documentation | ⭐⭐⭐⭐⭐ | Clear docstrings and comments |
| Backwards Compat | ⭐⭐⭐⭐⭐ | Alias support for legacy contracts |
| Thread Safety | ⭐⭐⭐⭐⭐ | Frozen models, well-documented |
| Consistency | ⭐⭐⭐⭐ | Minor inconsistency with validate_assignment |
🎯 Recommendation: APPROVE with Minor Suggestions
This is excellent work that properly implements the v1.5.0 schema upgrade. The only recommendation is to add validate_assignment=True to ModelFSMSubcontract for consistency with the rest of the codebase.
Priority Actions:
- Optional: Add
validate_assignment=TruetoModelFSMSubcontract.model_config(consistency fix) - Optional: Add test for simultaneous state_machine + state_transitions in YAML
- Optional: Add comment in
_validate_fsm_subcontractexplaining intentional defense-in-depth validation
None of these are blockers - the PR is ready to merge as-is if you prefer.
📊 Test Execution
The 50 new tests should integrate smoothly with the existing 12,000+ test suite. Expected impact:
- CI runtime: Minimal (less than 5 seconds across 20 splits)
- Coverage: Maintains 60%+ requirement
- No conflicts with existing reducer tests
Reviewed by: Claude Sonnet 4.5
Review Date: 2025-12-10
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/omnibase_core/models/contracts/subcontracts/model_fsm_subcontract.py (1)
326-330: Consider removing the comparison to ModelContractReducer.The note comparing to ModelContractReducer's configuration may confuse readers. The explanation that frozen=True already prevents modifications is sufficient without the comparison.
Apply this diff to simplify the note:
- # Note: validate_assignment is intentionally omitted here because: - # 1. frozen=True already prevents all attribute modifications (raises ValidationError) - # 2. validate_assignment would be redundant - any assignment attempt fails immediately - # 3. This differs from ModelContractReducer which includes validate_assignment=True - # for explicit documentation purposes, but the behavior is identical + # Note: validate_assignment is intentionally omitted because frozen=True + # already prevents all attribute modifications (raises ValidationError). + # Any assignment attempt fails immediately, making validate_assignment redundant.src/omnibase_core/models/contracts/model_contract_reducer.py (1)
505-520: Consider removing validate_assignment=True as redundant.While the comment acknowledges that
validate_assignment=Trueis redundant withfrozen=True, keeping it may confuse future maintainers. Since frozen models cannot be modified, validation on assignment is unnecessary.If you prefer to keep it for documentation purposes, that's acceptable. Otherwise, apply this diff:
model_config = ConfigDict( extra="forbid", # Strict validation - reject unknown fields frozen=True, # Thread safety and immutability - instances cannot be modified use_enum_values=False, # Keep enum objects, don't convert to strings - validate_assignment=True, # Explicit for documentation - redundant with frozen=True but kept for clarity str_strip_whitespace=True, # Clean string inputs validate_default=True, # Validate default values at model definition time populate_by_name=True, # Allow both field name and alias for YAML flexibility )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/omnibase_core/models/contracts/model_contract_reducer.py(8 hunks)src/omnibase_core/models/contracts/subcontracts/model_fsm_subcontract.py(6 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Always use Poetry for all Python package management and task execution - never use pip directly or execute python/pytest withoutpoetry runprefix
Format all Python code with black and sort imports with isort - maintain consistent code formatting across the project
Files:
src/omnibase_core/models/contracts/model_contract_reducer.pysrc/omnibase_core/models/contracts/subcontracts/model_fsm_subcontract.py
src/omnibase_core/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/omnibase_core/**/*.py: Use structured error handling withModelOnexErrorandEnumCoreErrorCode- never raise genericException
Use the@standard_error_handlingdecorator to eliminate 6+ lines of try/catch boilerplate in async operations
Use protocol-based dependency injection withcontainer.get_service("ProtocolName")- never use concrete class names or isinstance checks with protocols
Type check all Python code with strict mypy (100% strict mode compliance required) - all functions must have type annotations and disallow Any return types
Use ModelEventEnvelope for inter-service event-driven communication - wrap event data in standardized envelope pattern
All source files must achieve 100% strict mypy compliance - no Any types allowed, all function parameters and return types must be explicitly annotated
Document all public APIs, classes, and functions with docstrings explaining purpose, parameters, return values, and usage examples
Never use generic Exception - always use ModelOnexError with specific EnumCoreErrorCode for error handling consistency
Use EnumNodeKind for high-level architectural routing (EFFECT, COMPUTE, REDUCER, ORCHESTRATOR, RUNTIME_HOST) - use EnumNodeType for specific implementation discovery
Files:
src/omnibase_core/models/contracts/model_contract_reducer.pysrc/omnibase_core/models/contracts/subcontracts/model_fsm_subcontract.py
src/omnibase_core/**/*reducer*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/omnibase_core/**/*reducer*.py: NodeReducer and NodeOrchestrator are the primary implementations for v0.4.0+ - import fromomnibase_core.nodes(not deprecated 'Declarative' versions)
Implement FSM transitions using ModelIntent pattern in REDUCER nodes - keep state transition logic separate from side effects
Files:
src/omnibase_core/models/contracts/model_contract_reducer.py
src/omnibase_core/models/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use Pydantic 2.11+ models for all data structures with strict validation enabled
Files:
src/omnibase_core/models/contracts/model_contract_reducer.pysrc/omnibase_core/models/contracts/subcontracts/model_fsm_subcontract.py
🧠 Learnings (3)
📓 Common learnings
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-10T19:23:31.049Z
Learning: Applies to src/omnibase_core/**/*reducer*.py : Implement FSM transitions using ModelIntent pattern in REDUCER nodes - keep state transition logic separate from side effects
📚 Learning: 2025-12-10T19:23:31.049Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-10T19:23:31.049Z
Learning: Applies to src/omnibase_core/**/*reducer*.py : Implement FSM transitions using ModelIntent pattern in REDUCER nodes - keep state transition logic separate from side effects
Applied to files:
src/omnibase_core/models/contracts/model_contract_reducer.pysrc/omnibase_core/models/contracts/subcontracts/model_fsm_subcontract.py
📚 Learning: 2025-12-10T19:23:31.049Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-10T19:23:31.049Z
Learning: Applies to src/omnibase_core/**/*reducer*.py : NodeReducer and NodeOrchestrator are the primary implementations for v0.4.0+ - import from `omnibase_core.nodes` (not deprecated 'Declarative' versions)
Applied to files:
src/omnibase_core/models/contracts/model_contract_reducer.py
🧬 Code graph analysis (1)
src/omnibase_core/models/contracts/subcontracts/model_fsm_subcontract.py (1)
src/omnibase_core/models/primitives/model_semver.py (1)
ModelSemVer(24-143)
⏰ 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 (9)
src/omnibase_core/models/contracts/subcontracts/model_fsm_subcontract.py (3)
4-4: LGTM! Version bump to 1.5.0 is consistent.The schema version and INTERFACE_VERSION are correctly updated to 1.5.0, aligning with the ModelContractReducer upgrade.
Also applies to: 64-64
47-59: LGTM! Immutability documentation is clear and accurate.The documentation correctly explains that frozen=True provides thread safety by making instances immutable after creation, and that validators run during construction before freezing.
331-335: LGTM! Model configuration is correct for v1.5.0.The frozen=True setting correctly implements immutability for thread safety, and the other configuration options are appropriate for FSM subcontracts.
src/omnibase_core/models/contracts/model_contract_reducer.py (6)
110-124: LGTM! Fingerprint field implementation is well-designed.The fingerprint field correctly implements the
<semver>:<sha256_12>format with proper regex validation and comprehensive documentation explaining its purpose for drift detection.
227-240: LGTM! Field aliasing is correctly implemented.The state_machine field with
alias="state_transitions"correctly supports backward compatibility with existing YAML contracts. Combined withpopulate_by_name=Truein model_config, this allows both field names to be used when loading from YAML.
365-367: LGTM! FSM validation correctly uses state_machine field.The validation methods correctly reference
self.state_machine(the primary field name) rather than the alias. This is the proper approach since Pydantic internally uses the primary field name.Also applies to: 435-437
76-91: LGTM! Comprehensive documentation for v1.5.0 features.The documentation clearly explains the three key features added in v1.5.0: fingerprint for drift detection, state_machine with alias support, and frozen=True for immutability. The explanations are concise and informative.
512-520: LGTM! Model configuration correctly implements v1.5.0 requirements.The ConfigDict properly combines frozen=True for immutability, populate_by_name=True for alias support, and other appropriate settings. The inline comments clearly explain each configuration option.
393-404: Clarify the validation logic for dual data sources.The validation checks both
state_machineandstate_transitionsto handle two scenarios: whenoriginal_contract_datais provided from YAML (where either name could appear due topopulate_by_name=True), and when it's None andmodel_dump()is called (which returns onlystate_machineby default). The comment should clarify this distinction—specifically thatmodel_dump()withoutby_alias=Truereturns the field name only, making thestate_transitionscheck necessary only when validating YAML-loaded data. Consider updating the comment to: "Check both field name and alias to support YAML contracts that may use either name due to populate_by_name=True; when using model_dump(), only state_machine will be present."
- Add validate_assignment=True to ModelFSMSubcontract for consistency - Add defense-in-depth docstring to _validate_fsm_subcontract method - Add test for simultaneous state_machine/state_transitions field usage - Add docstring note about frozen model hashing limitations
Code Review for PR #148: ModelContractReducer v1.5.0 Schema UpgradeOverall AssessmentThis PR implements important foundational work for the NodeReducer v1.0 specification. The code quality is excellent with comprehensive tests (50 new tests), thorough documentation, and proper type safety. However, there is a critical backwards compatibility issue that must be addressed before merging. 🚨 Critical Issue: Breaking Change in Field AccessProblemThe PR renames Affected Code Locations:
Example from node_reducer.py:142: if hasattr(self, "contract") and hasattr(self.contract, "state_transitions"):
self.fsm_contract = self.contract.state_transitions # ❌ Will fail!Why This BreaksIn Pydantic v2 with
Solution OptionsOption 1: Add @Property for Backwards Compatibility (Recommended) class ModelContractReducer(MixinNodeTypeValidator, ModelContractBase):
state_machine: ModelFSMSubcontract | None = Field(
default=None,
alias="state_transitions",
# ...
)
@property
def state_transitions(self) -> ModelFSMSubcontract | None:
"""Backwards-compatible alias for state_machine field."""
return self.state_machineOption 2: Update All Call Sites Recommendation: Use Option 1 to maintain backwards compatibility during the transition period, then deprecate the property in a future version. ✅ Strengths1. Excellent Test Coverage
2. Documentation Quality
3. Type Safety
4. Thread Safety
5. Validation Logic
💡 Minor Suggestions1. Fingerprint Validation Could Be More ExplicitLocation: model_contract_reducer.py:133 Consider adding a field_validator for more detailed error messages: @field_validator('fingerprint')
@classmethod
def validate_fingerprint_format(cls, v: str | None) -> str | None:
if v is None:
return v
parts = v.split(':')
if len(parts) != 2:
raise ValueError("Fingerprint must be in format <semver>:<sha256_12>")
semver, hash_part = parts
if not re.match(r'^\d+\.\d+\.\d+$', semver):
raise ValueError(f"Invalid semver format: {semver}")
if not re.match(r'^[a-f0-9]{12}$', hash_part):
raise ValueError(f"Invalid hash format (must be 12 lowercase hex chars): {hash_part}")
return vThis would provide clearer error messages than Pydantic's generic pattern validation. 2. Consider Adding Fingerprint Generation UtilitySince the fingerprint format is well-defined, consider adding a utility function: def generate_contract_fingerprint(contract: ModelContractReducer) -> str:
"""Generate fingerprint for contract drift detection."""
import hashlib
canonical = contract.model_dump_json(sort_keys=True, exclude={'fingerprint', 'correlation_id'})
hash_full = hashlib.sha256(canonical.encode()).hexdigest()
return f"{contract.INTERFACE_VERSION}:{hash_full[:12]}"3. Test Could Verify Frozen Behavior More ThoroughlyLocation: test_model_contract_reducer_v150.py The frozen tests verify basic immutability, but could also test:
Example: def test_frozen_model_raises_validation_error_with_clear_message(self) -> None:
contract = create_minimal_reducer_contract()
with pytest.raises(ValidationError) as exc_info:
contract.name = "modified"
assert "frozen" in str(exc_info.value).lower()4. ModelFSMSubcontract: Comment RedundancyLocation: model_fsm_subcontract.py:330 The comment 🔒 Security Considerations✅ No security concerns identified:
📊 Performance Considerations✅ Minimal performance impact:
Potential Concern: The ✅ Test Coverage AssessmentBased on the test file structure:
Missing Test: Interaction between def test_yaml_loading_with_both_field_names(self) -> None:
"""Test that YAML can use either state_machine or state_transitions."""
fsm = self._create_minimal_fsm_subcontract()
# Test with state_machine field name
yaml1 = yaml.dump({'state_machine': fsm.model_dump(), **base_fields})
contract1 = ModelContractReducer.from_yaml(yaml1)
assert contract1.state_machine is not None
# Test with state_transitions alias
yaml2 = yaml.dump({'state_transitions': fsm.model_dump(), **base_fields})
contract2 = ModelContractReducer.from_yaml(yaml2)
assert contract2.state_machine is not None # Should work via alias📋 Checklist Before Merge
🎯 RecommendationStatus: This is high-quality work that significantly advances the NodeReducer specification. The implementation is clean, well-tested, and properly documented. However, the backwards compatibility issue must be resolved before merging to avoid breaking existing nodes. Once the Estimated Fix Time: 10 minutes Great work on the comprehensive test suite and documentation! The fingerprint mechanism is well-designed and will be valuable for contract drift detection. 🚀 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/omnibase_core/models/contracts/subcontracts/model_fsm_subcontract.py (1)
193-221: Inconsistent error message contains "list[Any]" artifact.The error messages in validator methods contain
list[Any]which appears to be a copy-paste artifact or typo. These should likely just saylistorstates list.Examples at lines 208, 243, 260, 297, 311:
"Initial state '{self.initial_state}' not found in states list[Any]""Terminal state '{state_name}' not found in states list[Any]"This is a pre-existing issue, not introduced by this PR, but worth noting for consistency.
Also applies to: 223-273, 275-324
src/omnibase_core/models/contracts/model_contract_reducer.py (1)
614-626: Coding guidelines: Generic Exception in from_yaml.The
from_yamlmethod at line 614 catches a genericException. Per coding guidelines, structured error handling withModelOnexErrorandEnumCoreErrorCodeshould be used, and genericExceptionshould be avoided.However, this is a catch-all for unexpected errors during YAML parsing/validation, and it does wrap the exception in
ModelOnexError, so the re-raised error follows the guidelines. This is acceptable as a defensive catch-all after handling specific exceptions.Consider being more specific about what exceptions might be caught here, or adding a comment explaining this is an intentional defensive catch-all:
except yaml.YAMLError as e: raise ModelOnexError( ... ) from e - except Exception as e: + # Defensive catch-all for unexpected errors during contract loading + except Exception as e: # noqa: BLE001 - intentional broad exception handling raise ModelOnexError(
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/omnibase_core/models/contracts/model_contract_reducer.py(8 hunks)src/omnibase_core/models/contracts/subcontracts/model_fsm_subcontract.py(6 hunks)tests/unit/models/contracts/test_model_contract_reducer_v150.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Always use Poetry for all Python package management and task execution - never use pip directly or execute python/pytest withoutpoetry runprefix
Format all Python code with black and sort imports with isort - maintain consistent code formatting across the project
Files:
src/omnibase_core/models/contracts/model_contract_reducer.pysrc/omnibase_core/models/contracts/subcontracts/model_fsm_subcontract.pytests/unit/models/contracts/test_model_contract_reducer_v150.py
src/omnibase_core/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/omnibase_core/**/*.py: Use structured error handling withModelOnexErrorandEnumCoreErrorCode- never raise genericException
Use the@standard_error_handlingdecorator to eliminate 6+ lines of try/catch boilerplate in async operations
Use protocol-based dependency injection withcontainer.get_service("ProtocolName")- never use concrete class names or isinstance checks with protocols
Type check all Python code with strict mypy (100% strict mode compliance required) - all functions must have type annotations and disallow Any return types
Use ModelEventEnvelope for inter-service event-driven communication - wrap event data in standardized envelope pattern
All source files must achieve 100% strict mypy compliance - no Any types allowed, all function parameters and return types must be explicitly annotated
Document all public APIs, classes, and functions with docstrings explaining purpose, parameters, return values, and usage examples
Never use generic Exception - always use ModelOnexError with specific EnumCoreErrorCode for error handling consistency
Use EnumNodeKind for high-level architectural routing (EFFECT, COMPUTE, REDUCER, ORCHESTRATOR, RUNTIME_HOST) - use EnumNodeType for specific implementation discovery
Files:
src/omnibase_core/models/contracts/model_contract_reducer.pysrc/omnibase_core/models/contracts/subcontracts/model_fsm_subcontract.py
src/omnibase_core/**/*reducer*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/omnibase_core/**/*reducer*.py: NodeReducer and NodeOrchestrator are the primary implementations for v0.4.0+ - import fromomnibase_core.nodes(not deprecated 'Declarative' versions)
Implement FSM transitions using ModelIntent pattern in REDUCER nodes - keep state transition logic separate from side effects
Files:
src/omnibase_core/models/contracts/model_contract_reducer.py
src/omnibase_core/models/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use Pydantic 2.11+ models for all data structures with strict validation enabled
Files:
src/omnibase_core/models/contracts/model_contract_reducer.pysrc/omnibase_core/models/contracts/subcontracts/model_fsm_subcontract.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Write comprehensive test coverage with minimum 60% coverage requirement - organize tests in unit/ and integration/ directories with appropriate markers
Use appropriate pytest markers (@pytest.mark.unit, @pytest.mark.integration, @pytest.mark.slow, @pytest.mark.performance) to categorize tests
Files:
tests/unit/models/contracts/test_model_contract_reducer_v150.py
🧠 Learnings (3)
📓 Common learnings
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-10T19:23:31.049Z
Learning: Applies to src/omnibase_core/**/*reducer*.py : Implement FSM transitions using ModelIntent pattern in REDUCER nodes - keep state transition logic separate from side effects
📚 Learning: 2025-12-10T19:23:31.049Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-10T19:23:31.049Z
Learning: Applies to src/omnibase_core/**/*reducer*.py : Implement FSM transitions using ModelIntent pattern in REDUCER nodes - keep state transition logic separate from side effects
Applied to files:
src/omnibase_core/models/contracts/model_contract_reducer.pysrc/omnibase_core/models/contracts/subcontracts/model_fsm_subcontract.py
📚 Learning: 2025-12-10T19:23:31.049Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-10T19:23:31.049Z
Learning: Applies to src/omnibase_core/**/*reducer*.py : NodeReducer and NodeOrchestrator are the primary implementations for v0.4.0+ - import from `omnibase_core.nodes` (not deprecated 'Declarative' versions)
Applied to files:
src/omnibase_core/models/contracts/model_contract_reducer.py
🧬 Code graph analysis (2)
src/omnibase_core/models/contracts/model_contract_reducer.py (3)
src/omnibase_core/models/primitives/model_semver.py (1)
ModelSemVer(24-143)src/omnibase_core/models/contracts/subcontracts/model_fsm_subcontract.py (1)
ModelFSMSubcontract(39-331)tests/unit/contracts/test_runtime_contracts.py (1)
contract_data(183-185)
src/omnibase_core/models/contracts/subcontracts/model_fsm_subcontract.py (1)
src/omnibase_core/models/primitives/model_semver.py (1)
ModelSemVer(24-143)
⏰ 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 (18)
src/omnibase_core/models/contracts/subcontracts/model_fsm_subcontract.py (3)
1-21: LGTM! Well-documented schema upgrade.The module docstring clearly documents the v1.5.0 schema version, immutability guarantees via
frozen=True, and thread-safety implications. Good addition of the pre-freeze construction semantics explanation.
64-64: LGTM! Interface version correctly updated.The
INTERFACE_VERSIONis correctly set toModelSemVer(major=1, minor=5, patch=0), aligning with the v1.5.0 schema upgrade.
326-331: LGTM! Configuration correctly enables immutability.The
frozen=Truesetting is correctly added for thread safety. The comment explaining thatvalidate_assignment=Trueis redundant but kept for documentation clarity is helpful.src/omnibase_core/models/contracts/model_contract_reducer.py (7)
1-15: LGTM! Clear v1.5.0 schema documentation.The module docstring effectively summarizes the key v1.5.0 features: fingerprint for drift detection, field aliasing for YAML flexibility, and thread-safe immutability.
67-103: LGTM! Comprehensive class-level documentation.Excellent documentation of the v1.5.0 schema features, including the important note about hashability limitations with nested mutable objects. This proactive documentation will help developers understand why
hash()fails despitefrozen=True.
120-134: LGTM! Well-designed fingerprint field.The fingerprint field is well-implemented:
- Optional with
Nonedefault for backwards compatibility- Clear regex pattern
r"^\d+\.\d+\.\d+:[a-f0-9]{12}$"matching the documented format- Descriptive field documentation with format example
The pattern correctly enforces lowercase hex characters and exactly 12 characters for the SHA256 prefix.
237-250: LGTM! State machine alias provides backwards compatibility.The
state_machinefield withalias="state_transitions"combined withpopulate_by_name=Truein the config correctly enables YAML contracts to use either field name. The documentation clearly explains the aliasing mechanism.
403-423: LGTM! Validation correctly checks both field name and alias.The validation logic at lines 405-408 correctly checks for both
"state_machine"and"state_transitions"keys when validating against the original contract data, ensuring the validation works regardless of which key the YAML contract uses.
439-464: LGTM! Defense-in-depth pattern well documented.The detailed docstring explaining why the contract-level validation intentionally duplicates FSM-level validation is excellent. This documents the architectural decision and helps future maintainers understand the deliberate redundancy.
Based on learnings, this aligns with the pattern of keeping state transition logic separate from side effects in REDUCER nodes.
531-546: LGTM! Model configuration correctly implements v1.5.0 requirements.The configuration correctly adds:
frozen=Truefor immutability and thread safetypopulate_by_name=Truefor alias flexibility- Clear comments explaining each setting's purpose
The note about
validate_assignment=Truebeing redundant withfrozen=Truebut kept for documentation clarity is helpful.tests/unit/models/contracts/test_model_contract_reducer_v150.py (8)
1-21: LGTM! Comprehensive test documentation.The module docstring clearly outlines what features are being tested and the requirements from v1.5.0. This makes it easy to understand the test coverage at a glance.
23-23: Use ofAnytype in test file.Line 23 imports
Anyfrom typing. Per coding guidelines forsrc/omnibase_core/**/*.py, "No Any types allowed" with strict mypy compliance required. However, this file is intests/directory, notsrc/omnibase_core/, so the strict typing requirement may not apply here.The
Anyusage in the helper functions (lines 42, 49, 61) is reasonable for test flexibility, allowing tests to pass arbitrary overrides without type constraints.
42-64: LGTM! Well-designed test helpers.The helper functions
get_minimal_reducer_contract_dataandcreate_minimal_reducer_contractprovide a clean way to create test fixtures with optional overrides, reducing boilerplate across tests.
66-167: LGTM! Thorough fingerprint validation tests.Excellent coverage of fingerprint validation edge cases including:
- Valid formats (basic, zero version, large version, all hex chars)
- Invalid formats (missing colon, wrong semver, hex length issues, uppercase, non-hex chars, negative version, empty string, special chars, spaces)
- Optional/default behavior (None allowed, defaults to None)
This provides good confidence in the regex pattern validation.
169-418: LGTM! Comprehensive alias compatibility tests.The state machine alias tests thoroughly verify:
- Direct field access via
state_machine- Alias-based population via
state_transitions(dict/YAML compatibility)- Both field name and alias working with
populate_by_name=True- Rejection when both keys are provided simultaneously (extra='forbid' behavior)
The detailed docstring in
test_both_state_machine_and_state_transitions_providedis particularly helpful for documenting the expected Pydantic behavior.
420-516: LGTM! Frozen model behavior tests.Good coverage of immutability verification:
- Config check for
frozen=True- Mutation rejection for multiple fields
- Correlation ID and node_name immutability
- Important documentation of hashability limitations with nested mutable objects
The test at lines 488-500 correctly documents and tests the expected Pydantic limitation where frozen models with nested mutable objects cannot be hashed.
518-572: LGTM! Version tests validate v1.5.0 contract.Tests correctly verify:
- INTERFACE_VERSION is 1.5.0 for both ModelContractReducer and ModelFSMSubcontract
- Version is proper ModelSemVer instance
- Version is a class variable (not instance variable)
- FSM subcontract also has frozen=True
574-640: LGTM! Configuration and serialization tests.Good coverage of:
extra='forbid'rejecting unknown fieldspopulate_by_name=Trueconfigurationuse_enum_values=Falsepreserving enum objects- Serialization including/preserving fingerprint in model_dump and to_yaml
These tests ensure the v1.5.0 configuration behaves as documented.
…es to state_machine [OMN-648] Breaking Change: All code now uses `state_machine` field directly. The `state_transitions` alias ONLY works for YAML/dict parsing (Pydantic alias), NOT for attribute access. No backwards-compatible property provided - no tech debt until v0.4.0. Changes: - Remove @Property state_transitions from ModelContractReducer - Update node_reducer.py to use state_machine field - Update mixin_fsm_execution.py docstring examples - Update mixin_metadata.yaml code examples
Code Review: ModelContractReducer v1.5.0 Schema Upgrade [OMN-648]SummaryExcellent work on this schema upgrade! The implementation is production-ready with comprehensive test coverage (50 tests), clean architectural patterns, and thorough documentation. The changes align perfectly with omnibase_core conventions and NodeReducer v1.0 spec requirements. ✅ Strengths1. Exceptional Test Coverage ⭐
2. Excellent Documentation 📚
3. Backwards Compatibility 🔄
4. Clean Architecture 🏗️
5. Consistent Updates Across Codebase
💡 Minor Suggestions (Optional Improvements)1. Fingerprint Validation Could Be More ExplicitLocation: The regex pattern @field_validator('fingerprint')
@classmethod
def validate_fingerprint_format(cls, v: str | None) -> str | None:
"""Validate fingerprint format with descriptive error messages."""
if v is None:
return v
if ':' not in v:
raise ValueError("Fingerprint must contain ':' separator (format: <semver>:<sha256_12>)")
semver_part, hash_part = v.split(':', 1)
if not re.match(r'^\d+\.\d+\.\d+$', semver_part):
raise ValueError(f"Invalid semver format: {semver_part} (expected X.Y.Z)")
if not re.match(r'^[a-f0-9]{12}$', hash_part):
raise ValueError(f"Invalid hash format: {hash_part} (expected 12 lowercase hex chars)")
return vWhy: Current pattern validation works but gives generic "pattern does not match" errors. Custom validator provides actionable feedback for developers. Priority: Low (current implementation is functional) 2. Consider Type Narrowing in FSM ValidationLocation: The check def _validate_fsm_subcontract(self) -> None:
fsm = self.state_machine
assert fsm is not None, "_validate_fsm_subcontract called with None FSM"
# ... validation logicWhy: Makes the invariant explicit and helps mypy with type narrowing. Priority: Low (current code is safe and mypy-compliant) 3. Test Missing: Serialization Round-Trip with FSMLocation: Tests cover serialization basics (lines 471-565) but don't verify round-trip with a full FSM subcontract. Consider adding: def test_serialization_roundtrip_with_full_fsm(self) -> None:
"""Test that contract with FSM survives model_dump + reconstruction."""
fsm = create_minimal_fsm_subcontract()
original = create_minimal_reducer_contract(state_machine=fsm)
# Serialize and reconstruct
data = original.model_dump()
reconstructed = ModelContractReducer(**data)
# Verify FSM preserved
assert reconstructed.state_machine is not None
assert reconstructed.state_machine.state_machine_name == fsm.state_machine_name
assert len(reconstructed.state_machine.states) == len(fsm.states)Why: Ensures complex nested structures survive serialization (important for contract generation). Priority: Low (current tests are comprehensive, this adds defense-in-depth) 4. Documentation: Clarify Fingerprint GenerationLocation: The docstring explains the format but not how to generate it. Consider adding a note: fingerprint: str | None = Field(
default=None,
description=(
"Contract fingerprint for drift detection. Format: <semver>:<sha256_12> "
"where semver is the schema version (e.g., '1.5.0') and sha256_12 is the "
"first 12 hex characters of the SHA256 hash of the canonical contract content. "
"Example: '1.5.0:a1b2c3d4e5f6'. "
"Generation: Use 'poetry run onex-cli contract fingerprint <contract.yaml>' "
"or see docs/guides/CONTRACT_FINGERPRINTING.md for implementation details."
),
pattern=r"^\d+\.\d+\.\d+:[a-f0-9]{12}$",
)Why: Developers will need guidance on fingerprint generation workflow. Priority: Low (can be addressed in OMN-260 when contracts are upgraded) 🔒 Security Considerations✅ No security concerns identified
🚀 Performance Considerations✅ No performance concerns
📊 Test ExecutionBased on the PR description:
Coverage: Comprehensive across all v1.5.0 features 📋 Checklist ReviewPer CLAUDE.md requirements:
🎯 RecommendationAPPROVE ✅ This PR is production-ready and can be merged. The suggested improvements above are optional enhancements that can be addressed in future PRs if desired. Why Approve:
Post-Merge Actions:
Reviewed by: Claude Sonnet 4.5 (omnibase_core CLAUDE.md compliant review) |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/omnibase_core/nodes/node_reducer.py (1)
93-103: FSM initialization correctly targetsstate_machine; optional refinement for contract loading.
- The example and
__init__now usecontract.state_machine, which aligns with the v1.5.0 contract (with aliasing fromstate_transitions) and matches the mixin/docs.- The extra
if self.fsm_contract is not None:guard beforeinitialize_fsm_stateis a good safety improvement with the new optional/aliased field.- One thing to consider:
hasattr(self, "contract")(and thenhasattr(self.contract, "state_machine")) will still swallow exceptions raised while loading the contract, treating a failing load as “no FSM present”. Long term, it may be cleaner to accessself.contractdirectly and surface contract load errors explicitly, rather than hiding them behind the debug log. This is pre‑existing behavior but worth tightening when you next touch this path. Based on learnings, this still respects thesuper().__init__(container)constructor requirement.Also applies to: 135-154
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/omnibase_core/mixins/mixin_fsm_execution.py(5 hunks)src/omnibase_core/mixins/mixin_metadata.yaml(5 hunks)src/omnibase_core/nodes/node_reducer.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{py,pyc,pyi}
📄 CodeRabbit inference engine (CLAUDE.md)
Always use Poetry for Python package management and task execution - never use pip or python directly
Files:
src/omnibase_core/nodes/node_reducer.pysrc/omnibase_core/mixins/mixin_fsm_execution.py
**/{node,nodes}/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/{node,nodes}/*.py: Use ModelONEXContainer (from omnibase_core.models.container.model_onex_container) for dependency injection in node constructors, never ModelContainer[T]
Always call super().init(container) when subclassing NodeCompute, NodeEffect, NodeReducer, or NodeOrchestrator
Files:
src/omnibase_core/nodes/node_reducer.py
src/omnibase_core/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/omnibase_core/**/*.py: Use ModelOnexError with EnumCoreErrorCode for all error handling, never generic Exception
Use @standard_error_handling decorator from omnibase_core.decorators.error_handling to eliminate try/catch boilerplate
Use protocol names (e.g., 'ProtocolEventBus') with container.get_service(), never concrete class names
Use EnumNodeKind for high-level architectural classification (EFFECT, COMPUTE, REDUCER, ORCHESTRATOR) and EnumNodeType for specific implementation types
Maintain 100% strict mypy compliance - all functions must have type annotations and zero mypy errors
Do NOT share node instances across threads without explicit synchronization - most ONEX components are not thread-safe by default
Implement mixin system using available mixins (MixinDiscoveryResponder, MixinEventHandler, MixinNodeExecutor, etc.) for cross-cutting concerns
Use ModelEventEnvelope for inter-service communication and event-driven processing patterns
Python version requirement: 3.12+
Structure project following the omnibase_core directory layout with clear separation of concerns (constants, container, decorators, discovery, enums, errors, events, infrastructure, logging, mixins, models, nodes, primitives, types, utils, validation)
Files:
src/omnibase_core/nodes/node_reducer.pysrc/omnibase_core/mixins/mixin_fsm_execution.py
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Import node classes from omnibase_core.nodes (NodeCompute, NodeEffect, NodeReducer, NodeOrchestrator), not from infrastructure submodules
Files:
src/omnibase_core/nodes/node_reducer.pysrc/omnibase_core/mixins/mixin_fsm_execution.py
**/*.{py,pyi}
📄 CodeRabbit inference engine (CLAUDE.md)
Format code with black and sort imports with isort before committing
Files:
src/omnibase_core/nodes/node_reducer.pysrc/omnibase_core/mixins/mixin_fsm_execution.py
**/*.{yaml,yml}
📄 CodeRabbit inference engine (CLAUDE.md)
Use declarative YAML contracts for node configuration in v0.4.0+ architecture
Files:
src/omnibase_core/mixins/mixin_metadata.yaml
🧠 Learnings (1)
📚 Learning: 2025-12-10T21:31:33.541Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-10T21:31:33.541Z
Learning: Applies to **/{node,nodes}/*.py : Always call super().__init__(container) when subclassing NodeCompute, NodeEffect, NodeReducer, or NodeOrchestrator
Applied to files:
src/omnibase_core/nodes/node_reducer.py
🧬 Code graph analysis (1)
src/omnibase_core/nodes/node_reducer.py (3)
src/omnibase_core/models/contracts/subcontracts/model_fsm_subcontract.py (1)
ModelFSMSubcontract(39-331)src/omnibase_core/mixins/mixin_node_setup.py (1)
contract(61-64)src/omnibase_core/mixins/mixin_fsm_execution.py (1)
initialize_fsm_state(184-205)
⏰ 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 (2)
src/omnibase_core/mixins/mixin_fsm_execution.py (1)
75-87: Doc examples correctly migrated tocontract.state_machine.All updated examples now reference
contract.state_machine, matching the new contract schema and the NodeReducer usage. No runtime behavior is affected, and the examples remain coherent and accurate.Also applies to: 131-139, 177-181, 195-200, 217-221
src/omnibase_core/mixins/mixin_metadata.yaml (1)
2570-2584: FSM mixin metadata is consistent with the newstate_machinefield.Generated Python patterns now use
contract.state_machine, while YAML examples still declarestate_transitions, which is appropriate given the aliasing on the contract model. This keeps code generation and docs aligned with v1.5.0 without breaking existing YAML.Also applies to: 2587-2604, 2607-2619, 2637-2661, 2665-2682, 2687-2703
…elcontractreducer-v150-schema-upgrade
Code Review: ModelContractReducer v1.5.0 Schema UpgradeOverviewThis PR upgrades ModelContractReducer and ModelFSMSubcontract to v1.5.0 schema with fingerprint support, field aliasing, and immutability. The changes are well-structured and thoroughly tested. ✅ Strengths1. Excellent Test Coverage
2. Backwards Compatibility
3. Thread Safety & Immutability
4. Documentation Quality
5. Consistent Updates
🔍 Observations & Suggestions1. Fingerprint Pattern Validation (Minor) 2. Defense-in-Depth Documentation (Positive) 3. FSM Contract Loading Logic (Minor) 🔒 Security Considerations✅ No security concerns identified
🎯 Performance Considerations✅ No performance regressions expected
🧪 Test Coverage AssessmentExcellent coverage across all v1.5.0 changes:
Total: 50 tests for v1.5.0 changes + existing reducer tests passing 📋 Code Quality Checklist
🎯 RecommendationsPriority: Low (All optional improvements)
✅ Final AssessmentLGTM with minor suggestions ✨ This is a high-quality PR with:
The suggestions above are minor documentation enhancements and do not block merging. 📚 References
Recommendation: ✅ Approve and merge (after addressing optional suggestions if desired) Reviewed using omnibase_core CLAUDE.md v0.4.0 guidelines |
1 participant
Summary
Upgrades
ModelContractReducerandModelFSMSubcontractto v1.5.0 schema to align with the NodeReducer v1.0 specification. This is a prerequisite for upgrading actual contracts (OMN-260) and validating them (OMN-257).Changes
ModelContractReducer (
src/omnibase_core/models/contracts/model_contract_reducer.py):fingerprintfield with regex validation (<semver>:<sha256_12>)state_transitions→state_machinewith alias support for YAML flexibilityfrozen=Truefor thread safety and immutabilitypopulate_by_name=Truefor YAML alias flexibilityINTERFACE_VERSIONto 1.5.0ModelFSMSubcontract (
src/omnibase_core/models/contracts/subcontracts/model_fsm_subcontract.py):frozen=Truefor immutability after creationINTERFACE_VERSIONto 1.5.0Tests (
tests/unit/models/contracts/test_model_contract_reducer_v150.py):Test Plan
Linear Ticket
Dependencies
Summary by CodeRabbit
New Features
Behavior Changes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.