Repository navigation
fix(models): remove Any type annotations from workflow/fsm/orchestrator (OMN-1177) - #298
Conversation
…or (OMN-1177) Replace all `Any` type annotations with `object` to comply with ONEX type safety standard: "NO `Any` types - use `object` for generic payloads" Changes: - models/workflow/: Replace Any in helper functions and Callable/list types - models/fsm/: Replace Any in Pydantic validator parameters - models/orchestrator/: Replace Any in validators and kwargs All 664 tests pass, mypy reports 0 errors on 41 source files.
|
Warning Rate limit exceeded@jonahgabriel has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 0 minutes and 30 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughReplaced liberal uses of Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
Comment |
Code Review - PR #298SummaryThis PR removes all ✅ Strengths
🔍 Observations
🎯 Type Safety AnalysisBefore (with
After (with
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/omnibase_core/models/fsm/model_fsm_transition_result.py (1)
13-16: Fix import sorting to resolve pipeline failure.The pipeline reports that the import block is unsorted. Run
ruff --fixorpoetry run ruff --fixto automatically resolve this formatting issue.Based on coding guidelines, use
poetry runprefix for Python commands:#!/bin/bash # Fix import sorting poetry run ruff check --fix src/omnibase_core/models/fsm/model_fsm_transition_result.py
🧹 Nitpick comments (1)
src/omnibase_core/models/fsm/model_fsm_data_primary.py (1)
108-116: Consider: Sorting metadata keys for deterministic ordering.The
_convert_metadata_to_frozenvalidator inmodel_fsm_transition_result.pysorts keys for deterministic ordering (as shown in the relevant code snippets). This file's implementation doesn't sort, which could lead to non-deterministic tuple ordering when created from dicts.This is a minor consistency consideration and may be intentional if ordering preservation is desired here.
🔎 Optional: Add sorting for consistency
@field_validator("metadata", mode="before") @classmethod def _convert_metadata_to_frozen( cls, v: dict[str, str] | tuple[tuple[str, str], ...] | object ) -> tuple[tuple[str, str], ...]: - """Convert dict to tuple of tuples for deep immutability.""" + """Convert dict to sorted tuple of tuples for deep immutability.""" if isinstance(v, dict): - return tuple(v.items()) + return tuple(sorted(v.items(), key=lambda x: x[0])) return v # type: ignore[return-value]
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/omnibase_core/models/fsm/model_fsm_data_primary.pysrc/omnibase_core/models/fsm/model_fsm_state.pysrc/omnibase_core/models/fsm/model_fsm_transition.pysrc/omnibase_core/models/fsm/model_fsm_transition_result.pysrc/omnibase_core/models/orchestrator/model_orchestrator_output.pysrc/omnibase_core/models/orchestrator/payloads/model_action_typed_payload.pysrc/omnibase_core/models/workflow/execution/model_workflow_state_snapshot.pysrc/omnibase_core/models/workflow/execution/model_workflow_step_execution.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/workflow/execution/model_workflow_step_execution.pysrc/omnibase_core/models/fsm/model_fsm_data_primary.pysrc/omnibase_core/models/orchestrator/model_orchestrator_output.pysrc/omnibase_core/models/fsm/model_fsm_state.pysrc/omnibase_core/models/fsm/model_fsm_transition_result.pysrc/omnibase_core/models/workflow/execution/model_workflow_state_snapshot.pysrc/omnibase_core/models/fsm/model_fsm_transition.pysrc/omnibase_core/models/orchestrator/payloads/model_action_typed_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/workflow/execution/model_workflow_step_execution.pysrc/omnibase_core/models/fsm/model_fsm_data_primary.pysrc/omnibase_core/models/orchestrator/model_orchestrator_output.pysrc/omnibase_core/models/fsm/model_fsm_state.pysrc/omnibase_core/models/fsm/model_fsm_transition_result.pysrc/omnibase_core/models/workflow/execution/model_workflow_state_snapshot.pysrc/omnibase_core/models/fsm/model_fsm_transition.pysrc/omnibase_core/models/orchestrator/payloads/model_action_typed_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/workflow/execution/model_workflow_step_execution.pysrc/omnibase_core/models/fsm/model_fsm_data_primary.pysrc/omnibase_core/models/orchestrator/model_orchestrator_output.pysrc/omnibase_core/models/fsm/model_fsm_state.pysrc/omnibase_core/models/fsm/model_fsm_transition_result.pysrc/omnibase_core/models/workflow/execution/model_workflow_state_snapshot.pysrc/omnibase_core/models/fsm/model_fsm_transition.pysrc/omnibase_core/models/orchestrator/payloads/model_action_typed_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:
src/omnibase_core/models/workflow/execution/model_workflow_step_execution.pysrc/omnibase_core/models/fsm/model_fsm_data_primary.pysrc/omnibase_core/models/orchestrator/model_orchestrator_output.pysrc/omnibase_core/models/fsm/model_fsm_state.pysrc/omnibase_core/models/fsm/model_fsm_transition_result.pysrc/omnibase_core/models/workflow/execution/model_workflow_state_snapshot.pysrc/omnibase_core/models/fsm/model_fsm_transition.pysrc/omnibase_core/models/orchestrator/payloads/model_action_typed_payload.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/**/*.py : Use duck typing with protocols instead of isinstance checks for type validation
Learnt from: jonahgabriel
Repo: OmniNode-ai/omnibase_core PR: 32
File: examples/practical_migration_example.py:125-134
Timestamp: 2025-09-23T22:28:00.333Z
Learning: The omnibase_core codebase extensively uses constrained TypeVars for primitive types with the pattern `T = TypeVar("T", str, int, float, bool)` across multiple models including ModelTypedMetrics, ModelGenericMetadata, and ModelExecutionResult. This is the preferred approach over hardcoded union types like `str | int | bool | float | None`.
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 PEP 604 union syntax (str | None) instead of Optional/Union - enforce via ruff UP007
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 : Both mypy AND pyright must pass in CI with strict type checking (0 errors required for mypy)
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:
src/omnibase_core/models/workflow/execution/model_workflow_step_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/fsm/model_fsm_data_primary.pysrc/omnibase_core/models/fsm/model_fsm_state.pysrc/omnibase_core/models/fsm/model_fsm_transition_result.pysrc/omnibase_core/models/fsm/model_fsm_transition.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/fsm/model_fsm_state.pysrc/omnibase_core/models/fsm/model_fsm_transition.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/fsm/model_fsm_transition_result.py
🧬 Code graph analysis (3)
src/omnibase_core/models/fsm/model_fsm_data_primary.py (1)
src/omnibase_core/models/fsm/model_fsm_transition_result.py (1)
_convert_metadata_to_frozen(104-123)
src/omnibase_core/models/orchestrator/model_orchestrator_output.py (2)
src/omnibase_core/models/common/model_schema_value.py (1)
ModelSchemaValue(20-348)src/omnibase_core/models/orchestrator/model_action.py (1)
ModelAction(67-351)
src/omnibase_core/models/workflow/execution/model_workflow_state_snapshot.py (1)
src/omnibase_core/protocols/event_bus/protocol_event_message.py (2)
value(33-35)key(28-30)
🪛 GitHub Actions: Test Suite
src/omnibase_core/models/fsm/model_fsm_transition_result.py
[error] 13-13: Import block is un-sorted or un-formatted. Organize imports. Found 1 error. This can be fixed with: ruff --fix.
⏰ 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 (16)
src/omnibase_core/models/workflow/execution/model_workflow_state_snapshot.py (5)
33-33: LGTM: Import cleanup aligns with ONEX standards.Removing
Anyfrom the import statement is correct, as the file now usesobjectfor all generic type annotations.
514-537: LGTM: Type narrowing from Any to object improves type safety.The change from
Anytoobjectin_sanitize_valueproperly restricts the type while maintaining duck typing capabilities. The function correctly handles strings, dicts, lists, tuples, and preserves other types, which all work withobjectas the base type.
643-662: LGTM: Type narrowing maintains PII scanning functionality.The change from
Anytoobjectin_scan_valueis appropriate. The function usesisinstance()checks for string, dict, list, and tuple types, which all work correctly withobjectas the parameter type.
722-730: LGTM: Type narrowing for key counting is correct.The change from
Anytoobjectin_count_keysmaintains the recursive key counting logic while improving type safety.
757-767: LGTM: Type narrowing for depth calculation is correct.The change from
Anytoobjectin_max_depthmaintains the nesting depth calculation logic while improving type safety.src/omnibase_core/models/orchestrator/payloads/model_action_typed_payload.py (1)
198-198: LGTM: Kwargs type narrowing aligns with ONEX standards.Changing
**kwargs: Anyto**kwargs: objectproperly restricts the factory function parameters while maintaining flexibility for payload construction. This aligns with the ONEX standard: "NOAnytypes - useobjectfor generic payloads".src/omnibase_core/models/workflow/execution/model_workflow_step_execution.py (3)
46-46: LGTM: Config comment updated to reflect type changes.The comment correctly documents that
arbitrary_types_allowed=Trueis needed forCallable[..., object]andExceptiontypes.
79-83: LGTM: Condition function return type narrowed appropriately.Changing
Callable[..., object]fromCallable[..., Any]improves type safety for custom condition functions while maintaining flexibility for various return types that can be evaluated as truthy/falsy.
121-124: LGTM: Results type narrowed from Any to object.Changing
list[Any]tolist[object]for theresultsfield improves type safety while still allowing heterogeneous execution results. This aligns with the ONEX standard to avoidAnytypes.src/omnibase_core/models/fsm/model_fsm_transition.py (2)
54-60: LGTM: Type narrowing with appropriate type: ignore annotations.The change from
Anytoobjectin the validator parameter improves type safety. Thetype: ignore[return-value]comments are necessary because:
- When
vis alist[object],tuple(v)returnstuple[object, ...], nottuple[str, ...]- When
vis not a list, it could betuple[object, ...]orobject, not the expectedtuple[str, ...]Pydantic performs additional validation after the validator returns, ensuring the final type is correct. This pattern maintains runtime correctness while satisfying stricter static type checking.
64-70: LGTM: Consistent type narrowing pattern applied.The same type narrowing pattern as
_convert_conditions_to_tupleis correctly applied here for actions. Thetype: ignore[return-value]comments are justified for the same reasons.src/omnibase_core/models/fsm/model_fsm_transition_result.py (1)
94-100: LGTM: Type narrowing with appropriate type: ignore annotation.The change from
Anytoobjectin the validator parameter improves type safety. Thetype: ignore[return-value]comment is necessary because the validator returnstuple[object, ...]while the field expectstuple[ModelIntent, ...]. Pydantic performs validation after the pre-validator runs, ensuring the final type is correct.src/omnibase_core/models/fsm/model_fsm_state.py (1)
80-108: LGTM! Clean replacement ofAnywithobjectin validators.The validator pattern is correct:
mode="before"validators receive raw input, convert lists/dicts to tuples for immutability, and pass through already-valid types. Thetype: ignore[return-value]annotations are appropriate since Pydantic will validate the final type after the validator runs.src/omnibase_core/models/orchestrator/model_orchestrator_output.py (2)
169-177: LGTM! Clean type signature update.The
object | ModelSchemaValue | Noneinput type correctly represents the possible inputs, and the conversion logic properly handles all cases.
179-197: LGTM! Validator logic is sound.The first-element type check is a reasonable optimization to distinguish between
list[ModelAction](already validated) andlist[dict](needs Pydantic validation). Thetype: ignore[return-value]annotations are appropriate since Pydantic will validate individual list items.src/omnibase_core/models/fsm/model_fsm_data_primary.py (1)
68-86: LGTM! Return type correctly reflects validator responsibility.Using
tuple[object, ...]as the return type is appropriate here since these before-mode validators only handle list-to-tuple conversion. Pydantic will validate that items areModelFsmState/ModelFsmTransitionafter the validator runs.
- Add explanatory comment block for type ignore pattern in validators - Create reusable validator utilities in utils/util_validators.py - Sort metadata/variables keys for deterministic ordering in FSM models The type ignore pattern is needed because replacing Any with object means the type checker can't verify tuple passthrough, but Pydantic validates the final type making this safe.
Code Review - PR #298: Remove Any type annotations from workflow/fsm/orchestratorSummaryThis PR successfully removes all ✅ Strengths1. Excellent Type Safety Improvement
2. Outstanding Documentation
3. Code Reusability & DRY Principle
4. Improved Deterministic Behavior
5. Proper Type Annotations
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/omnibase_core/models/fsm/model_fsm_state.py (1)
80-112: Changes align with PR objectives; consider using shared utilities.The type annotation changes from
Anytoobjectare correct and thetype: ignore[return-value]comments are well-justified. The deterministic ordering viasorted()is a good improvement.However, this logic duplicates what's now available in
convert_list_to_tupleandconvert_dict_to_frozen_pairsfromutil_validators.py. Consider using these utilities to reduce duplication and centralize the pattern.🔎 Optional refactor using shared utilities
+from omnibase_core.utils import convert_dict_to_frozen_pairs, convert_list_to_tuple + class ModelFsmState(BaseModel): # ... fields ... @field_validator("entry_actions", mode="before") @classmethod def _convert_entry_actions_to_tuple( cls, v: list[str] | tuple[str, ...] | object ) -> tuple[str, ...]: """Convert list of entry actions to tuple for deep immutability.""" - if isinstance(v, list): - return tuple(v) - return v # type: ignore[return-value] + return convert_list_to_tuple(v) @field_validator("exit_actions", mode="before") @classmethod def _convert_exit_actions_to_tuple( cls, v: list[str] | tuple[str, ...] | object ) -> tuple[str, ...]: """Convert list of exit actions to tuple for deep immutability.""" - if isinstance(v, list): - return tuple(v) - return v # type: ignore[return-value] + return convert_list_to_tuple(v) @field_validator("properties", mode="before") @classmethod def _convert_properties_to_frozen( cls, v: dict[str, str] | tuple[tuple[str, str], ...] | object ) -> tuple[tuple[str, str], ...]: """Convert dict to tuple of tuples for deep immutability. Keys are sorted for deterministic ordering, which ensures consistent hashing and comparison of model instances. """ - if isinstance(v, dict): - return tuple(sorted(v.items())) - return v # type: ignore[return-value] + return convert_dict_to_frozen_pairs(v, sort_keys=True)src/omnibase_core/models/fsm/model_fsm_data_primary.py (1)
79-135: Validators correctly updated; same optional refactor opportunity applies.The type annotation changes and deterministic ordering improvements are correct. Similar to
model_fsm_state.py, these validators duplicate logic now available inutil_validators.py.Consider using the shared utilities for consistency and maintainability. This would also simplify adding new validators in the future.
🔎 Optional refactor using shared utilities
+from omnibase_core.utils import convert_dict_to_frozen_pairs, convert_list_to_tuple + class ModelFsmData(BaseModel): # ... fields and comment block ... @field_validator("states", mode="before") @classmethod def _convert_states_to_tuple( cls, v: list[object] | tuple[object, ...] | object ) -> tuple[object, ...]: """Convert list of states to tuple for deep immutability.""" - if isinstance(v, list): - return tuple(v) - return v # type: ignore[return-value] + return convert_list_to_tuple(v) @field_validator("transitions", mode="before") @classmethod def _convert_transitions_to_tuple( cls, v: list[object] | tuple[object, ...] | object ) -> tuple[object, ...]: """Convert list of transitions to tuple for deep immutability.""" - if isinstance(v, list): - return tuple(v) - return v # type: ignore[return-value] + return convert_list_to_tuple(v) @field_validator("global_actions", mode="before") @classmethod def _convert_global_actions_to_tuple( cls, v: list[str] | tuple[str, ...] | object ) -> tuple[str, ...]: """Convert list of global actions to tuple for deep immutability.""" - if isinstance(v, list): - return tuple(v) - return v # type: ignore[return-value] + return convert_list_to_tuple(v) @field_validator("variables", mode="before") @classmethod def _convert_variables_to_frozen( cls, v: dict[str, str] | tuple[tuple[str, str], ...] | object ) -> tuple[tuple[str, str], ...]: """Convert dict to tuple of tuples for deep immutability. Keys are sorted for deterministic ordering, which ensures consistent hashing and comparison of model instances. """ - if isinstance(v, dict): - return tuple(sorted(v.items())) - return v # type: ignore[return-value] + return convert_dict_to_frozen_pairs(v, sort_keys=True) @field_validator("metadata", mode="before") @classmethod def _convert_metadata_to_frozen( cls, v: dict[str, str] | tuple[tuple[str, str], ...] | object ) -> tuple[tuple[str, str], ...]: """Convert dict to tuple of tuples for deep immutability. Keys are sorted for deterministic ordering, which ensures consistent hashing and comparison of model instances. """ - if isinstance(v, dict): - return tuple(sorted(v.items())) - return v # type: ignore[return-value] + return convert_dict_to_frozen_pairs(v, sort_keys=True)
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/omnibase_core/models/fsm/model_fsm_data_primary.pysrc/omnibase_core/models/fsm/model_fsm_state.pysrc/omnibase_core/utils/__init__.pysrc/omnibase_core/utils/util_validators.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/utils/util_validators.pysrc/omnibase_core/utils/__init__.pysrc/omnibase_core/models/fsm/model_fsm_data_primary.pysrc/omnibase_core/models/fsm/model_fsm_state.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_validators.pysrc/omnibase_core/utils/__init__.pysrc/omnibase_core/models/fsm/model_fsm_data_primary.pysrc/omnibase_core/models/fsm/model_fsm_state.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_validators.pysrc/omnibase_core/utils/__init__.pysrc/omnibase_core/models/fsm/model_fsm_data_primary.pysrc/omnibase_core/models/fsm/model_fsm_state.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/fsm/model_fsm_data_primary.pysrc/omnibase_core/models/fsm/model_fsm_state.py
🧠 Learnings (3)
📓 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/**/*.py : Use PEP 604 union syntax (str | None) instead of Optional/Union - enforce via ruff UP007
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)
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 duck typing with protocols instead of isinstance checks for type validation
📚 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/fsm/model_fsm_data_primary.pysrc/omnibase_core/models/fsm/model_fsm_state.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/fsm/model_fsm_state.py
🧬 Code graph analysis (1)
src/omnibase_core/models/fsm/model_fsm_state.py (1)
src/omnibase_core/models/core/model_enhanced_tool_collection.py (1)
items(401-403)
⏰ 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 (5)
src/omnibase_core/utils/__init__.py (1)
30-30: LGTM!Clean integration of the new utility functions. The imports and exports follow the existing module patterns and are properly positioned in the
__all__list.Also applies to: 47-48
src/omnibase_core/utils/util_validators.py (3)
1-61: Well-documented utility module with comprehensive examples.The module docstring provides excellent guidance on usage patterns and explains the rationale for
type: ignoreannotations. The examples are practical and directly applicable.
71-107: LGTM!The function correctly handles the three documented input cases (list, tuple, other) with appropriate type preservation via
TypeVar.
110-165: LGTM!The function correctly handles dict-to-tuple conversion with optional deterministic ordering. The
sort_keysparameter provides appropriate flexibility for different use cases.Note:
sorted()on dict items assumes comparable keys. This is typically fine since keys are usually homogeneous strings, and Pydantic validation would catch type mismatches.src/omnibase_core/models/fsm/model_fsm_data_primary.py (1)
68-77: Excellent explanatory comment block.This comment provides valuable context for maintainers and explains the rationale behind the
type: ignorepattern clearly. The reference to PR #298 helps with traceability.
Refactor FSM model validators to use the shared utility functions from util_validators.py instead of inline implementations. This addresses the PR #298 review feedback to reduce code duplication. - model_fsm_data_primary.py: 5 validators now use shared utilities - model_fsm_state.py: 3 validators now use shared utilities - model_fsm_transition.py: 2 validators now use shared utilities Reduces ~30 lines of duplicated validator logic.
PR Review: Remove
|
| Criteria | Score | Notes |
|---|---|---|
| Type Safety | ✅ Excellent | Any → object throughout, proper generics |
| Code Reuse | ✅ Excellent | Eliminates ~30 lines of duplication |
| Documentation | ✅ Good | Clear docstrings, type ignore explanation |
| Test Coverage | ❌ CRITICAL GAP | New utilities have 0 tests |
| Consistency | ✅ Good | Follows ONEX patterns, minor nits |
| Performance | ✅ Excellent | Sorting only when needed (sort_keys=True) |
🔒 Security & Performance
- No security concerns: Pure type annotation changes
- Performance: Minimal impact (sorting only on dict→tuple conversion with
sort_keys=True) - Thread safety: Maintained (validators are stateless)
📝 Recommended Changes
Priority 1 (MUST FIX)
- Add comprehensive tests for
util_validators.py
Priority 2 (SHOULD FIX)
- Add explanatory comment block to
model_orchestrator_output.pyfor type ignore pattern - Document
sort_keysusage guidelines inutil_validators.pydocstring
Priority 3 (NICE TO HAVE)
- Add "Public API" note to
util_validators.pymodule docstring - Consider adding validator utilities to docs/INDEX.md
✅ Approval Recommendation
Status: Approve with required test coverage before merge.
The PR demonstrates excellent refactoring discipline and addresses tech debt effectively. However, the missing test coverage for util_validators.py is a blocker per ONEX standards (60%+ coverage requirement).
Suggested Action:
- Add
tests/unit/utils/test_util_validators.pywith 10-15 test cases - Verify coverage:
poetry run pytest tests/unit/utils/test_util_validators.py --cov=src/omnibase_core/utils/util_validators.py - Target: 100% coverage (functions are small enough to fully test)
Once tests are added, this PR is ready to merge. Great work on the type safety improvements! 🎉
Review generated using ONEX standards from CLAUDE.md (v0.4.0)
- Import convert_list_to_tuple from util_validators for consistency - Refactor _convert_intents_to_tuple to use shared utility - Add explanatory comment for custom metadata validator logic Addresses PR #298 review feedback for consistent utility usage.
PR Review - Code Quality AnalysisOverall Assessment✅ APPROVED - This is a well-executed technical debt reduction PR that successfully eliminates Strengths1. Excellent Refactoring with Shared Utilities 🌟Creating
2. Type Safety ImprovementReplacing
3. Deterministic OrderingAdding return convert_dict_to_frozen_pairs(v, sort_keys=True)This is crucial for FSM state equality checks and prevents non-deterministic test failures. 4. Documentation Excellence
5. Code Quality
Areas for Improvement (Minor)1. Missing Test Coverage for New Utilities
|
| Standard | Status | Notes |
|---|---|---|
| Poetry usage | ✅ | Properly documented in commit messages |
| PEP 604 unions | ✅ | Consistent use of | syntax |
| Type annotations | ✅ | Eliminates Any per ONEX standard |
| Error handling | N/A | Not applicable to this PR |
| Documentation | ✅ | Excellent inline and module docs |
| Test coverage | Indirect coverage only for new utilities |
Verification Checklist
- mypy passes (0 errors on 41 files)
- All tests pass (664 tests)
- No remaining
Anyimports in target directories - Follows ONEX architecture patterns
- Proper use of Poetry
- Unit tests for
util_validators.py(recommended addition)
Recommendation
Approve and merge with optional follow-up to add explicit unit tests for util_validators.py. The PR successfully achieves its goal of eliminating Any types while improving code quality through thoughtful refactoring.
Impact: Low risk - type annotation changes only, validated by comprehensive test suite.
Technical Debt Reduction: ✅ Closes OMN-1177 as stated.
Review conducted per CLAUDE.md standards and ONEX architecture guidelines.
- Add 53 unit tests for convert_list_to_tuple and convert_dict_to_frozen_pairs - Test edge cases: empty collections, nested structures, type mismatches - Test Pydantic integration and frozen model immutability - Add explanatory comment to type ignore in model_orchestrator_output.py Addresses PR #298 review feedback for missing test coverage.
Code Review - PR #298: Remove Any Type AnnotationsSummaryThis PR successfully eliminates ✅ Strengths1. Architecture & Design
2. Code Quality
3. Test Coverage
4. Documentation Comments
🔍 Observations & Minor SuggestionsType Ignore Comments (Non-blocking)The def convert_list_to_tuple(v: list[T] | tuple[T, ...] | object) -> tuple[T, ...]:
if isinstance(v, list):
return tuple(v)
return v # type: ignore[return-value] # Safe: Pydantic validates final typeWhy this is correct:
Alternative approach (optional): You could eliminate the type ignore by making the passthrough more explicit: def convert_list_to_tuple(v: list[T] | tuple[T, ...] | object) -> tuple[T, ...] | object:
"""Return type is object to allow passthrough; Pydantic validates final type."""
if isinstance(v, list):
return tuple(v)
return vThis would change the return type to Test File Style (Non-blocking)Line 9 imports from typing import AnyThis is used in test cases for type hints like
No action needed, but worth noting for consistency. 🛡️ Security & PerformanceSecurity
Performance
📊 Test Coverage AnalysisRunning a quick check to see test execution: # Tests for util_validators should all pass
poetry run pytest tests/unit/utils/test_util_validators.py -vThe test file includes:
Total: 53 well-structured unit tests ✅ 🎯 Alignment with ONEX Standards✅ Follows CLAUDE.md Guidelines
✅ Follows ONEX Patterns
📝 Final RecommendationAPPROVE ✅ This PR is ready to merge. It successfully accomplishes the stated goals:
What Makes This PR Excellent
Minor Notes for Future Work (Optional)
Great work on this tech debt cleanup! 🎉 Review completed by Claude Code using CLAUDE.md guidelines and ONEX architecture standards. |
1 participant
Summary
Anytype annotations from workflow-related model directoriesobjectper ONEX standard: "NOAnytypes - useobjectfor generic payloads"Changes
models/workflow/Callable[..., object],list[object]models/fsm/models/orchestrator/**kwargsVerification
Anyimports remain in target directoriesTest plan
poetry run mypy src/omnibase_core/models/workflow/ src/omnibase_core/models/fsm/ src/omnibase_core/models/orchestrator/poetry run pytest tests/unit/models/workflow/ tests/unit/models/fsm/ tests/unit/models/orchestrator/Closes OMN-1177
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.