Repository navigation
feat(types): remove Any from utils, runtime, and container (OMN-1179) - #300
jonahgabriel merged 4 commits into
Conversation
Remove all Any type annotations from utils/, runtime/, and container/ directories per ONEX type safety standards. Changes: - Create new protocols: ProtocolPayloadData, ProtocolSmartLogFormatter, ProtocolContextAwareOutputHandler, ProtocolServiceValidator, ProtocolServiceFactory, ProtocolGenerationConfig, ProtocolImportTracker - Create new models: ModelInjectionContext, ModelServiceDependencyGraph, ModelServiceHealthValidationResult - Implement validate_service_health(), get_dependency_graph(), get_injection_context() features in service_registry.py - Replace 149 Any annotations with strongly typed alternatives - Add TypedDictYamlDumpOptions for type-safe YAML operations - Use PEP 695 type parameters for generic functions - Update tests for new implemented functionality All 1381 tests pass, mypy reports 0 errors. Note: Pre-existing hook failures (scattered models, duplicate filenames, backward compat patterns) are unrelated to this PR.
📝 WalkthroughWalkthroughThe PR introduces new public model and protocol types for the container dependency injection system (ModelInjectionContext, ModelServiceDependencyGraph, ModelServiceHealthValidationResult, ProtocolServiceFactory, ProtocolServiceValidator), expands logging and generation protocols, and systematically replaces generic Any type hints with object throughout the codebase to strengthen type safety and clarity. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/omnibase_core/utils/util_conflict_resolver.py (1)
79-93: Validate custom_resolver when strategy is CUSTOM.If
strategy=EnumConflictResolution.CUSTOMbutcustom_resolver=None, theresolvemethod silently falls through to default behavior (line 136). This is likely a configuration error and should be caught at initialization.🔎 Proposed fix
def __init__( self, strategy: EnumConflictResolution, custom_resolver: Callable[[object, object, str | None], object] | None = None, ): """ Initialize conflict resolver. Args: strategy: Conflict resolution strategy to use custom_resolver: Optional custom resolution function for CUSTOM strategy + + Raises: + ModelOnexError: If CUSTOM strategy is used without a custom_resolver """ + if strategy == EnumConflictResolution.CUSTOM and custom_resolver is None: + raise ModelOnexError( + error_code=EnumCoreErrorCode.VALIDATION_ERROR, + message="custom_resolver is required when using CUSTOM strategy", + context={"strategy": str(strategy)}, + ) self.strategy = strategy self.custom_resolver = custom_resolver self.conflicts_count = 0src/omnibase_core/utils/util_safe_yaml_loader.py (2)
422-422: Typo in error message.Missing space between
dict[str, Any]andin.🔎 Suggested fix
- message=f"Example at index {example_index} is not a dict[str, Any]in schema: {schema_path}", + message=f"Example at index {example_index} is not a dict[str, Any] in schema: {schema_path}",
437-437: Typo in comment.Missing space between
dict[str, Any]andto.🔎 Suggested fix
- # Convert example dict[str, Any]to ModelCustomProperties + # Convert example dict[str, Any] to ModelCustomProperties
♻️ Duplicate comments (1)
src/omnibase_core/protocols/compute/__init__.py (1)
26-29: Protocol exports are correct, but see naming conflict concern.The imports and
__all__exports properly exposeProtocolDictLikeandProtocolPayloadDatafrom the compute protocol package. However, note the critical naming conflict flagged insrc/omnibase_core/protocols/compute/protocol_payload_data.pywhereProtocolPayloadDataexists in two locations with different interfaces.Also applies to: 41-43
🧹 Nitpick comments (15)
src/omnibase_core/utils/util_tool_logger_code_block.py (1)
29-30: Use the standard context manager protocol signature for__exit__.While replacing
Anywithobjectachieves the immediate goal, the__exit__method should follow Python's context manager protocol with explicit parameter names and types for better type safety:from types import TracebackType def __exit__( self, exc_type: type[BaseException] | None, exc_val: BaseException | None, exc_tb: TracebackType | None, ) -> bool | None: """Exit context manager."""This provides type checkers with accurate information about exception handling and aligns with the standard protocol, even for stub implementations.
🔎 Proposed fix
Add the import at the top of the file:
from __future__ import annotations +from types import TracebackTypeThen update the signature:
- def __exit__(self, *args: object) -> None: + def __exit__( + self, + exc_type: type[BaseException] | None, + exc_val: BaseException | None, + exc_tb: TracebackType | None, + ) -> bool | None: """Exit context manager."""src/omnibase_core/utils/util_conflict_resolver.py (1)
82-82: Make custom_resolver signature explicit.The
Callable[..., object]annotation uses...which accepts any parameters. Since the resolver signature is known (called withexisting_value, new_value, keyon line 134), use an explicit signature for better type safety.🔎 Proposed fix
- custom_resolver: Callable[..., object] | None = None, + custom_resolver: Callable[[object, object, str | None], object] | None = None,src/omnibase_core/utils/compute_transformations.py (1)
362-364: Type annotations improved, minor redundancy noted.The types have been appropriately widened from
Anyto more specific types. Note thatdict[str, object] | objectis technically equivalent to justobjectin Python's type system, but the explicit union serves as useful documentation of the expected input types.Optional: Simplify to just `object` for consistency
If you prefer strictly minimal types without documentation redundancy:
-def transform_json_path( - data: dict[str, object] | object, - config: ModelTransformJsonPathConfig, -) -> object: +def transform_json_path( + data: object, + config: ModelTransformJsonPathConfig, +) -> object:However, the current approach is perfectly acceptable as documentation.
src/omnibase_core/utils/util_streaming_window.py (1)
142-164: Consider usingtyping.NoReturnor a more flexible return type.The
__getattr__function has a return type oftype[UtilStreamingWindow], but it will raiseAttributeErrorfor any name other than"ModelStreamingWindow". This makes the return type annotation misleading.🔎 Suggested options
Option 1: Use a union return type to be more explicit:
-def __getattr__(name: str) -> type[UtilStreamingWindow]: +def __getattr__(name: str) -> type[UtilStreamingWindow] | typing.Never:Option 2: Keep it simple with
objectsince this handles deprecation:-def __getattr__(name: str) -> type[UtilStreamingWindow]: +def __getattr__(name: str) -> object:The current implementation is functionally correct, but the type annotation could be more accurate.
src/omnibase_core/protocols/logging/protocol_minimal_logger.py (1)
35-43: Consider documenting default parameter values in docstrings.The protocol uses ellipsis (
...) as default values forevent_typeand_threshold_msparameters. While this is valid and allows implementations to choose their own defaults, it doesn't communicate what those defaults should be.Based on the implementation in
util_bootstrap.py, actual defaults areevent_type="generic"and_threshold_ms=1000. Consider documenting these expected defaults in the method docstrings to guide implementers.🔎 Example enhancement
def emit_log_event_sync( self, level: EnumLogLevel, message: str, event_type: str = ..., **kwargs: object, ) -> None: - """Synchronous log event emission.""" + """ + Synchronous log event emission. + + Args: + level: Log level + message: Log message + event_type: Event type identifier (default: "generic") + **kwargs: Additional context data + """ ...Also applies to: 45-53, 59-64
src/omnibase_core/models/container/model_service_health_validation_result.py (2)
50-54: Consider making the model immutable (frozen=True).The model is configured with
frozen=Falseand includes mutator methods (add_warning,set_error,add_diagnostic). Per coding guidelines, Pydantic models should usefrozen=Truefor immutable value objects.Result objects are typically immutable to prevent unintended modifications and ensure thread safety. The existing class methods (
healthy,unhealthy,degraded) already provide a factory pattern for construction.🔎 Alternative design approach
Consider refactoring to an immutable model with richer factory methods:
model_config = ConfigDict( extra="ignore", frozen=True, # Make immutable validate_assignment=True, ) # Replace mutators with factory methods that return new instances @classmethod def with_warning( cls, base: "ModelServiceHealthValidationResult", warning: str, ) -> "ModelServiceHealthValidationResult": """Create a new result with an additional warning.""" return cls( **base.model_dump(), warnings=[*base.warnings, warning], )This approach ensures thread safety and aligns with the coding guideline for immutable models. If mutability is essential for the validation workflow, document the rationale.
Also applies to: 107-119
121-167: Consider usingtyping_extensions.Selffor return type annotations.The class methods use string literal forward references (
"ModelServiceHealthValidationResult") for return types. Consider usingSelffromtyping_extensions(ortypingin Python 3.11+) for clearer type annotations.🔎 Modernized approach
+from typing_extensions import Self + @classmethod def healthy( cls, registration_id: UUID, instance_count: int = 1, response_time_ms: float | None = None, -) -> "ModelServiceHealthValidationResult": +) -> Self: """Create a healthy validation result."""This provides better IDE support and type checking without requiring string literals.
src/omnibase_core/models/container/model_injection_context.py (1)
57-61: Document thread safety guarantees for mutable context.Similar to
ModelServiceHealthValidationResult, this model usesfrozen=Falsewith mutator methods. Since this represents an active DI resolution context that tracks state changes, mutability may be justified. However, the coding guidelines preferfrozen=Truefor immutable models.If this context is intended to be used by a single thread during resolution, document that clearly. If it may be accessed concurrently, consider making it immutable or adding synchronization.
Add documentation clarifying the intended usage pattern:
class ModelInjectionContext(BaseModel): """ Context for dependency injection operations. Implements ProtocolInjectionContext. Tracks the state of an injection operation including resolution status, resolved dependencies, and error information. Thread Safety: This context is designed for single-threaded use during service resolution. Each resolution operation should use its own context instance. Do not share contexts across concurrent resolution operations. ... """Also applies to: 113-152
src/omnibase_core/protocols/compute/protocol_payload_data.py (1)
66-72: Consider tightening the return type for dict-like protocols.
ProtocolDictLike.keys()returnsobject, which is very permissive. For dict-like objects, keys are typically strings, so consider using a more specific return type likeIterator[str]orIterable[str]to improve type safety.Proposed refinement
+from typing import Iterator + @runtime_checkable class ProtocolDictLike(Protocol): """ Protocol for objects that support dictionary-like access. ... """ def __getitem__(self, key: str) -> object: """Get a value by key.""" ... - def keys(self) -> object: + def keys(self) -> Iterator[str]: """Return the keys of the container.""" ... def __contains__(self, key: object) -> bool: """Check if a key exists in the container.""" ...src/omnibase_core/protocols/logging/protocol_smart_log_formatter.py (1)
32-32: Type signature inconsistency with existing protocol.The
dataparameter is typed asdict[str, object], but the existingProtocolSmartLogFormatterinsrc/omnibase_core/protocols/protocol_smart_log_formatter.py(lines 53-60) usesdict[str, LogDataValue | None]. This creates a signature mismatch that could cause confusion for implementers.Consider aligning with the more specific type:
🔎 Proposed fix
if TYPE_CHECKING: from omnibase_core.enums.enum_log_level import EnumLogLevel from omnibase_core.models.core.model_log_context import ModelLogContext + from omnibase_core.logging.type_log_data_value import LogDataValue ... def format_log_event( self, level: EnumLogLevel, event_type: str, message: str, context: ModelLogContext, - data: dict[str, object], + data: dict[str, LogDataValue | None], correlation_id: str, ) -> str:src/omnibase_core/types/__init__.py (1)
26-70: Docstring contradicts actual import behavior.The docstring at lines 8-24 states "All constraint imports are now lazy-loaded via getattr", but lines 29-70 perform direct imports from
.constraintsat module level. The__getattr__function at lines 686-754 handles the same symbols, making one of these approaches redundant.Consider either removing the direct imports (relying solely on
__getattr__) or updating the docstring to reflect that imports are now direct.src/omnibase_core/types/typed_dict_yaml_dump_options.py (1)
1-23: Missing__all__export.For consistency with other TypedDict modules and to make the public API explicit, consider adding an
__all__export.🔎 Proposed fix
indent: int width: int + + +__all__ = ["TypedDictYamlDumpOptions"]src/omnibase_core/models/container/model_service_dependency_graph.py (1)
100-113: Consider adding self-reference validation.The mutator methods correctly prevent duplicates, but they don't prevent a service from being added as its own dependency (e.g.,
add_dependency(self.service_id)). If this is a valid scenario that should be caught, consider adding validation.🔎 Proposed enhancement to prevent self-references
def add_dependency(self, service_id: UUID) -> None: """Add a dependency to this service.""" + if service_id == self.service_id: + raise ValueError("Service cannot depend on itself") if service_id not in self.dependencies: self.dependencies.append(service_id) def add_dependent(self, service_id: UUID) -> None: """Add a service that depends on this one.""" + if service_id == self.service_id: + raise ValueError("Service cannot depend on itself") if service_id not in self.dependents: self.dependents.append(service_id)src/omnibase_core/utils/util_safe_yaml_loader.py (1)
254-254: Redundant cast.
yaml.dumpreturnsstrwhen called without thestreamargument, so the cast is unnecessary.🔎 Suggested fix
- return cast("str", yaml_str) + return yaml_strsrc/omnibase_core/utils/compute_path_resolver.py (1)
102-104: Type alias is effectively justobject.The union
dict[str, object] | BaseModel | objectis redundant becauseobjectis a supertype of bothdictandBaseModel. The entire union collapses to justobject.If the intent is to document expected types for consumers, consider using a
Protocolor keeping the alias but understanding it provides no runtime or static type narrowing beyondobject.🔎 Options to consider
Option 1 - Remove redundant
objectto make the union meaningful:-TraversableData = dict[str, object] | BaseModel | object +TraversableData = dict[str, object] | BaseModelOption 2 - If truly any object is acceptable, simplify to just
objectwith a documentation comment:-TraversableData = dict[str, object] | BaseModel | object +# Data that can be traversed: dicts, Pydantic models, or any object with attributes +TraversableData = object
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (44)
src/omnibase_core/container/service_registry.pysrc/omnibase_core/models/container/__init__.pysrc/omnibase_core/models/container/model_injection_context.pysrc/omnibase_core/models/container/model_service_dependency_graph.pysrc/omnibase_core/models/container/model_service_health_validation_result.pysrc/omnibase_core/protocols/__init__.pysrc/omnibase_core/protocols/compute/__init__.pysrc/omnibase_core/protocols/compute/protocol_payload_data.pysrc/omnibase_core/protocols/container/protocol_service_factory.pysrc/omnibase_core/protocols/container/protocol_service_validator.pysrc/omnibase_core/protocols/logging/__init__.pysrc/omnibase_core/protocols/logging/protocol_context_aware_output_handler.pysrc/omnibase_core/protocols/logging/protocol_minimal_logger.pysrc/omnibase_core/protocols/logging/protocol_registry_node.pysrc/omnibase_core/protocols/logging/protocol_smart_log_formatter.pysrc/omnibase_core/protocols/protocol_context_aware_output_handler.pysrc/omnibase_core/protocols/protocol_generation_config.pysrc/omnibase_core/protocols/protocol_import_tracker.pysrc/omnibase_core/protocols/protocol_payload_data.pysrc/omnibase_core/protocols/protocol_smart_log_formatter.pysrc/omnibase_core/runtime/envelope_router.pysrc/omnibase_core/runtime/message_dispatch_engine.pysrc/omnibase_core/types/__init__.pysrc/omnibase_core/types/type_compute_pipeline.pysrc/omnibase_core/types/typed_dict_yaml_dump_options.pysrc/omnibase_core/utils/compute_executor.pysrc/omnibase_core/utils/compute_path_resolver.pysrc/omnibase_core/utils/compute_transformations.pysrc/omnibase_core/utils/fsm_executor.pysrc/omnibase_core/utils/fsm_operators.pysrc/omnibase_core/utils/generation/utility_reference_resolver.pysrc/omnibase_core/utils/util_bootstrap.pysrc/omnibase_core/utils/util_cli_result_formatter.pysrc/omnibase_core/utils/util_conflict_resolver.pysrc/omnibase_core/utils/util_hash.pysrc/omnibase_core/utils/util_logger_cache.pysrc/omnibase_core/utils/util_payload_migration.pysrc/omnibase_core/utils/util_protocol_cache_holder.pysrc/omnibase_core/utils/util_safe_yaml_loader.pysrc/omnibase_core/utils/util_security.pysrc/omnibase_core/utils/util_simple_fallback_logger.pysrc/omnibase_core/utils/util_streaming_window.pysrc/omnibase_core/utils/util_tool_logger_code_block.pytests/unit/container/test_service_registry_extended.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/protocols/compute/protocol_payload_data.pysrc/omnibase_core/protocols/compute/__init__.pysrc/omnibase_core/protocols/protocol_generation_config.pysrc/omnibase_core/utils/util_tool_logger_code_block.pysrc/omnibase_core/types/type_compute_pipeline.pysrc/omnibase_core/protocols/protocol_payload_data.pysrc/omnibase_core/utils/fsm_executor.pysrc/omnibase_core/runtime/envelope_router.pysrc/omnibase_core/types/typed_dict_yaml_dump_options.pysrc/omnibase_core/runtime/message_dispatch_engine.pysrc/omnibase_core/utils/fsm_operators.pysrc/omnibase_core/models/container/model_service_health_validation_result.pysrc/omnibase_core/protocols/protocol_context_aware_output_handler.pysrc/omnibase_core/models/container/model_service_dependency_graph.pysrc/omnibase_core/utils/util_payload_migration.pysrc/omnibase_core/utils/util_simple_fallback_logger.pysrc/omnibase_core/protocols/logging/protocol_smart_log_formatter.pytests/unit/container/test_service_registry_extended.pysrc/omnibase_core/utils/compute_transformations.pysrc/omnibase_core/utils/util_safe_yaml_loader.pysrc/omnibase_core/models/container/__init__.pysrc/omnibase_core/utils/util_security.pysrc/omnibase_core/protocols/logging/protocol_context_aware_output_handler.pysrc/omnibase_core/utils/generation/utility_reference_resolver.pysrc/omnibase_core/models/container/model_injection_context.pysrc/omnibase_core/utils/util_streaming_window.pysrc/omnibase_core/protocols/container/protocol_service_validator.pysrc/omnibase_core/protocols/__init__.pysrc/omnibase_core/protocols/protocol_import_tracker.pysrc/omnibase_core/utils/util_cli_result_formatter.pysrc/omnibase_core/utils/compute_path_resolver.pysrc/omnibase_core/utils/compute_executor.pysrc/omnibase_core/protocols/logging/protocol_registry_node.pysrc/omnibase_core/protocols/logging/protocol_minimal_logger.pysrc/omnibase_core/types/__init__.pysrc/omnibase_core/utils/util_conflict_resolver.pysrc/omnibase_core/container/service_registry.pysrc/omnibase_core/protocols/protocol_smart_log_formatter.pysrc/omnibase_core/utils/util_logger_cache.pysrc/omnibase_core/utils/util_protocol_cache_holder.pysrc/omnibase_core/protocols/logging/__init__.pysrc/omnibase_core/utils/util_bootstrap.pysrc/omnibase_core/protocols/container/protocol_service_factory.pysrc/omnibase_core/utils/util_hash.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/protocols/compute/protocol_payload_data.pysrc/omnibase_core/protocols/compute/__init__.pysrc/omnibase_core/protocols/protocol_generation_config.pysrc/omnibase_core/utils/util_tool_logger_code_block.pysrc/omnibase_core/types/type_compute_pipeline.pysrc/omnibase_core/protocols/protocol_payload_data.pysrc/omnibase_core/utils/fsm_executor.pysrc/omnibase_core/runtime/envelope_router.pysrc/omnibase_core/types/typed_dict_yaml_dump_options.pysrc/omnibase_core/runtime/message_dispatch_engine.pysrc/omnibase_core/utils/fsm_operators.pysrc/omnibase_core/models/container/model_service_health_validation_result.pysrc/omnibase_core/protocols/protocol_context_aware_output_handler.pysrc/omnibase_core/models/container/model_service_dependency_graph.pysrc/omnibase_core/utils/util_payload_migration.pysrc/omnibase_core/utils/util_simple_fallback_logger.pysrc/omnibase_core/protocols/logging/protocol_smart_log_formatter.pysrc/omnibase_core/utils/compute_transformations.pysrc/omnibase_core/utils/util_safe_yaml_loader.pysrc/omnibase_core/models/container/__init__.pysrc/omnibase_core/utils/util_security.pysrc/omnibase_core/protocols/logging/protocol_context_aware_output_handler.pysrc/omnibase_core/utils/generation/utility_reference_resolver.pysrc/omnibase_core/models/container/model_injection_context.pysrc/omnibase_core/utils/util_streaming_window.pysrc/omnibase_core/protocols/container/protocol_service_validator.pysrc/omnibase_core/protocols/__init__.pysrc/omnibase_core/protocols/protocol_import_tracker.pysrc/omnibase_core/utils/util_cli_result_formatter.pysrc/omnibase_core/utils/compute_path_resolver.pysrc/omnibase_core/utils/compute_executor.pysrc/omnibase_core/protocols/logging/protocol_registry_node.pysrc/omnibase_core/protocols/logging/protocol_minimal_logger.pysrc/omnibase_core/types/__init__.pysrc/omnibase_core/utils/util_conflict_resolver.pysrc/omnibase_core/container/service_registry.pysrc/omnibase_core/protocols/protocol_smart_log_formatter.pysrc/omnibase_core/utils/util_logger_cache.pysrc/omnibase_core/utils/util_protocol_cache_holder.pysrc/omnibase_core/protocols/logging/__init__.pysrc/omnibase_core/utils/util_bootstrap.pysrc/omnibase_core/protocols/container/protocol_service_factory.pysrc/omnibase_core/utils/util_hash.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/protocols/compute/protocol_payload_data.pysrc/omnibase_core/protocols/compute/__init__.pysrc/omnibase_core/protocols/protocol_generation_config.pysrc/omnibase_core/utils/util_tool_logger_code_block.pysrc/omnibase_core/types/type_compute_pipeline.pysrc/omnibase_core/protocols/protocol_payload_data.pysrc/omnibase_core/utils/fsm_executor.pysrc/omnibase_core/runtime/envelope_router.pysrc/omnibase_core/types/typed_dict_yaml_dump_options.pysrc/omnibase_core/runtime/message_dispatch_engine.pysrc/omnibase_core/utils/fsm_operators.pysrc/omnibase_core/models/container/model_service_health_validation_result.pysrc/omnibase_core/protocols/protocol_context_aware_output_handler.pysrc/omnibase_core/models/container/model_service_dependency_graph.pysrc/omnibase_core/utils/util_payload_migration.pysrc/omnibase_core/utils/util_simple_fallback_logger.pysrc/omnibase_core/protocols/logging/protocol_smart_log_formatter.pytests/unit/container/test_service_registry_extended.pysrc/omnibase_core/utils/compute_transformations.pysrc/omnibase_core/utils/util_safe_yaml_loader.pysrc/omnibase_core/models/container/__init__.pysrc/omnibase_core/utils/util_security.pysrc/omnibase_core/protocols/logging/protocol_context_aware_output_handler.pysrc/omnibase_core/utils/generation/utility_reference_resolver.pysrc/omnibase_core/models/container/model_injection_context.pysrc/omnibase_core/utils/util_streaming_window.pysrc/omnibase_core/protocols/container/protocol_service_validator.pysrc/omnibase_core/protocols/__init__.pysrc/omnibase_core/protocols/protocol_import_tracker.pysrc/omnibase_core/utils/util_cli_result_formatter.pysrc/omnibase_core/utils/compute_path_resolver.pysrc/omnibase_core/utils/compute_executor.pysrc/omnibase_core/protocols/logging/protocol_registry_node.pysrc/omnibase_core/protocols/logging/protocol_minimal_logger.pysrc/omnibase_core/types/__init__.pysrc/omnibase_core/utils/util_conflict_resolver.pysrc/omnibase_core/container/service_registry.pysrc/omnibase_core/protocols/protocol_smart_log_formatter.pysrc/omnibase_core/utils/util_logger_cache.pysrc/omnibase_core/utils/util_protocol_cache_holder.pysrc/omnibase_core/protocols/logging/__init__.pysrc/omnibase_core/utils/util_bootstrap.pysrc/omnibase_core/protocols/container/protocol_service_factory.pysrc/omnibase_core/utils/util_hash.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/container/model_service_health_validation_result.pysrc/omnibase_core/models/container/model_service_dependency_graph.pysrc/omnibase_core/models/container/__init__.pysrc/omnibase_core/models/container/model_injection_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/container/test_service_registry_extended.py
🧠 Learnings (9)
📓 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
📚 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 duck typing with protocols instead of isinstance checks for type validation
Applied to files:
src/omnibase_core/protocols/protocol_payload_data.pysrc/omnibase_core/runtime/envelope_router.pysrc/omnibase_core/utils/generation/utility_reference_resolver.pysrc/omnibase_core/protocols/container/protocol_service_validator.pysrc/omnibase_core/protocols/logging/protocol_registry_node.pysrc/omnibase_core/utils/util_bootstrap.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:
src/omnibase_core/runtime/envelope_router.pysrc/omnibase_core/utils/util_safe_yaml_loader.pysrc/omnibase_core/utils/compute_path_resolver.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 abstractions for all service interfaces (ProtocolLogger, ProtocolEventBus, etc.) - never use concrete implementations
Applied to files:
src/omnibase_core/protocols/protocol_context_aware_output_handler.pysrc/omnibase_core/protocols/logging/protocol_smart_log_formatter.pysrc/omnibase_core/protocols/logging/protocol_context_aware_output_handler.pysrc/omnibase_core/protocols/container/protocol_service_validator.pysrc/omnibase_core/protocols/__init__.pysrc/omnibase_core/protocols/logging/protocol_registry_node.pysrc/omnibase_core/protocols/logging/protocol_minimal_logger.pysrc/omnibase_core/container/service_registry.pysrc/omnibase_core/protocols/protocol_smart_log_formatter.pysrc/omnibase_core/protocols/logging/__init__.pysrc/omnibase_core/utils/util_bootstrap.pysrc/omnibase_core/protocols/container/protocol_service_factory.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:
src/omnibase_core/models/container/model_service_dependency_graph.pysrc/omnibase_core/models/container/__init__.pysrc/omnibase_core/models/container/model_injection_context.pysrc/omnibase_core/container/service_registry.py
📚 Learning: 2025-09-23T22:28:00.333Z
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`.
Applied to files:
src/omnibase_core/utils/compute_transformations.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/utils/util_safe_yaml_loader.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/models/container/__init__.pysrc/omnibase_core/models/container/model_injection_context.pysrc/omnibase_core/protocols/container/protocol_service_validator.pysrc/omnibase_core/protocols/logging/protocol_registry_node.pysrc/omnibase_core/container/service_registry.pysrc/omnibase_core/protocols/container/protocol_service_factory.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 PEP 604 union syntax (str | None) instead of Optional/Union - enforce via ruff UP007
Applied to files:
src/omnibase_core/utils/generation/utility_reference_resolver.pysrc/omnibase_core/utils/util_cli_result_formatter.pysrc/omnibase_core/utils/util_conflict_resolver.py
🧬 Code graph analysis (25)
src/omnibase_core/protocols/compute/protocol_payload_data.py (1)
src/omnibase_core/protocols/protocol_payload_data.py (2)
keys(73-80)ProtocolPayloadData(28-125)
src/omnibase_core/protocols/compute/__init__.py (1)
src/omnibase_core/protocols/compute/protocol_payload_data.py (2)
ProtocolDictLike(51-76)ProtocolPayloadData(80-118)
src/omnibase_core/protocols/protocol_payload_data.py (3)
src/omnibase_core/protocols/compute/protocol_payload_data.py (2)
ProtocolPayloadData(80-118)keys(70-72)src/omnibase_core/utils/util_logger_cache.py (1)
get(19-25)src/omnibase_core/protocols/event_bus/protocol_event_message.py (1)
key(28-30)
src/omnibase_core/runtime/message_dispatch_engine.py (1)
src/omnibase_core/models/events/model_event_envelope.py (1)
ModelEventEnvelope(33-517)
src/omnibase_core/protocols/protocol_context_aware_output_handler.py (1)
src/omnibase_core/protocols/logging/protocol_context_aware_output_handler.py (2)
ProtocolContextAwareOutputHandler(14-34)output_log_entry(22-34)
src/omnibase_core/models/container/model_service_dependency_graph.py (2)
src/omnibase_core/models/metadata/model_metadata_field_info.py (1)
uuid(157-176)src/omnibase_core/models/node_metadata/model_node_metadata_info.py (1)
dependents(169-171)
src/omnibase_core/utils/util_simple_fallback_logger.py (1)
src/omnibase_core/logging/core_logging.py (2)
_SimpleFallbackLogger(73-86)emit(76-86)
src/omnibase_core/protocols/logging/protocol_smart_log_formatter.py (2)
src/omnibase_core/protocols/protocol_smart_log_formatter.py (2)
ProtocolSmartLogFormatter(27-78)format_log_event(55-78)src/omnibase_core/protocols/event_bus/protocol_event_bus_headers.py (1)
event_type(55-57)
tests/unit/container/test_service_registry_extended.py (2)
src/omnibase_core/container/service_registry.py (1)
validate_service_health(812-883)tests/unit/container/test_service_registry.py (2)
MockServiceImplementation(20-31)ITestService(12-16)
src/omnibase_core/utils/compute_transformations.py (2)
src/omnibase_core/models/transformations/model_transform_json_path_config.py (1)
ModelTransformJsonPathConfig(13-37)src/omnibase_core/enums/enum_transformation_type.py (1)
EnumTransformationType(11-34)
src/omnibase_core/utils/util_safe_yaml_loader.py (1)
src/omnibase_core/models/utils/model_yaml_value.py (2)
ModelYamlValue(17-123)to_serializable(70-93)
src/omnibase_core/models/container/__init__.py (3)
src/omnibase_core/models/container/model_injection_context.py (1)
ModelInjectionContext(27-152)src/omnibase_core/models/container/model_service_dependency_graph.py (1)
ModelServiceDependencyGraph(22-113)src/omnibase_core/models/container/model_service_health_validation_result.py (1)
ModelServiceHealthValidationResult(19-167)
src/omnibase_core/utils/util_security.py (1)
src/omnibase_core/models/security/model_security_policy_data.py (1)
data(27-32)
src/omnibase_core/protocols/logging/protocol_context_aware_output_handler.py (1)
src/omnibase_core/protocols/protocol_context_aware_output_handler.py (2)
ProtocolContextAwareOutputHandler(20-59)output_log_entry(47-59)
src/omnibase_core/utils/generation/utility_reference_resolver.py (2)
src/omnibase_core/protocols/protocol_generation_config.py (1)
ProtocolGenerationConfig(13-28)src/omnibase_core/protocols/protocol_import_tracker.py (1)
ProtocolImportTracker(13-36)
src/omnibase_core/protocols/container/protocol_service_validator.py (3)
src/omnibase_core/protocols/container/protocol_validation_result.py (1)
ProtocolValidationResult(22-57)src/omnibase_core/protocols/validation/protocol_validation_result.py (1)
ProtocolValidationResult(27-81)src/omnibase_core/protocols/container/protocol_service_dependency.py (1)
ProtocolServiceDependency(17-40)
src/omnibase_core/protocols/logging/protocol_registry_node.py (1)
src/omnibase_core/utils/util_bootstrap.py (1)
get_service(66-94)
src/omnibase_core/protocols/logging/protocol_minimal_logger.py (3)
src/omnibase_core/utils/util_bootstrap.py (7)
emit_log_event(23-29)emit_log_event(118-141)emit_log_event_sync(32-38)emit_log_event_sync(144-171)emit_log_event_async(41-47)trace_function_lifecycle(50-52)tool_logger_performance_metrics(55-63)src/omnibase_core/protocols/event_bus/protocol_event_bus_headers.py (1)
event_type(55-57)src/omnibase_core/models/errors/model_onex_error.py (1)
message(248-250)
src/omnibase_core/types/__init__.py (1)
src/omnibase_core/types/typed_dict_yaml_dump_options.py (1)
TypedDictYamlDumpOptions(10-23)
src/omnibase_core/container/service_registry.py (6)
src/omnibase_core/models/container/model_injection_context.py (1)
ModelInjectionContext(27-152)src/omnibase_core/models/container/model_service_dependency_graph.py (1)
ModelServiceDependencyGraph(22-113)src/omnibase_core/models/container/model_service_health_validation_result.py (4)
ModelServiceHealthValidationResult(19-167)unhealthy(138-151)degraded(154-167)healthy(122-135)src/omnibase_core/models/container/model_service_instance.py (2)
ModelServiceInstance(13-93)is_active(76-83)src/omnibase_core/protocols/container/protocol_service_factory.py (1)
ProtocolServiceFactory(25-75)src/omnibase_core/protocols/container/protocol_service_validator.py (1)
ProtocolServiceValidator(29-83)
src/omnibase_core/protocols/protocol_smart_log_formatter.py (4)
src/omnibase_core/protocols/logging/protocol_smart_log_formatter.py (2)
ProtocolSmartLogFormatter(18-49)format_log_event(26-49)src/omnibase_core/protocols/event_bus/protocol_event_bus_headers.py (1)
event_type(55-57)src/omnibase_core/models/errors/model_onex_error.py (2)
message(248-250)context(275-278)src/omnibase_core/models/security/model_security_policy_data.py (1)
data(27-32)
src/omnibase_core/utils/util_logger_cache.py (4)
src/omnibase_core/utils/util_action_registry_holder.py (1)
get(39-50)src/omnibase_core/utils/util_event_type_registry_holder.py (1)
get(41-52)src/omnibase_core/utils/util_secret_manager_holder.py (1)
get(39-50)src/omnibase_core/utils/util_command_registry_holder.py (1)
get(41-52)
src/omnibase_core/utils/util_protocol_cache_holder.py (2)
src/omnibase_core/protocols/logging/protocol_context_aware_output_handler.py (1)
ProtocolContextAwareOutputHandler(14-34)src/omnibase_core/protocols/logging/protocol_smart_log_formatter.py (1)
ProtocolSmartLogFormatter(18-49)
src/omnibase_core/protocols/logging/__init__.py (5)
src/omnibase_core/models/core/model_capability.py (1)
logging(194-196)src/omnibase_core/protocols/logging/protocol_context_aware_output_handler.py (1)
ProtocolContextAwareOutputHandler(14-34)src/omnibase_core/protocols/logging/protocol_minimal_logger.py (1)
ProtocolMinimalLogger(17-64)src/omnibase_core/protocols/logging/protocol_registry_node.py (1)
ProtocolRegistryNode(16-54)src/omnibase_core/protocols/logging/protocol_smart_log_formatter.py (1)
ProtocolSmartLogFormatter(18-49)
src/omnibase_core/utils/util_bootstrap.py (3)
src/omnibase_core/protocols/logging/protocol_registry_node.py (1)
ProtocolRegistryNode(16-54)src/omnibase_core/protocols/logging/protocol_minimal_logger.py (2)
trace_function_lifecycle(55-57)tool_logger_performance_metrics(59-64)src/omnibase_core/logging/emit.py (2)
trace_function_lifecycle(222-296)decorator(392-432)
Resolved conflict in protocols/__init__.py by including both: - ProtocolLoggerLike (from main) - ProtocolContextAwareOutputHandler, ProtocolSmartLogFormatter, etc. (from this branch)
Code Review - PR #300: Remove Any from utils, runtime, and containerOverviewThis PR successfully removes 149 ✅ Strengths1. Excellent Protocol DesignThe new protocols follow best practices consistently:
Example from @overload
def get(self, key: str) -> PayloadValue | None: ...
@overload
def get(self, key: str, default: _T) -> PayloadValue | _T: ...This is textbook-perfect overload usage for type safety. 2. Implementation QualityThe implementations of previously stubbed methods are well-crafted:
3. Test CoverageOutstanding test coverage in
4. Type Safety ImprovementsMinimal use of instances.append(cast(TInterface, instance)) # Justified - generic resolution
sync_handler = cast(SyncHandlerFunc, handler) # Justified - type narrowingBoth casts are legitimate type narrowing, not 5. Architectural Consistency
|
CRITICAL: - Rename ProtocolPayloadData → ProtocolComputePayloadData in compute module to resolve naming conflict with protocols.ProtocolPayloadData MAJOR: - Remove redundant `| object` from PipelineData union type - Consolidate duplicate ProtocolContextAwareOutputHandler to single location MINOR: - Remove type redundancy in LogDataValue usage (already includes None) - Add None to type annotation in util_logger_cache.py - Delete unused util_simple_fallback_logger.py (dead code) NITPICKS: - Use standard __exit__ context manager signature - Make custom_resolver signature explicit with validation - Fix typos in util_safe_yaml_loader.py - Tighten ProtocolDictLike.keys() return type to Iterator[str] - Add docstrings for default parameter values - Add thread safety documentation to ModelInjectionContext - Add self-reference validation to ModelServiceDependencyGraph - Add __all__ export to typed_dict_yaml_dump_options.py - Fix misleading docstring in types/__init__.py
PR Review: Type Safety Enhancement (OMN-1179)This is an excellent PR that significantly improves type safety in the codebase. The removal of 149 ✅ Strengths1. Comprehensive Protocol Design
2. Excellent Model Implementation
3. Service Registry Improvements
4. Type Safety Verification
5. Code Quality
🔍 Observations & Minor Suggestions1. Thread Safety Documentation (ModelInjectionContext:56-60)
This aligns with the ONEX threading guide. Consider adding similar warnings to 2. Implementation Completeness (service_registry.py:718-749) metadata={"note": "Dependency tracking not implemented in v1.0"}This is appropriate for v1.0 scope. For v2.0, consider tracking actual dependencies during 3. Test Coverage (test_service_registry_extended.py)
4. Protocol Naming Consistency
The PR description mentions resolving this naming conflict. Verify that both are needed or if one should be deprecated. 5. PEP 604 Compliance 🛡️ Security & PerformanceSecurity: ✅ No security concerns identified
Performance: ✅ No performance concerns
📋 Checklist VerificationPer CLAUDE.md standards:
🎯 RecommendationsBefore Merge:
Post-Merge:
🏆 Overall AssessmentQuality: ⭐⭐⭐⭐⭐ (5/5) Verdict: APPROVE ✅ This PR represents exemplary type safety work. The removal of 149 Great work on maintaining 0 mypy errors across the entire codebase while making such significant type system changes! Reviewed by: Claude Code (Sonnet 4.5) |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/omnibase_core/types/type_compute_pipeline.py (1)
40-49: Past issue resolved:| objectsuccessfully removed from PipelineData.The redundant
| objecthas been correctly removed from the union type, and the inline documentation clearly explains the rationale. This allows type checkers to provide meaningful narrowing betweendict[str, object]andBaseModel.
🧹 Nitpick comments (9)
src/omnibase_core/types/type_compute_pipeline.py (1)
59-61: Consider a more specific type for JSON-compatible values.While
objectaligns with the design principles, if path resolution primarily returns JSON-compatible values, a union likestr | int | float | bool | None | dict[str, object] | list[object]would provide better type safety and documentation. However, if truly arbitrary Python objects (beyond JSON types) are resolved, the currentobjecttype is appropriate.src/omnibase_core/utils/util_conflict_resolver.py (1)
95-103: Excellent validation with proper error handling.The validation correctly enforces that CUSTOM strategy requires a custom_resolver, using ModelOnexError with the appropriate error code and context per coding guidelines.
Minor observation: Given this validation, the check on line 146 (
and self.custom_resolver) is now redundant—when strategy is CUSTOM, custom_resolver is guaranteed to be not None. Consider simplifying to:if self.strategy == EnumConflictResolution.CUSTOM: return self.custom_resolver(existing_value, new_value, key)However, the current defensive check is also acceptable.
src/omnibase_core/protocols/compute/protocol_payload_data.py (1)
77-79: ConsiderIterable[str]instead ofIterator[str]for broader dict compatibility.The
keys()method returnsIterator[str], but Python'sdict.keys()actually returnsKeysView[str](a view type that'sIterablebut not strictly anIterator). While runtime checks and structural subtyping typically handle this, usingIterable[str]orCollection[str]would provide more precise typing and broader compatibility with dict-like objects.Note: This pattern is consistent with
src/omnibase_core/protocols/protocol_payload_data.pywhich also usesIterator[str], so this may be an intentional design choice across the codebase.🔎 Suggested type adjustment
+from collections.abc import Iterable -from collections.abc import Iterator - def keys(self) -> Iterator[str]: + def keys(self) -> Iterable[str]: """Return the keys of the container.""" ...src/omnibase_core/models/container/model_injection_context.py (2)
63-67: Consider addingfrom_attributes=Truefor compatibility.While the current configuration is appropriate for a mutable model, consider adding
from_attributes=Trueto ConfigDict for better compatibility with pytest-xdist and ORM/dataclass conversions, as indicated in the coding guidelines.🔎 Optional enhancement
model_config = ConfigDict( extra="ignore", frozen=False, validate_assignment=True, + from_attributes=True, )As per coding guidelines, this helps with immutable value objects nested in Pydantic models used with pytest-xdist.
105-107: Verify thehas_errorproperty logic covers all error statuses.The property checks
resolution_status == "failed" or self.error_details is not None, but based on the mutator methods, there are additional error statuses: "circular_dependency", "missing_dependency", and "type_mismatch". The current logic relies onerror_detailsbeing set to catch these cases, which is implicit rather than explicit.Consider making the error detection more explicit:
🔎 More explicit error detection
@property def has_error(self) -> bool: """Check if resolution encountered an error.""" - return self.resolution_status == "failed" or self.error_details is not None + return self.resolution_status in ( + "failed", + "circular_dependency", + "missing_dependency", + "type_mismatch", + )src/omnibase_core/utils/compute_executor.py (3)
53-65: Reorganize imports and type alias for better code organization.The
BaseModelimport andPipelineDataTypetype alias are placed in the middle of the import block, which breaks standard Python import organization conventions (stdlib → third-party → local → module-level definitions).📋 Suggested reorganization
Move the
BaseModelimport up with other third-party imports, and move the type alias definition after all imports:from collections.abc import Mapping from concurrent.futures import ThreadPoolExecutor from concurrent.futures import TimeoutError as FuturesTimeoutError - -from pydantic import BaseModel - -from omnibase_core.types.typed_dict_mapping_result import MappingResultDict - -logger = logging.getLogger(__name__) - -# Type alias for pipeline data - can be dict, Pydantic model, or arbitrary object -# Note: `| object` is intentionally kept here (unlike PipelineData in types module) -# because execute_transformation() can return any type (e.g., JSON_PATH extracts -# arbitrary values from nested structures). This local type differs from the -# stricter PipelineData type alias used for documentation purposes. -PipelineDataType = dict[str, object] | BaseModel | object +from pydantic import BaseModel + +from omnibase_core.types.typed_dict_mapping_result import MappingResultDict from omnibase_core.enums.enum_compute_step_type import EnumComputeStepType from omnibase_core.enums.enum_core_error_code import EnumCoreErrorCode +# ... rest of local imports ... from omnibase_core.utils.compute_transformations import execute_transformation +logger = logging.getLogger(__name__) + +# Type alias for pipeline data - can be dict, Pydantic model, or arbitrary object +# Note: `| object` is intentionally kept here (unlike PipelineData in types module) +# because execute_transformation() can return any type (e.g., JSON_PATH extracts +# arbitrary values from nested structures). This local type differs from the +# stricter PipelineData type alias used for documentation purposes. +PipelineDataType = dict[str, object] | BaseModel | object
60-65: Consider simplifying PipelineDataType for clearer type semantics.The type alias
dict[str, object] | BaseModel | objectis semantically equivalent to justobjectbecause bothdict[str, object]andBaseModelare subtypes ofobject. While the comment indicates this broad acceptance is intentional to accommodate arbitrary types fromexecute_transformation(), the current union form may be misleading—it appears to restrict to three specific types while actually accepting any type.For clarity and to better align with the PR's goal of strengthening type safety, consider either:
If truly any type must be accepted: Use
objectdirectly and document common cases in a comment:# Pipeline data type - typically dict[str, object] or BaseModel, but # execute_transformation() can return arbitrary types (e.g., JSON_PATH extracts) PipelineDataType: TypeAlias = objectIf types can be restricted: Remove
| objectto enforce stricter typing:PipelineDataType: TypeAlias = dict[str, object] | BaseModelThis would require verifying that all
execute_transformation()return types are compatible.
309-314: Optional: Simplify return type union.The return type
PipelineDataType | MappingResultDictis technically redundant sincePipelineDataTypealready includesobject, which coversMappingResultDict. You could simplify to justPipelineDataType:-) -> PipelineDataType | MappingResultDict: +) -> PipelineDataType:However, the explicit union may provide documentation value by highlighting that mapping steps specifically return
MappingResultDict. This is a minor style preference.src/omnibase_core/utils/util_safe_yaml_loader.py (1)
219-254: Consider removing redundant cast.The variable
yaml_stris already annotated asstron line 219, andyaml.dump()returnsstrwhen no stream is provided. Thecast("str", yaml_str)on line 254 is redundant.🔎 Proposed fix
- return cast("str", yaml_str) + return yaml_str
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
src/omnibase_core/models/container/model_injection_context.pysrc/omnibase_core/models/container/model_service_dependency_graph.pysrc/omnibase_core/protocols/__init__.pysrc/omnibase_core/protocols/compute/__init__.pysrc/omnibase_core/protocols/compute/protocol_payload_data.pysrc/omnibase_core/protocols/logging/__init__.pysrc/omnibase_core/protocols/logging/protocol_minimal_logger.pysrc/omnibase_core/protocols/logging/protocol_smart_log_formatter.pysrc/omnibase_core/protocols/protocol_smart_log_formatter.pysrc/omnibase_core/types/__init__.pysrc/omnibase_core/types/type_compute_pipeline.pysrc/omnibase_core/types/typed_dict_yaml_dump_options.pysrc/omnibase_core/utils/compute_executor.pysrc/omnibase_core/utils/util_conflict_resolver.pysrc/omnibase_core/utils/util_logger_cache.pysrc/omnibase_core/utils/util_safe_yaml_loader.pysrc/omnibase_core/utils/util_simple_fallback_logger.pysrc/omnibase_core/utils/util_streaming_window.pysrc/omnibase_core/utils/util_tool_logger_code_block.py
💤 Files with no reviewable changes (1)
- src/omnibase_core/utils/util_simple_fallback_logger.py
🚧 Files skipped from review as they are similar to previous changes (5)
- src/omnibase_core/models/container/model_service_dependency_graph.py
- src/omnibase_core/protocols/logging/init.py
- src/omnibase_core/types/init.py
- src/omnibase_core/types/typed_dict_yaml_dump_options.py
- src/omnibase_core/protocols/logging/protocol_smart_log_formatter.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/protocols/compute/__init__.pysrc/omnibase_core/types/type_compute_pipeline.pysrc/omnibase_core/protocols/protocol_smart_log_formatter.pysrc/omnibase_core/protocols/logging/protocol_minimal_logger.pysrc/omnibase_core/utils/compute_executor.pysrc/omnibase_core/protocols/__init__.pysrc/omnibase_core/utils/util_tool_logger_code_block.pysrc/omnibase_core/utils/util_safe_yaml_loader.pysrc/omnibase_core/models/container/model_injection_context.pysrc/omnibase_core/utils/util_conflict_resolver.pysrc/omnibase_core/protocols/compute/protocol_payload_data.pysrc/omnibase_core/utils/util_streaming_window.pysrc/omnibase_core/utils/util_logger_cache.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/protocols/compute/__init__.pysrc/omnibase_core/types/type_compute_pipeline.pysrc/omnibase_core/protocols/protocol_smart_log_formatter.pysrc/omnibase_core/protocols/logging/protocol_minimal_logger.pysrc/omnibase_core/utils/compute_executor.pysrc/omnibase_core/protocols/__init__.pysrc/omnibase_core/utils/util_tool_logger_code_block.pysrc/omnibase_core/utils/util_safe_yaml_loader.pysrc/omnibase_core/models/container/model_injection_context.pysrc/omnibase_core/utils/util_conflict_resolver.pysrc/omnibase_core/protocols/compute/protocol_payload_data.pysrc/omnibase_core/utils/util_streaming_window.pysrc/omnibase_core/utils/util_logger_cache.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/protocols/compute/__init__.pysrc/omnibase_core/types/type_compute_pipeline.pysrc/omnibase_core/protocols/protocol_smart_log_formatter.pysrc/omnibase_core/protocols/logging/protocol_minimal_logger.pysrc/omnibase_core/utils/compute_executor.pysrc/omnibase_core/protocols/__init__.pysrc/omnibase_core/utils/util_tool_logger_code_block.pysrc/omnibase_core/utils/util_safe_yaml_loader.pysrc/omnibase_core/models/container/model_injection_context.pysrc/omnibase_core/utils/util_conflict_resolver.pysrc/omnibase_core/protocols/compute/protocol_payload_data.pysrc/omnibase_core/utils/util_streaming_window.pysrc/omnibase_core/utils/util_logger_cache.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/container/model_injection_context.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/**/*.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 : Use Protocol-driven dependency injection with `container.get_service('ProtocolName')` - never use concrete class names or isinstance checks
📚 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 PEP 604 union syntax (str | None) instead of Optional/Union - enforce via ruff UP007
Applied to files:
src/omnibase_core/types/type_compute_pipeline.pysrc/omnibase_core/protocols/protocol_smart_log_formatter.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 abstractions for all service interfaces (ProtocolLogger, ProtocolEventBus, etc.) - never use concrete implementations
Applied to files:
src/omnibase_core/protocols/protocol_smart_log_formatter.pysrc/omnibase_core/protocols/logging/protocol_minimal_logger.pysrc/omnibase_core/protocols/__init__.pysrc/omnibase_core/protocols/compute/protocol_payload_data.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/utils/util_safe_yaml_loader.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:
src/omnibase_core/utils/util_safe_yaml_loader.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:
src/omnibase_core/models/container/model_injection_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/**/*.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/models/container/model_injection_context.py
🧬 Code graph analysis (8)
src/omnibase_core/protocols/compute/__init__.py (1)
src/omnibase_core/protocols/compute/protocol_payload_data.py (2)
ProtocolComputePayloadData(87-130)ProtocolDictLike(58-83)
src/omnibase_core/protocols/protocol_smart_log_formatter.py (2)
src/omnibase_core/protocols/logging/protocol_smart_log_formatter.py (2)
ProtocolSmartLogFormatter(22-53)format_log_event(30-53)src/omnibase_core/protocols/event_bus/protocol_event_bus_headers.py (1)
event_type(55-57)
src/omnibase_core/utils/compute_executor.py (2)
src/omnibase_core/models/compute/model_compute_step_result.py (1)
ModelComputeStepResult(64-122)src/omnibase_core/models/contracts/subcontracts/model_compute_pipeline_step.py (1)
ModelComputePipelineStep(81-207)
src/omnibase_core/protocols/__init__.py (7)
src/omnibase_core/protocols/protocol_context_aware_output_handler.py (1)
ProtocolContextAwareOutputHandler(20-59)src/omnibase_core/protocols/protocol_logger_like.py (1)
ProtocolLoggerLike(18-51)src/omnibase_core/protocols/protocol_from_event.py (1)
ProtocolFromEvent(28-45)src/omnibase_core/protocols/protocol_generation_config.py (1)
ProtocolGenerationConfig(13-28)src/omnibase_core/protocols/protocol_import_tracker.py (1)
ProtocolImportTracker(13-36)src/omnibase_core/protocols/protocol_payload_data.py (1)
ProtocolPayloadData(28-125)src/omnibase_core/protocols/logging/protocol_smart_log_formatter.py (1)
ProtocolSmartLogFormatter(22-53)
src/omnibase_core/utils/util_safe_yaml_loader.py (2)
src/omnibase_core/models/utils/model_yaml_value.py (2)
ModelYamlValue(17-123)to_serializable(70-93)src/omnibase_core/types/typed_dict_yaml_dump_options.py (1)
TypedDictYamlDumpOptions(10-23)
src/omnibase_core/utils/util_conflict_resolver.py (3)
src/omnibase_core/enums/enum_reducer_types.py (1)
EnumConflictResolution(26-33)src/omnibase_core/models/errors/model_onex_error.py (4)
ModelOnexError(49-354)error_code(253-255)message(248-250)context(275-278)src/omnibase_core/enums/enum_core_error_code.py (1)
EnumCoreErrorCode(9-241)
src/omnibase_core/protocols/compute/protocol_payload_data.py (2)
src/omnibase_core/protocols/event_bus/protocol_event_message.py (1)
key(28-30)src/omnibase_core/protocols/protocol_payload_data.py (1)
keys(73-80)
src/omnibase_core/utils/util_logger_cache.py (4)
src/omnibase_core/utils/util_action_registry_holder.py (1)
get(39-50)src/omnibase_core/utils/util_command_registry_holder.py (1)
get(41-52)src/omnibase_core/utils/util_event_type_registry_holder.py (1)
get(41-52)src/omnibase_core/utils/util_secret_manager_holder.py (1)
get(39-50)
⏰ 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 (27)
src/omnibase_core/utils/util_tool_logger_code_block.py (4)
3-3: LGTM!The
TracebackTypeimport is correctly added to support the improved type annotations in__exit__.
22-24: LGTM!Replacing
Anywithobjectfor variadic parameters appropriately removes theAnytype while maintaining the stub's ability to accept arbitrary arguments. The documented stub implementation justifies this minimal signature.
27-29: LGTM!The return type correctly specifies
ToolLoggerCodeBlock, which aligns with the context manager protocol where__enter__returnsself. This removesAnywhile improving type safety.
31-36: Excellent type safety improvement for context manager protocol.The
__exit__signature now properly specifies the standard exception context parameters with correct types, replacing the previous generic signature. This follows Python's context manager protocol specification and uses PEP 604 union syntax as per coding guidelines.src/omnibase_core/utils/util_streaming_window.py (2)
92-92: LGTM! Consistent typing improvements across the window class.The transition from
Anytoobjectfor buffer contents, method parameters, and return types is appropriate for this generic container. The window stores items without inspecting them, soobjectcorrectly represents "any object" while being more type-safe thanAny.Also applies to: 95-95, 112-112
142-142: LGTM! Appropriate return type for module-level lazy loader.The
objectreturn type correctly represents that__getattr__returns theUtilStreamingWindowclass for the deprecated alias. This is more precise thanAny.src/omnibase_core/utils/util_conflict_resolver.py (4)
82-93: LGTM! Explicit callable signature improves type safety.The custom_resolver parameter now has an explicit type signature instead of
Any, making the expected function contract clear. The documentation properly describes both the signature and the new validation requirement.
110-113: LGTM! Replacing Any with object improves type safety.The change from
Anytoobjectfor parameters and return type is appropriate here, as the resolver needs to handle various types dynamically. The isinstance checks in_merge_values(lines 163-178) correctly handle the runtime type checking needed withobjecttypes.
151-151: LGTM! Consistent typing with resolve method.The signature change is consistent with the
resolvemethod updates, maintaining uniform typing across the class's public and private APIs.
184-184: LGTM! Correct return type for class alias.The return type
type[UtilConflictResolver]correctly indicates that this function returns the class itself (not an instance), which is appropriate for the deprecated alias pattern implemented here.src/omnibase_core/utils/util_logger_cache.py (2)
15-15: Previous review comment addressed.The type annotation now correctly includes
None, addressing the past review feedback.
19-19: No changes needed—the genericobjecttype is appropriate for this bootstrap-phase cache.The original comment references "ProtocolLogger," which does not exist in the codebase. More importantly, the
objecttype here is intentional:_LoggerCacheexplicitly handles "fallback mechanisms for bootstrap and circular dependency scenarios" where different logger implementations may be cached at different stages of initialization. Using a specific protocol (likeProtocolLoggerLikeorProtocolMinimalLogger) would unnecessarily constrain flexibility during bootstrap when the logging infrastructure is still being initialized and circular dependencies are being resolved.Likely an incorrect or invalid review comment.
src/omnibase_core/protocols/logging/protocol_minimal_logger.py (1)
1-94: LGTM! Well-designed bootstrap logging protocol.The protocol correctly uses
objectinstead ofAnyfor kwargs (lines 30, 40, 58), applies modern PEP 695 type parameter syntax (line 71), and provides a clear minimal interface for bootstrap-phase logging scenarios. The documentation clearly explains the protocol's purpose and the method signatures are type-safe.src/omnibase_core/protocols/protocol_smart_log_formatter.py (1)
1-81: LGTM! Previous type redundancy issue resolved.The protocol correctly defines
LogDataValuewith None included (line 23) and uses it consistently without redundant unions. The past review concern aboutdict[str, LogDataValue | None]has been addressed—line 61 now correctly showsdict[str, LogDataValue]without the redundant| None. The protocol follows type safety standards and provides clear documentation.src/omnibase_core/protocols/compute/__init__.py (1)
26-29: LGTM! Clean protocol exposure.The imports and exports correctly expose
ProtocolComputePayloadDataandProtocolDictLikefrom the new protocol_payload_data module, maintaining consistency with the package's public API structure.Also applies to: 41-42
src/omnibase_core/protocols/compute/protocol_payload_data.py (1)
86-130: LGTM! Marker protocol design is well-documented.The marker protocol design is clearly explained with appropriate documentation about its intentional minimalism to support polymorphic data types (dicts, Pydantic models, regular objects). The distinction from
ProtocolPayloadDatais clearly documented, addressing the previous naming conflict concern through the rename toProtocolComputePayloadData.src/omnibase_core/protocols/__init__.py (1)
163-189: LGTM! Comprehensive protocol API surface expansion.The imports and exports correctly expose all new protocols introduced in this PR:
- Logging:
ProtocolContextAwareOutputHandler,ProtocolSmartLogFormatter,LogDataValue- Data:
ProtocolPayloadData,PayloadValue- Generation:
ProtocolGenerationConfig,ProtocolImportTrackerThe organization into logical sections with clear comments maintains readability and aligns with the PR's goal of strengthening the public protocol API surface.
Also applies to: 348-368
src/omnibase_core/models/container/model_injection_context.py (5)
1-24: LGTM: Clear documentation and proper imports.The module docstring effectively explains the model's purpose, and imports follow project conventions. Correctly avoids
from __future__ import annotationsfor Pydantic models.
119-142: LGTM: Mutator methods are well-designed.All mutator methods consistently maintain the relationship between
resolution_statusanderror_details. The methods follow a clear pattern and correctly handle state transitions.
144-158: LGTM: Helper methods provide clean, focused functionality.Both
add_to_pathandadd_resolved_dependencyare well-designed:
add_to_pathcorrectly detects circular dependencies without side effects, allowing callers to handle the situation appropriately.add_resolved_dependencyprovides type-safe dependency tracking.
161-161: LGTM: Proper explicit export.The
__all__declaration correctly exports the public API surface.
83-85: No changes required. Theresolution_statusfield correctly has no default value.
LiteralServiceResolutionStatusdefines only terminal resolution states: "resolved", "failed", "circular_dependency", "missing_dependency", "type_mismatch". These represent completed resolution outcomes, not provisional states. AModelInjectionContextmust be created with an explicit final status, as shown in the docstring example. The design is correct.Likely an incorrect or invalid review comment.
src/omnibase_core/utils/compute_executor.py (1)
242-248: Good use of PEP 695 generic type parameter.The generic type parameter
[ValidationT]correctly preserves the input type through the validation step, enabling better type inference at call sites while maintaining the pass-through semantics. This is an appropriate use of PEP 695 syntax.src/omnibase_core/utils/util_safe_yaml_loader.py (4)
11-11: Strong type safety improvement with TypedDict-based YAML options.The replacement of
AnywithUnpackand the introduction ofTypedDictYamlDumpOptionsprovides compile-time type checking for YAML dump parameters, eliminating a common source of runtime errors from invalid yaml.dump options.Also applies to: 23-23
266-315: LGTM - Type-safe YAML serialization for Pydantic models.The function correctly forwards type-safe YAML options to the centralized dumper while maintaining comprehensive error handling with
ModelOnexErroras required by coding guidelines.
318-358: LGTM - Consistent type-safe approach for arbitrary data serialization.The function mirrors the type-safe approach of
serialize_pydantic_model_to_yaml, providing a unified interface for YAML serialization with proper error handling.
361-503: LGTM - Secure YAML loading with proper validation.The function correctly uses
yaml.safe_load()for secure YAML parsing and provides comprehensive error handling. The mentions ofdict[str, Any]in error messages (line 422) and comments (line 437) are descriptive text, not type annotations, which is appropriate for user-facing messages.
1 participant
Summary
Remove all
Anytype annotations fromutils/,runtime/, andcontainer/directories per ONEX type safety standards (OMN-1179).Key Changes:
Anytype annotationsNew Protocols Created
ProtocolPayloadDataProtocolSmartLogFormatterProtocolContextAwareOutputHandlerProtocolServiceValidatorProtocolServiceFactoryProtocolGenerationConfigProtocolImportTrackerNew Models Created
ModelInjectionContextModelServiceDependencyGraphModelServiceHealthValidationResultTest Plan
Notes
Pre-existing hook failures (scattered models in
pipeline/, duplicate model filenames, backward compat patterns) are unrelated to this PR and tracked separately.Closes OMN-1179
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.