Repository navigation
refactor(types): reduce union complexity with type aliases [OMN-1358] - #157
Conversation
Reduce non-optional unions from 122 to 63 (48% reduction) by introducing centralized type aliases that consolidate repeated union patterns. New type aliases in types/type_infra_aliases.py: - MessageOutputCategory: EnumMessageCategory | EnumNodeOutputType (31 uses) - PathLike: Path | str (11 uses) - PolicyTypeInput: EnumPolicyType | str (11 uses) - ASTFunctionDef: ast.AsyncFunctionDef | ast.FunctionDef (10 uses) - VersionInput: ModelSemVer | str (available for future use) Also applied PrimitiveValue from omnibase_core for float | int | str patterns. Updated INFRA_MAX_UNIONS threshold from 122 to 70 to reflect the new baseline with appropriate buffer for minor growth.
|
Warning Rate limit exceeded
⌛ 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 (3)
📝 WalkthroughWalkthroughCentralized repeated union types into new aliases (PolicyTypeInput, MessageOutputCategory, PathInput, ASTFunctionDef) and propagated those aliases across many modules; switched several log-context types to PrimitiveValue, lowered INFRA_MAX_UNIONS from 122 to 70, and added tests for the types export surface. Changes
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Comment |
PR Review: Type Alias Union Reduction (OMN-1358)SummaryThis PR successfully reduces non-optional union complexity from 122 to 63 (48% reduction) by introducing centralized type aliases. The implementation aligns well with ONEX infrastructure guidelines and demonstrates strong adherence to code quality standards. ✅ Strengths1. Excellent CLAUDE.md Compliance
2. Well-Designed Type AliasesThe new aliases address real code duplication with clear semantic meaning:
3. Integration 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: 1
🤖 Fix all issues with AI agents
In `@src/omnibase_infra/types/type_infra_aliases.py`:
- Around line 20-41: The VersionInput type alias currently uses ModelSemVer
which is only imported under TYPE_CHECKING causing a NameError at import; change
the definition to defer evaluation by using typing.TypeAlias with a string
forward reference or import ModelSemVer at runtime: keep the TYPE_CHECKING
import as-is, add "from typing import TypeAlias" (or import ModelSemVer
normally), and replace the current "type VersionInput = ModelSemVer | str" with
a string-based alias like VersionInput: TypeAlias = "ModelSemVer | str" so the
runtime import error is avoided while preserving the intended type.
♻️ Duplicate comments (1)
src/omnibase_infra/models/validation/model_category_match_result.py (1)
50-56: Same PEP 695 compatibility concern as other new aliases.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Lite
📒 Files selected for processing (17)
src/omnibase_infra/errors/error_policy_registry.pysrc/omnibase_infra/models/dispatch/model_dispatch_log_context.pysrc/omnibase_infra/models/logging/model_log_context.pysrc/omnibase_infra/models/validation/model_category_match_result.pysrc/omnibase_infra/runtime/message_dispatch_engine.pysrc/omnibase_infra/runtime/models/model_policy_key.pysrc/omnibase_infra/runtime/models/model_policy_registration.pysrc/omnibase_infra/runtime/policy_registry.pysrc/omnibase_infra/types/__init__.pysrc/omnibase_infra/types/type_infra_aliases.pysrc/omnibase_infra/validation/any_type_validator.pysrc/omnibase_infra/validation/contract_linter.pysrc/omnibase_infra/validation/execution_shape_validator.pysrc/omnibase_infra/validation/infra_validators.pysrc/omnibase_infra/validation/routing_coverage_validator.pysrc/omnibase_infra/validation/runtime_shape_validator.pysrc/omnibase_infra/validation/topic_category_validator.py
🧰 Additional context used
📓 Path-based instructions (4)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Never useAnytype - useobjectfor generic payloads in function parameters and return types
UseX | None(PEP 604) instead ofOptional[X]for nullable types
All services must useModelONEXContainerfor dependency injection via__init__(self, container: ModelONEXContainer)
Use@allow_anydecorator with documented reason as exemption mechanism forAnytype violations
UseJsonTypefromomnibase_core.typesas the canonical type alias for JSON-compatible values
UseModelEventEnvelope[object]for generic dispatcher interfaces andobjectfor generic payloads
Infrastructure error handling must useOnexErrorbase class and never expose passwords, API keys, PII, or connection strings with credentials in error messages
UseInfraConnectionError,InfraTimeoutError,InfraAuthenticationError,InfraUnavailableErrorfor transport failures with properModelInfraErrorContext
Correlation IDs must be propagated from incoming requests, auto-generated withuuid4()if missing, and included in all error contexts
External service adapters must implementMixinAsyncCircuitBreakerwith appropriate threshold and reset_timeout configuration
Protocol resolution must use duck typing via protocols, never useisinstancechecks
Files:
src/omnibase_infra/models/validation/model_category_match_result.pysrc/omnibase_infra/validation/any_type_validator.pysrc/omnibase_infra/runtime/models/model_policy_registration.pysrc/omnibase_infra/runtime/message_dispatch_engine.pysrc/omnibase_infra/runtime/policy_registry.pysrc/omnibase_infra/types/__init__.pysrc/omnibase_infra/models/dispatch/model_dispatch_log_context.pysrc/omnibase_infra/types/type_infra_aliases.pysrc/omnibase_infra/validation/routing_coverage_validator.pysrc/omnibase_infra/validation/infra_validators.pysrc/omnibase_infra/validation/execution_shape_validator.pysrc/omnibase_infra/validation/runtime_shape_validator.pysrc/omnibase_infra/runtime/models/model_policy_key.pysrc/omnibase_infra/models/logging/model_log_context.pysrc/omnibase_infra/errors/error_policy_registry.pysrc/omnibase_infra/validation/contract_linter.pysrc/omnibase_infra/validation/topic_category_validator.py
**/model_*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/model_*.py: File naming convention: Models must usemodel_<name>.pywith class nameModel<Name>
Each model file must contain exactly oneModel*class
Pydantic workaround forAnytype must include# NOTE:comment documenting the reason when technically required
UseSerializeAsAnytype wrapper for Pydantic fields containing complex nested models to preserve subclass fields during serialization
Files:
src/omnibase_infra/models/validation/model_category_match_result.pysrc/omnibase_infra/runtime/models/model_policy_registration.pysrc/omnibase_infra/models/dispatch/model_dispatch_log_context.pysrc/omnibase_infra/runtime/models/model_policy_key.pysrc/omnibase_infra/models/logging/model_log_context.py
**/model_*result*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Result models may override
__bool__to enable idiomatic conditional checks, with requiredWarningsection in docstring explaining non-standard behavior
Files:
src/omnibase_infra/models/validation/model_category_match_result.py
**/errors/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Error classes must follow naming pattern
<Domain><Type>Errorand be located inerrors/directory
Files:
src/omnibase_infra/errors/error_policy_registry.py
🧠 Learnings (23)
📓 Common learnings
Learnt from: CR
Repo: OmniNode-ai/omninode_bridge PR: 0
File: .cursor/rules/canonical_patterns.mdc:0-0
Timestamp: 2025-11-28T18:58:53.781Z
Learning: Applies to **/*.py : Use proper union type definitions and discriminated unions where appropriate
📚 Learning: 2025-11-28T18:58:53.781Z
Learnt from: CR
Repo: OmniNode-ai/omninode_bridge PR: 0
File: .cursor/rules/canonical_patterns.mdc:0-0
Timestamp: 2025-11-28T18:58:53.781Z
Learning: Applies to **/*.py : Use `EnumCoreErrorCode` with `ModelOnexError` for proper error code usage
Applied to files:
src/omnibase_infra/runtime/message_dispatch_engine.pysrc/omnibase_infra/models/dispatch/model_dispatch_log_context.py
📚 Learning: 2026-01-15T23:08:36.545Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-15T23:08:36.545Z
Learning: Applies to **/*.py : Use `ModelOnexError` with error codes for ONEX-specific domain errors that need serialization or cross-service communication
Applied to files:
src/omnibase_infra/runtime/message_dispatch_engine.py
📚 Learning: 2025-11-24T16:33:32.747Z
Learnt from: CR
Repo: OmniNode-ai/omninode_bridge PR: 0
File: .cursor/rules/standards.mdc:0-0
Timestamp: 2025-11-24T16:33:32.747Z
Learning: Applies to **/*.py : Import enums from `omnibase.enums` package
Applied to files:
src/omnibase_infra/runtime/message_dispatch_engine.py
📚 Learning: 2025-11-24T17:22:32.195Z
Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: .cursor/rules/canonical_patterns.mdc:0-0
Timestamp: 2025-11-24T17:22:32.195Z
Learning: Applies to **/*.py : All error handling must use OnexError exception class with specific error codes from error enum, never raise generic Exception or ValueError
Applied to files:
src/omnibase_infra/runtime/message_dispatch_engine.py
📚 Learning: 2025-11-24T17:24:41.687Z
Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: .cursor/rules/standards.mdc:0-0
Timestamp: 2025-11-24T17:24:41.687Z
Learning: Applies to **/*.py : Import enums from `omnibase.enums` module
Applied to files:
src/omnibase_infra/runtime/message_dispatch_engine.py
📚 Learning: 2026-01-15T23:08:36.545Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-15T23:08:36.545Z
Learning: Applies to **/*.py : Use `EnumSeverity` from omnibase_core.enums for all general severity classifications (not custom severity enums)
Applied to files:
src/omnibase_infra/runtime/message_dispatch_engine.pysrc/omnibase_infra/validation/contract_linter.py
📚 Learning: 2026-01-16T14:05:23.806Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_spi PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-16T14:05:23.806Z
Learning: Applies to src/omnibase_spi/**/*.py : SPI modules may import from `omnibase_core` at runtime for type hints and model references
Applied to files:
src/omnibase_infra/types/__init__.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_infra/types/__init__.pysrc/omnibase_infra/types/type_infra_aliases.py
📚 Learning: 2026-01-11T17:31:33.550Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_infra PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T17:31:33.550Z
Learning: Applies to **/*.py : Use `JsonType` from `omnibase_core.types` as the canonical type alias for JSON-compatible values
Applied to files:
src/omnibase_infra/types/__init__.pysrc/omnibase_infra/types/type_infra_aliases.py
📚 Learning: 2025-11-28T18:58:53.781Z
Learnt from: CR
Repo: OmniNode-ai/omninode_bridge PR: 0
File: .cursor/rules/canonical_patterns.mdc:0-0
Timestamp: 2025-11-28T18:58:53.781Z
Learning: Applies to **/*.py : Use proper union type definitions and discriminated unions where appropriate
Applied to files:
src/omnibase_infra/types/type_infra_aliases.py
📚 Learning: 2025-11-30T21:55:10.298Z
Learnt from: CR
Repo: OmniNode-ai/omninode_bridge PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-30T21:55:10.298Z
Learning: Deviations from omnibase_core standards are only acceptable for: (1) Orchestrator/Reducer nodes (ModelService* disabled), (2) Experimental features being prototyped for upstream, (3) Performance-critical optimizations with benchmark proof, (4) Bridge-specific unique patterns. All deviations require explicit documentation and justification.
Applied to files:
src/omnibase_infra/validation/infra_validators.py
📚 Learning: 2025-11-24T16:32:55.606Z
Learnt from: CR
Repo: OmniNode-ai/omninode_bridge PR: 0
File: .cursor/rules/node_standards.mdc:0-0
Timestamp: 2025-11-24T16:32:55.606Z
Learning: Applies to **/*contract*.yaml : All ONEX nodes must have validated YAML contracts following the contract-driven development pattern with input_state and output_state schema definitions
Applied to files:
src/omnibase_infra/validation/infra_validators.py
📚 Learning: 2025-11-24T17:23:49.777Z
Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: .cursor/rules/node_standards.mdc:0-0
Timestamp: 2025-11-24T17:23:49.777Z
Learning: Applies to **/node_*/v[0-9]*_[0-9]*_[0-9]*/contracts/contract_*.yaml : All ONEX node subcontracts must be organized in a `contracts/` subdirectory within the versioned implementation directory with separate files for contract_actions.yaml, contract_models.yaml, contract_validation.yaml, contract_cli.yaml (optional), and contract_capabilities.yaml (optional)
Applied to files:
src/omnibase_infra/validation/infra_validators.py
📚 Learning: 2025-12-07T17:50:13.678Z
Learnt from: CR
Repo: OmniNode-ai/omniintelligence PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-07T17:50:13.678Z
Learning: Applies to **/*.py : Use file prefix naming conventions in Python files: model_* for Pydantic models, enum_* for enumerations, protocol_* for protocol interfaces, service_* for service implementations, node_* for ONEX nodes
Applied to files:
src/omnibase_infra/validation/execution_shape_validator.py
📚 Learning: 2025-11-24T17:24:41.687Z
Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: .cursor/rules/standards.mdc:0-0
Timestamp: 2025-11-24T17:24:41.687Z
Learning: Applies to **/models/model_*.py : Model class names must follow the pattern `Model<Name>` (e.g., `ModelNodeGeneratorInputState`)
Applied to files:
src/omnibase_infra/validation/execution_shape_validator.py
📚 Learning: 2026-01-15T23:08:36.545Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-15T23:08:36.545Z
Learning: Applies to src/omnibase_core/**/*.py : File naming in `src/omnibase_core/` must follow directory-specific prefix convention (e.g., `decorator_*.py` in decorators/, `model_*.py` in models/, `enum_*.py` in enums/)
Applied to files:
src/omnibase_infra/validation/execution_shape_validator.py
📚 Learning: 2025-12-06T22:21:32.649Z
Learnt from: CR
Repo: OmniNode-ai/omniagent PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T22:21:32.649Z
Learning: Applies to shared/models/model_*.py : Use `model_*` prefix for Pydantic models in `shared/models/` directory
Applied to files:
src/omnibase_infra/validation/execution_shape_validator.py
📚 Learning: 2025-12-06T22:21:32.649Z
Learnt from: CR
Repo: OmniNode-ai/omniagent PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T22:21:32.649Z
Learning: Applies to nodes/**/node_*.py : Use `node_*` prefix for ONEX node implementation files in `nodes/{type}/` directory
Applied to files:
src/omnibase_infra/validation/execution_shape_validator.py
📚 Learning: 2025-11-24T17:23:49.777Z
Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: .cursor/rules/node_standards.mdc:0-0
Timestamp: 2025-11-24T17:23:49.777Z
Learning: Applies to **/node_*/v[0-9]*_[0-9]*_[0-9]*/models/model_contract_*.py : All ONEX node auto-generated Pydantic models must be organized in a `models/` directory with files for state.py, model_contract_actions.py, model_contract_models.py, model_contract_validation.py, model_contract_cli.py (optional), model_contract_capabilities.py (optional), and error_codes.py, generated from the corresponding contract definitions
Applied to files:
src/omnibase_infra/validation/execution_shape_validator.py
📚 Learning: 2025-11-24T16:33:32.747Z
Learnt from: CR
Repo: OmniNode-ai/omninode_bridge PR: 0
File: .cursor/rules/standards.mdc:0-0
Timestamp: 2025-11-24T16:33:32.747Z
Learning: Applies to **/{models,protocols}/{model_*,protocol_*}.py : Avoid using Any, dict, or primitive types in model and protocol definitions; use strongest typing possible
Applied to files:
src/omnibase_infra/models/logging/model_log_context.py
📚 Learning: 2026-01-11T17:31:33.550Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_infra PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T17:31:33.550Z
Learning: Applies to **/*.py : Use `InfraConnectionError`, `InfraTimeoutError`, `InfraAuthenticationError`, `InfraUnavailableError` for transport failures with proper `ModelInfraErrorContext`
Applied to files:
src/omnibase_infra/errors/error_policy_registry.py
📚 Learning: 2025-12-07T17:50:13.678Z
Learnt from: CR
Repo: OmniNode-ai/omniintelligence PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-07T17:50:13.678Z
Learning: Applies to **/contracts/*.yaml : Validate contracts using the contract linter tool: python -m omniintelligence.tools.contract_linter for YAML contract definitions
Applied to files:
src/omnibase_infra/validation/contract_linter.py
🧬 Code graph analysis (9)
src/omnibase_infra/runtime/models/model_policy_registration.py (2)
src/omnibase_infra/runtime/protocol_policy.py (1)
policy_type(254-286)src/omnibase_infra/runtime/models/model_policy_key.py (1)
validate_policy_type(96-114)
src/omnibase_infra/types/type_infra_aliases.py (3)
src/omnibase_infra/enums/enum_message_category.py (1)
EnumMessageCategory(34-196)src/omnibase_infra/enums/enum_node_output_type.py (1)
EnumNodeOutputType(40-182)src/omnibase_infra/enums/enum_policy_type.py (1)
EnumPolicyType(12-29)
src/omnibase_infra/validation/routing_coverage_validator.py (3)
src/omnibase_infra/nodes/node_registration_orchestrator/handlers/handler_node_heartbeat.py (1)
category(169-171)src/omnibase_infra/protocols/protocol_message_dispatcher.py (1)
category(164-176)tests/unit/runtime/test_dispatcher_registry.py (1)
category(59-60)
src/omnibase_infra/validation/execution_shape_validator.py (2)
src/omnibase_infra/enums/enum_node_output_type.py (4)
is_event(86-88)EnumNodeOutputType(40-182)is_intent(94-96)is_projection(98-100)src/omnibase_infra/enums/enum_message_category.py (3)
is_event(186-188)EnumMessageCategory(34-196)is_intent(194-196)
src/omnibase_infra/validation/runtime_shape_validator.py (4)
src/omnibase_infra/enums/enum_node_archetype.py (1)
EnumNodeArchetype(41-71)src/omnibase_infra/nodes/node_registration_orchestrator/handlers/handler_node_heartbeat.py (1)
category(169-171)src/omnibase_infra/nodes/node_registration_orchestrator/handlers/handler_node_introspected.py (1)
category(214-216)src/omnibase_infra/protocols/protocol_message_dispatcher.py (1)
category(164-176)
src/omnibase_infra/runtime/models/model_policy_key.py (2)
src/omnibase_infra/runtime/protocol_policy.py (1)
policy_type(254-286)src/omnibase_infra/runtime/models/model_policy_registration.py (1)
validate_policy_type(120-138)
src/omnibase_infra/models/logging/model_log_context.py (4)
src/omnibase_infra/models/dispatch/model_dispatch_log_context.py (1)
to_dict(208-267)src/omnibase_infra/models/validation/model_coverage_metrics.py (1)
to_dict(240-273)src/omnibase_infra/models/dispatch/model_dispatcher_metrics.py (1)
to_dict(251-272)src/omnibase_infra/models/registration/model_introspection_metrics.py (1)
to_dict(147-184)
src/omnibase_infra/errors/error_policy_registry.py (4)
tests/unit/runtime/test_policy_registry_performance.py (4)
policy_type(45-47)policy_type(356-357)policy_type(371-372)policy_type(386-387)tests/unit/runtime/test_policy_registry.py (8)
policy_type(48-49)policy_type(66-67)policy_type(84-85)policy_type(102-103)policy_type(123-124)policy_type(144-145)policy_type(162-163)policy_type(180-181)src/omnibase_infra/runtime/protocol_policy.py (1)
policy_type(254-286)tests/integration/runtime/test_policy_registry_container_integration.py (3)
policy_type(133-134)policy_type(202-203)policy_type(216-217)
src/omnibase_infra/validation/topic_category_validator.py (4)
src/omnibase_infra/enums/enum_node_archetype.py (1)
EnumNodeArchetype(41-71)src/omnibase_infra/nodes/node_registration_orchestrator/handlers/handler_node_heartbeat.py (1)
category(169-171)src/omnibase_infra/protocols/protocol_message_dispatcher.py (1)
category(164-176)tests/unit/runtime/test_dispatcher_registry.py (1)
category(59-60)
🔇 Additional comments (24)
src/omnibase_infra/types/type_infra_aliases.py (1)
43-49: Export list matches the new aliases.src/omnibase_infra/types/__init__.py (1)
31-46: Re-exports are consistent and clear.src/omnibase_infra/validation/contract_linter.py (3)
71-75: Good consolidation onPathLikefor public APIs.
929-949: LGTM: PathLike boundary is handled cleanly.
952-1004: Consistent PathLike usage across directory/CI helpers.src/omnibase_infra/validation/any_type_validator.py (1)
58-175: Nice alignment with centralized ASTFunctionDef alias.src/omnibase_infra/validation/routing_coverage_validator.py (2)
65-175: Type-alias propagation looks consistent.
The MessageOutputCategory typing is applied uniformly across discovery helpers and discovery output.Also applies to: 189-192, 293-297, 341-392
714-715: Cache typing aligned with discovery output.
The cached discovered types now match the new MessageOutputCategory alias.src/omnibase_infra/errors/error_policy_registry.py (1)
13-13: PolicyTypeInput adoption is clean.
Signature now matches the centralized alias while keeping EnumPolicyType/string handling intact.Also applies to: 73-78
src/omnibase_infra/models/dispatch/model_dispatch_log_context.py (1)
35-35: LogContextDict typing update is consistent.
The alias now reflects the centralized primitive value type without affecting behavior.Also applies to: 47-47
src/omnibase_infra/models/logging/model_log_context.py (1)
89-89: to_dict typing update is consistent.
Return typing now aligns with the shared primitive value alias used across log contexts.Also applies to: 235-261
src/omnibase_infra/runtime/message_dispatch_engine.py (1)
147-147: Structured log context typing updated consistently.
Return typing now matches the centralized primitive value alias used elsewhere.Also applies to: 767-770
src/omnibase_infra/runtime/models/model_policy_registration.py (2)
15-18: Policy type alias adoption looks consistent.
Nice consolidation of the policy_type input shape on the model field using the shared alias.Also applies to: 71-74
118-138: Validator signature aligns with PolicyTypeInput.
Typing now matches the alias while keeping the validation behavior intact.src/omnibase_infra/runtime/models/model_policy_key.py (2)
13-16: Policy key now matches shared policy_type input shape.
Good alignment with the centralized PolicyTypeInput alias.Also applies to: 46-50
94-114: Validator signature update is clean.
No behavior change, just consistent typing for policy_type validation.src/omnibase_infra/validation/infra_validators.py (2)
51-53: PathLike standardized across validator entry points.
Consistent path typing across the infra validators reads well and keeps call sites uniform.Also applies to: 420-423, 460-461, 477-479, 684-686, 1034-1038, 1132-1134, 1152-1155
388-402: Confirm the new union threshold baseline (70) still passes.
Please re-run the union usage validation to ensure the reduced cap is aligned with the current non-optional union count.src/omnibase_infra/runtime/policy_registry.py (1)
93-98: PolicyTypeInput adoption across registry APIs is consistent.
The normalization path and public method signatures now share the same policy_type input shape, which should reduce call-site variance.Also applies to: 595-598, 769-775, 851-855, 1298-1301, 1374-1378, 1423-1427
src/omnibase_infra/validation/runtime_shape_validator.py (2)
174-178: MessageOutputCategory integration is cohesive in helpers and mappings.
The mapping and category-detection utilities now consistently operate on the unified category type.Also applies to: 206-208, 311-313, 366-369, 425-430
551-555: Runtime validator APIs now consistently use MessageOutputCategory.
Public validation methods align with the consolidated category type, keeping call sites uniform.Also applies to: 604-609, 706-711
src/omnibase_infra/validation/execution_shape_validator.py (2)
99-100: ASTFunctionDef alias applied consistently across handler discovery and validation.
Keeps node typing consistent end‑to‑end and removes duplicated unions.Also applies to: 275-279, 462-473, 512-513, 645-652, 673-676, 1009-1013, 1058-1062
717-758: MessageOutputCategory integration in return‑type detection is clean.
Normalization and PROJECTION handling remain explicit and easy to follow.Also applies to: 810-908, 910-947
src/omnibase_infra/validation/topic_category_validator.py (1)
49-50: MessageOutputCategory usage is consistently applied across constants and APIs.
Unifies routing and output-type expectations while keeping PROJECTION handling centralized.Also applies to: 86-91, 102-111, 170-174, 244-249, 362-366, 1009-1013
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
- Move ModelSemVer import to runtime (was in TYPE_CHECKING block) - Rename PathLike to PathInput to avoid stdlib os.PathLike collision - Update INFRA_MAX_UNIONS test to expect 70 (reduced from 122)
Code Review: Type Alias Refactoring (OMN-1358)SummaryThis PR successfully reduces union complexity from 122 to 70 non-optional unions (48% reduction) by introducing centralized type aliases. The implementation follows ONEX patterns and the approach is sound. ✅ Strengths1. Excellent Type Alias Design
2. Strong ONEX Compliance
3. Thorough Testing
4. Clean Import Hygiene
🔍 Issues & Recommendations1. CRITICAL: Incomplete Type Alias AdoptionThe PR description claims reduction from 122 to 70, but there may be additional opportunities for using the new aliases: # VERIFY: Are there remaining instances of these patterns?
# - Path | str (should use PathInput)
# - EnumPolicyType | str (should use PolicyTypeInput)
# - EnumMessageCategory | EnumNodeOutputType (should use MessageOutputCategory)
# - ast.AsyncFunctionDef | ast.FunctionDef (should use ASTFunctionDef)Action Required: Run a grep audit to ensure all applicable locations now use the aliases: # Check for remaining raw patterns
rg "Path \| str" src/omnibase_infra/
rg "EnumPolicyType \| str" src/omnibase_infra/
rg "AsyncFunctionDef \| ast\.FunctionDef" src/omnibase_infra/2. MINOR: Type Alias Documentation GapFile: The module docstring mentions occurrence counts but these will become stale: # Replaces 24 occurrences of: EnumMessageCategory | EnumNodeOutputType
type MessageOutputCategory = EnumMessageCategory | EnumNodeOutputTypeRecommendation: Remove numeric counts from comments (or make them historical): # Message category or node output type (for routing and validation)
# Historically replaced 24 direct union occurrences (OMN-1358)
type MessageOutputCategory = EnumMessageCategory | EnumNodeOutputType3. MINOR: Inconsistent Usage of
|
| Priority | Issue | Action |
|---|---|---|
| HIGH | Verify complete adoption | Grep audit for remaining raw union patterns |
| MEDIUM | VersionInput unused |
Apply in this PR or remove |
| MEDIUM | Backward compat contradiction | Remove CategoryType or clarify internal use |
| LOW | Stale occurrence counts | Make comments historical or remove counts |
| LOW | LogContextDict placement |
Document why local vs central |
| LOW | Threshold buffer | Document 7-unit buffer rationale |
✅ Approval Recommendation
APPROVE with minor revisions
This PR delivers significant value (48% union reduction) with clean implementation. The issues identified are non-blocking but should be addressed before merge:
- Must Fix: Verify complete type alias adoption (grep audit)
- Should Fix: Resolve
VersionInputunused type andCategoryTypebackward compat note - Nice to Have: Address documentation and testing gaps
Great work on this refactoring! 🎉
📋 Checklist for Merge
- Grep audit confirms no remaining raw
Path | str,EnumPolicyType | str, etc. -
VersionInputeither used or removed -
CategoryTypebackward compat note clarified per CLAUDE.md - CI passes (ONEX validators, any_type check, union threshold)
- Consider adding import test for new
typesmodule exports
Add comprehensive test coverage for the new types module to verify all exports are importable at runtime. Addresses PR review feedback.
PR Review: Union Complexity Reduction with Type Aliases [OMN-1358]SummaryThis PR successfully reduces non-optional union complexity from 122 to 63 instances (48% reduction) by introducing centralized type aliases. The implementation is clean, well-tested, and follows ONEX architectural principles. ✅ Strengths1. Excellent Design Pattern
2. Comprehensive Test CoverageThe new test file
3. Strong Type Safety
4. Validation Threshold Update
5. Documentation Quality
📋 Code Quality ObservationsImport Pattern ConsistencyFile: The centralized export pattern is excellent: from omnibase_infra.types.type_infra_aliases import (
ASTFunctionDef,
MessageOutputCategory,
PathInput,
PolicyTypeInput,
VersionInput,
)
__all__: list[str] = [
"ASTFunctionDef",
"MessageOutputCategory",
# ... etc
]This provides both direct imports ( Type Alias Usage PatternsMessageOutputCategory (44 occurrences across 7 files):
PolicyTypeInput (20 occurrences across 6 files):
PrimitiveValue Usage (from
Backward Compatibility NoteFile: # Type alias for the category union - improves readability
# Note: CategoryType is kept for backward compatibility; prefer MessageOutputCategory for new code
type CategoryType = MessageOutputCategory
Question: Is 🔍 Potential Issues1.
|
| Metric | Before | After | Change |
|---|---|---|---|
| Non-optional unions | 122 | 63 | -48% ✅ |
| Union threshold | 122 | 70 | Conservative buffer |
| Type alias definitions | 0 | 5 | New abstractions |
| Test coverage | N/A | 100% | Comprehensive |
✅ ONEX Compliance Checklist
| Requirement | Status | Notes |
|---|---|---|
No Any types |
✅ PASS | Zero Any usage |
PEP 604 unions (X | Y) |
✅ PASS | All unions use PEP 604 syntax |
| Strong typing | ✅ PASS | Type aliases maintain strict typing |
| One model per file | ✅ PASS | N/A - no new models |
| File naming conventions | ✅ PASS | type_infra_aliases.py follows pattern |
| Test coverage | ✅ PASS | Comprehensive test suite added |
| Documentation | ✅ PASS | Clear docstrings and comments |
| CI validation update | ✅ PASS | INFRA_MAX_UNIONS threshold updated |
🎯 Recommendations
Critical (Before Merge)
- ✅ VERIFIED: Ensure no remaining
EnumMessageCategory | EnumNodeOutputTypepatterns outside type definitions ⚠️ DECISION NEEDED: RemoveCategoryTypebackward compatibility alias per ONEX policy (unless actively used)
Nice-to-Have (Future PRs)
- Consider standardizing
Inputsuffix across all type aliases (MessageCategoryInput?) - Add runtime validation tests for Pydantic models using these aliases
- Document type alias usage patterns in CLAUDE.md or ADR
🏁 Verdict
APPROVED ✅
This is a well-executed refactoring that:
- Reduces complexity measurably (48% reduction)
- Follows ONEX architectural patterns
- Includes excellent test coverage
- Updates validation thresholds correctly
- Maintains type safety throughout
Minor Action Items
- Verify
CategoryTypebackward compatibility alias is needed (or remove per ONEX policy) - Consider naming convention standardization in future work
Excellent work on this refactoring! The type alias pattern is a clean solution to union complexity, and the comprehensive testing gives confidence in the changes.
Review completed by: Claude Code
CLAUDE.md compliance: ✅ Verified
Test coverage: ✅ Comprehensive
Security: ✅ No concerns
Performance: ✅ No degradation
…N-1358] Address PR #157 review feedback: - Remove unused VersionInput type alias and ModelSemVer import - Enhance PathInput documentation to clarify os.PathLike distinction - Update tests to reflect 7 exports (was 8)
PR Review: Union Complexity Reduction with Type AliasesSummaryThis PR successfully reduces non-optional union complexity from 122 to 70 (48% reduction) by introducing centralized type aliases. The implementation is clean, well-tested, and aligns with ONEX coding standards. ✅ Strengths1. Excellent Type Alias Design
2. Strong Test CoverageThe test suite (
3. Threshold Adjustment Well-Documented
4. Backward Compatibility ConsiderationLine 54-55 in # Note: CategoryType is kept for backward compatibility; prefer MessageOutputCategory for new code
type CategoryType = MessageOutputCategoryDespite the "no backwards compatibility" policy in CLAUDE.md, this transitional alias is acceptable as it's within the same PR and helps gradual migration. 🔍 Code Quality ObservationsConsistent ApplicationThe type aliases are applied consistently across:
Proper Import PatternsAll imports follow ONEX conventions: from omnibase_infra.types import MessageOutputCategory, PathInput, PolicyTypeInput, ASTFunctionDefPrimitiveValue ReuseSmart reuse of 🐛 Potential Issues1. VersionInput Removal - Verification NeededCommit Question: Was this based on static analysis or manual review? If static analysis, ensure no dynamic usage exists (e.g., Recommendation: Run grep to confirm zero usage: rg "VersionInput" src/2. isinstance() Usage with Type AliasesLine 462 in elif isinstance(node, (ast.AsyncFunctionDef, ast.FunctionDef)):While the type alias # Note: isinstance requires concrete types, not type alias
elif isinstance(node, (ast.AsyncFunctionDef, ast.FunctionDef)):3. Union Threshold Buffer AnalysisCurrent state: 63 actual non-optional unions, threshold set to 70 (buffer of 7). Question: Is a 10% buffer sufficient? Previous threshold (122) had a ~20 buffer above baseline (~100). Recommendation: Document the rationale for the tighter buffer in 🔒 Security ConsiderationsNo security concerns identified. The changes are purely type system refactoring with no:
📊 Performance ConsiderationsPositive impact:
No performance regressions expected. 🧪 Test Coverage AssessmentWhat's Covered ✅
Potential Gaps 🤔
📝 Documentation QualityExcellent Documentation ✅
Minor Improvement OpportunityConsider adding a docstring to """
When to use these aliases:
- MessageOutputCategory: When accepting either message categories or node output types
- PathInput: When accepting filesystem paths flexibly (Path or str)
- PolicyTypeInput: When accepting policy types with string fallback
- ASTFunctionDef: When processing both sync and async function definitions
"""🎯 CLAUDE.md Compliance✅ Compliant
|
…N-1358] - Remove CategoryType backward compat alias (violates no-compat policy) - Replace with MessageOutputCategory in model_category_match_result.py - Add isinstance() clarity comment explaining single-type check - Enhance type_infra_aliases.py docstring with "when to use" guidance - Add TestTypeAliasMyPyCompatibility class with 8 mypy verification tests
Pull Request Review: Type Alias Union Complexity ReductionSummaryThis PR successfully reduces union complexity from 122 to 63 non-optional unions (48% reduction) by introducing well-designed type aliases. The changes align with ONEX infrastructure patterns and demonstrate excellent engineering discipline. ✅ Strengths1. Excellent Type Alias Design
2. Outstanding Test Coverage (366 lines of new tests!)The test suite in
This level of testing sets a strong precedent for type infrastructure changes. 3. Significant Complexity Reduction
4. Appropriate Threshold AdjustmentThe
5. Consistent ApplicationChanges are applied systematically across:
🔍 Observations & Minor Suggestions1. Documentation Comment Discrepancy (line 389)# - 70 (2026-01-16): OMN-1358 type alias replacements reduced from 122 to 63 non-optional unions
# Applied HandlerMap, NodeId, PayloadDict, EventMetadata, MetadataDict type aliasesThe comment mentions
Recommendation: Update the comment to reflect the actual type aliases introduced, or clarify if those other aliases were applied in a separate commit. 2. PrimitiveValue UsageThe PR also applies Suggestion: Consider documenting this in the PR description alongside the new type aliases, as it contributes to the union reduction. 3. isinstance() Pattern in model_category_match_result.pyLine 196 has a good comment explaining why a single # Single type check is sufficient - EnumMessageCategory is the only message
# category enum. EnumNodeOutputType contains PROJECTION which is NOT a
# routable message category (it's for reducer state aggregation outputs).
return isinstance(self.category, EnumMessageCategory)This demonstrates excellent defensive coding with clear rationale. 🛡️ Security & Best Practices✅ No security concerns - Type aliases are compile-time constructs with no runtime security implications ✅ Follows ONEX patterns:
✅ Backward compatibility: Not a concern per ONEX policy ("No Backwards Compatibility" - all changes are breaking) 📊 Code Quality Metrics
🎯 Final VerdictAPPROVE with minor documentation update suggestion This PR represents excellent engineering work:
The only minor issue is the documentation comment discrepancy in Recommendation✅ Safe to merge after addressing the documentation comment discrepancy (or clarifying if those aliases were applied elsewhere) Review performed by Claude Code following ONEX infrastructure patterns and CLAUDE.md guidelines |
Merge PR #157 (OMN-1358 type alias changes) into PR #158 (OMN-1181 structured errors). Changes: - Resolve merge conflicts in model_policy_key.py, model_policy_registration.py, infra_validators.py - Update INFRA_MAX_UNIONS from 122 to 66 (actual count: 64 after type alias cleanup) - Fix defensive pattern in mixin_vault_token.py: RuntimeHostError → InfraVaultError - Update test expectation in test_validator_defaults.py The type alias changes from OMN-1358 reduced union count from 122 to ~64. OMN-1181 adds 2 unions for EnumPolicyType | str validators, setting threshold to 66.
Reduce non-optional unions from 122 to 63 (48% reduction) by introducing centralized type aliases that consolidate repeated union patterns.
New type aliases in types/type_infra_aliases.py:
Also applied PrimitiveValue from omnibase_core for float | int | str patterns.
Updated INFRA_MAX_UNIONS threshold from 122 to 70 to reflect the new baseline with appropriate buffer for minor growth.
Summary by CodeRabbit
Refactor
Bug Fix
Tests
✏️ Tip: You can customize this high-level summary in your review settings.