Repository navigation
refactor(naming): rename 9 non-conforming Model/Protocol classes (OMN-1071) - #261
Conversation
…-1071) Rename classes with misleading prefixes to follow ONEX naming conventions: - Model prefix → only for Pydantic BaseModel classes - Protocol prefix → only for typing.Protocol interfaces - Service prefix → for stateful service classes - Util prefix → for utility classes ## Util classes (moved to utils/): - ModelCliResultFormatter → UtilCliResultFormatter - ModelConflictResolver → UtilConflictResolver - ModelStreamingWindow → UtilStreamingWindow - ModelSecurityUtils → UtilSecurity - ProtocolContractLoader → UtilContractLoader ## Service classes (moved to services/): - ModelProtocolAuditor → ServiceProtocolAuditor - ModelValidationSuite → ServiceValidationSuite - ProtocolContractValidator → ServiceContractValidator - ProtocolMigrator → ServiceProtocolMigrator Backwards compatibility aliases are maintained for existing imports.
📝 WalkthroughWalkthroughRenames and relocations across omnibase_core: Protocol*/Model* classes moved to services/* or utils/* and renamed to Service*/Util*; lazy-loading and deprecated alias paths added to preserve backwards compatibility; docs, scripts, and tests updated to the new module/class names and import locations. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
Code Review - PR #261: Rename Non-Conforming Model/Protocol ClassesSummaryThis PR implements a systematic refactoring to align class naming with ONEX conventions by renaming 9 classes with misleading ✅ Strengths1. Excellent Adherence to ONEX Naming ConventionsThe renaming follows the documented conventions precisely:
This eliminates confusion and makes the codebase more intuitive. 2. Strategic Backwards CompatibilityThe PR implements backwards compatibility at the right layers: ✅ Good examples:
This allows existing code to continue working while new code uses correct names. 3. Comprehensive Documentation UpdatesAll documentation files have been systematically updated:
This prevents documentation drift and ensures future developers see the correct patterns. 4. Test CoverageThe PR correctly updates test files:
|
…N-1071) Address PR #261 review feedback: CRITICAL fixes: - Add ModelCliResultFormatter alias in models/cli/__init__.py - Add ModelSecurityUtils alias in models/security/__init__.py - Add ProtocolContractLoader alias in utils/__init__.py - Add missing ProtocolMigrator alias in validation/__init__.py Documentation: - Document lazy __getattr__ vs direct alias strategy in validation/__init__.py - Add import policy docstring to services/__init__.py - Add thread-safety docs to UtilSecurity and UtilStreamingWindow Testing: - Add backwards compatibility tests (14 tests) verifying all old imports work
Code Review: PR #261 - Rename Non-Conforming Model/Protocol Classes (OMN-1071)SummaryThis is an excellent refactoring that systematically addresses naming convention violations across the codebase. The PR successfully renames 9 classes with misleading prefixes while maintaining full backwards compatibility. The implementation demonstrates exceptional attention to detail and adherence to ONEX standards. ✅ Strengths1. Comprehensive Backwards Compatibility StrategyThe dual approach to backwards compatibility is well-executed:
2. Exceptional Test Coverage
3. Correct Architectural OrganizationThe file relocations follow ONEX principles perfectly:
4. Circular Import PreventionThe
5. Documentation Quality
6. Type Safety Maintained
🔍 Observations & Recommendations1. Import Strategy Inconsistency (Minor)Location: The comment reads: # Import ServiceValidationSuite DIRECTLY from the module file (not from services/__init__.py)
# to avoid circular imports.Observation: This is correct given the new Recommendation: Update comment to clarify: # Import ServiceValidationSuite directly from module file (services/__init__.py has no exports per OMN-1071 policy)2. Deprecation Timeline (Question)Locations: Multiple Question: The PR description and test file mention "until v0.5.0" but CLAUDE.md states "No Backwards Compatibility: Until v0.4.0 is released, breaking changes may occur between commits." Recommendation:
3. Thread Safety Documentation (Excellent!)Location: Thread safety warnings are correctly preserved: """
Thread Safety:
UtilConflictResolver is NOT thread-safe. Each thread should use its
own instance. The conflicts_count attribute is mutated during resolution.
"""This aligns perfectly with CLAUDE.md's threading guidance. 🎯 Code Quality Assessment
🔒 Security & PerformanceSecurity✅ No security concerns identified
Performance✅ No performance impact
📋 Checklist Verification
🎉 RecommendationAPPROVE - This PR is production-ready and demonstrates exceptional software engineering practices. Why This PR Excels:
Suggested Next Steps:
Great work on this refactoring! 🚀 The attention to backwards compatibility and documentation sets a high standard for future PRs. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/omnibase_core/services/service_protocol_migrator.py (1)
9-31: Fix import sorting to resolve pipeline failure.The import block is flagged as unsorted by ruff (I001). This causes the pipeline to fail and must be fixed before merge.
Run the following to fix:
poetry run ruff check --select I --fix src/omnibase_core/services/service_protocol_migrator.pyAs per coding guidelines: "Use isort for import sorting to maintain consistent import order" and "Use ruff for linting and black for formatting - run pre-commit hooks before commit".
src/omnibase_core/validation/cli.py (1)
1-4: Remove unused import causing pipeline failure.The
ModelOnexErrorimport is unused in this file and flagged by the linter (F401). The import statement on line 3 should be removed.🔎 Proposed fix
from __future__ import annotations -from omnibase_core.models.errors.model_onex_error import ModelOnexError - """ Unified CLI interface for all omnibase_core validation tools.src/omnibase_core/services/service_protocol_auditor.py (1)
10-30: Fix import sorting to resolve pipeline failure.The pipeline reports
I001 Import block is un-sorted or un-formatted. Per coding guidelines, useisortfor import sorting.🔎 Proposed fix
import logging from collections import defaultdict from pathlib import Path from typing import TYPE_CHECKING from omnibase_core.errors.exceptions import ( ExceptionConfigurationError, ExceptionInputValidationError, ) from omnibase_core.models.validation.model_audit_result import ModelAuditResult from omnibase_core.models.validation.model_duplication_report import ( ModelDuplicationReport, ) - from omnibase_core.validation.validation_utils import ( ModelDuplicationInfo, ModelProtocolInfo, determine_repository_name, extract_protocols_from_directory, validate_directory_path, )Run
poetry run isort src/omnibase_core/services/service_protocol_auditor.pyto auto-fix.tests/unit/validation/test_auditor_protocol_extended.py (1)
14-27: Fix import sorting to resolve pipeline failure.The pipeline reports
I001 Import block is un-sorted or un-formatted. Runpoetry run isort tests/unit/validation/test_auditor_protocol_extended.pyto auto-fix.
🧹 Nitpick comments (7)
src/omnibase_core/models/security/__init__.py (1)
5-6: LGTM - Proper backward compatibility implementation.The alias is correctly placed in the original import location, ensuring existing code continues to work. The deprecation is clearly marked in comments and the
__all__list.Optional: Consider adding deprecation timeline
You might consider adding a version or date when the alias will be removed to help users plan their migration:
# DEPRECATED: Use UtilSecurity from omnibase_core.utils (will be removed in v2.0.0)Also applies to: 40-42, 74-75
docs/architecture/IMPORT_COMPATIBILITY_MATRIX.md (1)
481-486: Validation module exports updated correctly.The documentation now reflects
ServiceValidationSuite,ServiceProtocolAuditor, andServiceContractValidatoras the canonical exports.Consider updating the "Last Updated" date on line 5 from
2025-12-09to reflect this change.tests/unit/validation/test_migrator_protocol.py (1)
1-908: Migrator tests consistently updated to ServiceProtocolMigrator (only naming changes)All usages of the migrator in this suite—imports, test class names, and instantiations—now target
ServiceProtocolMigratorwhile exercising the same public and private methods (create_migration_plan,execute_migration,_detect_migration_conflicts,_generate_migration_steps,_update_spi_imports,_find_import_references,rollback_migration). This preserves the behavioral coverage after the rename.As a small cosmetic follow-up, you may want to update the module docstring’s first line (“Comprehensive unit tests for Protocol Migrator.”) to mention
ServiceProtocolMigratorfor consistency with the new naming.src/omnibase_core/utils/__init__.py (1)
7-41: Lazy-loading UtilContractLoader with a deprecated ProtocolContractLoader alias looks correctThe utils package now:
- Exposes
UtilConflictResolvereagerly while keeping heavy utilities (includingUtilContractLoader) out of import-time execution.- Adds
UtilContractLoaderand a deprecatedProtocolContractLoaderalias to__all__.- Implements
__getattr__so that bothUtilContractLoaderandProtocolContractLoaderare resolved lazily from.util_contract_loader, preserving backward compatibility without introducing new circular dependencies.This design lets callers use either the new or old name via
omnibase_core.utilswithout paying the cost of loading contract models until needed.If you later introduce additional lazily-loaded utilities that aren’t classes, consider loosening the
__getattr__return annotation fromtypetoobjectto avoid misleading type hints, but that’s purely optional given the current usage.Also applies to: 44-64
src/omnibase_core/services/service_validation_suite.py (2)
73-96: Consider using more specific exception handling.The bare
except Exceptioncatch at line 85 could mask unexpected errors. While the current approach ensures all validators run and results are collected, you might want to log the exception type for debugging or consider catching more specific exceptions.🔎 Optional improvement
for validation_type in self.validators: try: result = self.run_validation(validation_type, directory, **kwargs) results[validation_type] = result - except Exception as e: + except ModelOnexError as e: # Create error result results[validation_type] = ModelValidationResult( is_valid=False, - errors=[f"Validation failed: {e}"], + errors=[f"Validation failed: {e.message}"], metadata=ModelValidationMetadata( validation_type=validation_type, files_processed=0, ), )
98-109: Consider returning validator info instead of printing.Using
print()directly in library code limits flexibility. Consider returning a string or structured data that callers can format as needed.🔎 Optional improvement for future consideration
def list_validators(self) -> dict[str, str]: """Return available validators and their descriptions.""" return {name: info["description"] for name, info in self.validators.items()}The current implementation is acceptable for CLI usage but may want refactoring if this class is used programmatically.
src/omnibase_core/validation/__init__.py (1)
108-177: Consider consolidating duplicate imports in__getattr__.The function has 6 if-blocks where the 3 alias cases duplicate the same imports as their canonical versions. This could be simplified with a mapping dictionary to reduce duplication and make future alias additions easier.
🔎 Proposed refactor using a mapping dictionary
def __getattr__(name: str) -> type: """ Lazy import for service classes to avoid circular imports. ... """ + # Mapping of name -> (module_path, class_name) + _LAZY_IMPORTS = { + "ServiceProtocolAuditor": ("omnibase_core.services.service_protocol_auditor", "ServiceProtocolAuditor"), + "ServiceContractValidator": ("omnibase_core.services.service_contract_validator", "ServiceContractValidator"), + "ServiceProtocolMigrator": ("omnibase_core.services.service_protocol_migrator", "ServiceProtocolMigrator"), + # Backwards compatibility aliases (OMN-1071) + "ModelProtocolAuditor": ("omnibase_core.services.service_protocol_auditor", "ServiceProtocolAuditor"), + "ProtocolContractValidator": ("omnibase_core.services.service_contract_validator", "ServiceContractValidator"), + "ProtocolMigrator": ("omnibase_core.services.service_protocol_migrator", "ServiceProtocolMigrator"), + } + + if name in _LAZY_IMPORTS: + module_path, class_name = _LAZY_IMPORTS[name] + import importlib + module = importlib.import_module(module_path) + return getattr(module, class_name) + msg = f"module {__name__!r} has no attribute {name!r}" raise AttributeError(msg)
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (47)
.yaml-validation-allowlist.yamldocs/MVP_PLAN.mddocs/architecture/CONTAINER_DI_DESIGN.mddocs/architecture/CONTRACT_STABILITY_SPEC.mddocs/architecture/IMPORT_COMPATIBILITY_MATRIX.mddocs/architecture/SPI_PROTOCOL_ADOPTION_ROADMAP.mddocs/architecture/VALIDATION_PROTOCOL_COMPLIANCE.mddocs/patterns/PURE_FSM_REDUCER_PATTERN.mdexamples/contract_validator_usage.pyscripts/validate-no-spi-imports.shsrc/omnibase_core/__init__.pysrc/omnibase_core/__init___fast.pysrc/omnibase_core/infrastructure/node_base.pysrc/omnibase_core/models/cli/__init__.pysrc/omnibase_core/models/cli/model_cli_result.pysrc/omnibase_core/models/reducer/__init__.pysrc/omnibase_core/models/security/__init__.pysrc/omnibase_core/models/services/model_external_service_config.pysrc/omnibase_core/nodes/__init__.pysrc/omnibase_core/services/__init__.pysrc/omnibase_core/services/service_contract_validator.pysrc/omnibase_core/services/service_protocol_auditor.pysrc/omnibase_core/services/service_protocol_migrator.pysrc/omnibase_core/services/service_validation_suite.pysrc/omnibase_core/utils/__init__.pysrc/omnibase_core/utils/util_cli_result_formatter.pysrc/omnibase_core/utils/util_conflict_resolver.pysrc/omnibase_core/utils/util_contract_loader.pysrc/omnibase_core/utils/util_security.pysrc/omnibase_core/utils/util_streaming_window.pysrc/omnibase_core/validation/__init__.pysrc/omnibase_core/validation/cli.pytests/unit/api_snapshots/test_nodes_api_snapshot.pytests/unit/cli/test_commands.pytests/unit/infrastructure/test_node_base.pytests/unit/test_backwards_compatibility_omn1071.pytests/unit/test_init_exports.pytests/unit/test_init_fast.pytests/unit/utils/test_util_cli_result_formatter.pytests/unit/utils/test_util_contract_loader_part1.pytests/unit/utils/test_util_contract_loader_part2.pytests/unit/validation/test_architecture.pytests/unit/validation/test_auditor_protocol_extended.pytests/unit/validation/test_cli.pytests/unit/validation/test_contract_validator.pytests/unit/validation/test_migrator_protocol.pytests/unit/validation/test_protocol_auditor.py
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{py,sh}
📄 CodeRabbit inference engine (CLAUDE.md)
Always use
poetry runprefix for Python commands (pytest, mypy, python -m, etc.) - never use pip or direct python execution
Files:
src/omnibase_core/utils/util_streaming_window.pysrc/omnibase_core/services/service_contract_validator.pysrc/omnibase_core/utils/util_conflict_resolver.pytests/unit/cli/test_commands.pyscripts/validate-no-spi-imports.shtests/unit/infrastructure/test_node_base.pysrc/omnibase_core/__init__.pysrc/omnibase_core/models/reducer/__init__.pytests/unit/utils/test_util_cli_result_formatter.pysrc/omnibase_core/utils/util_contract_loader.pysrc/omnibase_core/models/cli/model_cli_result.pysrc/omnibase_core/utils/util_security.pysrc/omnibase_core/utils/__init__.pytests/unit/validation/test_architecture.pytests/unit/validation/test_contract_validator.pytests/unit/test_backwards_compatibility_omn1071.pysrc/omnibase_core/services/service_protocol_migrator.pysrc/omnibase_core/__init___fast.pytests/unit/test_init_exports.pyexamples/contract_validator_usage.pysrc/omnibase_core/infrastructure/node_base.pytests/unit/api_snapshots/test_nodes_api_snapshot.pytests/unit/validation/test_migrator_protocol.pysrc/omnibase_core/models/cli/__init__.pytests/unit/test_init_fast.pysrc/omnibase_core/models/services/model_external_service_config.pytests/unit/validation/test_auditor_protocol_extended.pysrc/omnibase_core/services/service_validation_suite.pytests/unit/utils/test_util_contract_loader_part2.pysrc/omnibase_core/nodes/__init__.pysrc/omnibase_core/validation/__init__.pytests/unit/validation/test_protocol_auditor.pysrc/omnibase_core/utils/util_cli_result_formatter.pysrc/omnibase_core/services/service_protocol_auditor.pysrc/omnibase_core/services/__init__.pytests/unit/utils/test_util_contract_loader_part1.pysrc/omnibase_core/models/security/__init__.pytests/unit/validation/test_cli.pysrc/omnibase_core/validation/cli.py
src/omnibase_core/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/omnibase_core/**/*.py: Use Protocol-driven dependency injection withcontainer.get_service('ProtocolName')- never use concrete class names or isinstance checks
Use ModelOnexError with EnumCoreErrorCode for all error handling - never use generic Exception
Use @standard_error_handling decorator to eliminate try/catch boilerplate in async operations
Use PEP 604 union syntax (str | None) instead of Optional/Union - enforce via ruff UP007
Use FileRegistry for loading YAML contracts:FileRegistry().load(Path('config/runtime_host.yaml'))
Use duck typing with protocols instead of isinstance checks for type validation
Maintain minimum 60% code coverage - enforce via pytest --cov in CI
Both mypy AND pyright must pass in CI with strict type checking (0 errors required for mypy)
Use EnumNodeKind for architectural routing (EFFECT, COMPUTE, REDUCER, ORCHESTRATOR) and EnumNodeType for implementation discovery (COMPUTE_GENERIC, TRANSFORMER, AGGREGATOR, etc.)
Use Protocol abstractions for all service interfaces (ProtocolLogger, ProtocolEventBus, etc.) - never use concrete implementations
Files:
src/omnibase_core/utils/util_streaming_window.pysrc/omnibase_core/services/service_contract_validator.pysrc/omnibase_core/utils/util_conflict_resolver.pysrc/omnibase_core/__init__.pysrc/omnibase_core/models/reducer/__init__.pysrc/omnibase_core/utils/util_contract_loader.pysrc/omnibase_core/models/cli/model_cli_result.pysrc/omnibase_core/utils/util_security.pysrc/omnibase_core/utils/__init__.pysrc/omnibase_core/services/service_protocol_migrator.pysrc/omnibase_core/__init___fast.pysrc/omnibase_core/infrastructure/node_base.pysrc/omnibase_core/models/cli/__init__.pysrc/omnibase_core/models/services/model_external_service_config.pysrc/omnibase_core/services/service_validation_suite.pysrc/omnibase_core/nodes/__init__.pysrc/omnibase_core/validation/__init__.pysrc/omnibase_core/utils/util_cli_result_formatter.pysrc/omnibase_core/services/service_protocol_auditor.pysrc/omnibase_core/services/__init__.pysrc/omnibase_core/models/security/__init__.pysrc/omnibase_core/validation/cli.py
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Use ruff for linting and black for formatting - run pre-commit hooks before commit
Use isort for import sorting to maintain consistent import order
Files:
src/omnibase_core/utils/util_streaming_window.pysrc/omnibase_core/services/service_contract_validator.pysrc/omnibase_core/utils/util_conflict_resolver.pytests/unit/cli/test_commands.pytests/unit/infrastructure/test_node_base.pysrc/omnibase_core/__init__.pysrc/omnibase_core/models/reducer/__init__.pytests/unit/utils/test_util_cli_result_formatter.pysrc/omnibase_core/utils/util_contract_loader.pysrc/omnibase_core/models/cli/model_cli_result.pysrc/omnibase_core/utils/util_security.pysrc/omnibase_core/utils/__init__.pytests/unit/validation/test_architecture.pytests/unit/validation/test_contract_validator.pytests/unit/test_backwards_compatibility_omn1071.pysrc/omnibase_core/services/service_protocol_migrator.pysrc/omnibase_core/__init___fast.pytests/unit/test_init_exports.pyexamples/contract_validator_usage.pysrc/omnibase_core/infrastructure/node_base.pytests/unit/api_snapshots/test_nodes_api_snapshot.pytests/unit/validation/test_migrator_protocol.pysrc/omnibase_core/models/cli/__init__.pytests/unit/test_init_fast.pysrc/omnibase_core/models/services/model_external_service_config.pytests/unit/validation/test_auditor_protocol_extended.pysrc/omnibase_core/services/service_validation_suite.pytests/unit/utils/test_util_contract_loader_part2.pysrc/omnibase_core/nodes/__init__.pysrc/omnibase_core/validation/__init__.pytests/unit/validation/test_protocol_auditor.pysrc/omnibase_core/utils/util_cli_result_formatter.pysrc/omnibase_core/services/service_protocol_auditor.pysrc/omnibase_core/services/__init__.pytests/unit/utils/test_util_contract_loader_part1.pysrc/omnibase_core/models/security/__init__.pytests/unit/validation/test_cli.pysrc/omnibase_core/validation/cli.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/cli/test_commands.pytests/unit/infrastructure/test_node_base.pytests/unit/utils/test_util_cli_result_formatter.pytests/unit/validation/test_architecture.pytests/unit/validation/test_contract_validator.pytests/unit/test_backwards_compatibility_omn1071.pytests/unit/test_init_exports.pytests/unit/api_snapshots/test_nodes_api_snapshot.pytests/unit/validation/test_migrator_protocol.pytests/unit/test_init_fast.pytests/unit/validation/test_auditor_protocol_extended.pytests/unit/utils/test_util_contract_loader_part2.pytests/unit/validation/test_protocol_auditor.pytests/unit/utils/test_util_contract_loader_part1.pytests/unit/validation/test_cli.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/reducer/__init__.pysrc/omnibase_core/models/cli/model_cli_result.pysrc/omnibase_core/models/cli/__init__.pysrc/omnibase_core/models/services/model_external_service_config.pysrc/omnibase_core/models/security/__init__.py
src/omnibase_core/infrastructure/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/omnibase_core/infrastructure/**/*.py: Always callsuper().__init__(container)in node constructors - this is mandatory and eliminates 80+ lines of boilerplate
Use mixins (MixinDiscoveryResponder, MixinEventHandler, etc.) to add reusable capabilities to nodes
Files:
src/omnibase_core/infrastructure/node_base.py
src/omnibase_core/nodes/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/omnibase_core/nodes/**/*.py: Do NOT share node instances across threads without explicit synchronization - use thread-local instances or create separate instances per thread
Distinguish between ModelContainer[T] (value wrapper) and ModelONEXContainer (DI container) - use ModelONEXContainer in node constructors
Implement nodes by inheriting from appropriate base class (NodeEffect, NodeCompute, NodeReducer, NodeOrchestrator) - never implement nodes without base classes
Files:
src/omnibase_core/nodes/__init__.py
🧠 Learnings (13)
📚 Learning: 2025-12-27T15:46:10.813Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-27T15:46:10.813Z
Learning: Applies to src/omnibase_core/**/*.py : Use Protocol-driven dependency injection with `container.get_service('ProtocolName')` - never use concrete class names or isinstance checks
Applied to files:
docs/architecture/CONTAINER_DI_DESIGN.mddocs/architecture/VALIDATION_PROTOCOL_COMPLIANCE.mdsrc/omnibase_core/services/service_protocol_migrator.pytests/unit/validation/test_migrator_protocol.pysrc/omnibase_core/services/service_protocol_auditor.pysrc/omnibase_core/services/__init__.py
📚 Learning: 2025-12-27T15:46:10.813Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-27T15:46:10.813Z
Learning: Applies to src/omnibase_core/**/*.py : Use Protocol abstractions for all service interfaces (ProtocolLogger, ProtocolEventBus, etc.) - never use concrete implementations
Applied to files:
docs/architecture/CONTAINER_DI_DESIGN.mddocs/architecture/CONTRACT_STABILITY_SPEC.mddocs/architecture/SPI_PROTOCOL_ADOPTION_ROADMAP.mddocs/architecture/VALIDATION_PROTOCOL_COMPLIANCE.mdtests/unit/validation/test_architecture.pysrc/omnibase_core/services/service_protocol_migrator.pytests/unit/validation/test_migrator_protocol.pytests/unit/validation/test_auditor_protocol_extended.pytests/unit/validation/test_protocol_auditor.pysrc/omnibase_core/services/service_protocol_auditor.pysrc/omnibase_core/services/__init__.py
📚 Learning: 2025-12-27T15:46:10.813Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-27T15:46:10.813Z
Learning: Applies to src/omnibase_core/**/*.py : Use duck typing with protocols instead of isinstance checks for type validation
Applied to files:
docs/architecture/CONTAINER_DI_DESIGN.mddocs/architecture/CONTRACT_STABILITY_SPEC.mddocs/architecture/SPI_PROTOCOL_ADOPTION_ROADMAP.mddocs/architecture/VALIDATION_PROTOCOL_COMPLIANCE.mdsrc/omnibase_core/services/service_protocol_migrator.pytests/unit/validation/test_auditor_protocol_extended.py
📚 Learning: 2025-12-27T15:46:10.813Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-27T15:46:10.813Z
Learning: Applies to src/omnibase_core/**/*.py : Use ModelOnexError with EnumCoreErrorCode for all error handling - never use generic Exception
Applied to files:
docs/architecture/IMPORT_COMPATIBILITY_MATRIX.mdsrc/omnibase_core/utils/util_conflict_resolver.pysrc/omnibase_core/__init__.pytests/unit/utils/test_util_contract_loader_part2.pytests/unit/utils/test_util_contract_loader_part1.py
📚 Learning: 2025-12-27T15:46:10.813Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-27T15:46:10.813Z
Learning: Applies to src/omnibase_core/**/*.py : Both mypy AND pyright must pass in CI with strict type checking (0 errors required for mypy)
Applied to files:
scripts/validate-no-spi-imports.sh
📚 Learning: 2025-12-27T15:46:10.813Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-27T15:46:10.813Z
Learning: Applies to src/omnibase_core/**/*.py : Use FileRegistry for loading YAML contracts: `FileRegistry().load(Path('config/runtime_host.yaml'))`
Applied to files:
tests/unit/infrastructure/test_node_base.pysrc/omnibase_core/utils/util_contract_loader.py.yaml-validation-allowlist.yamlsrc/omnibase_core/infrastructure/node_base.pytests/unit/utils/test_util_contract_loader_part2.pytests/unit/utils/test_util_contract_loader_part1.py
📚 Learning: 2025-12-27T15:46:10.813Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-27T15:46:10.813Z
Learning: Applies to src/omnibase_core/nodes/node_reducer.py : ModelReducer uses FSM-driven state machines with ModelIntent pattern for pure state transitions
Applied to files:
docs/patterns/PURE_FSM_REDUCER_PATTERN.mdsrc/omnibase_core/models/reducer/__init__.pysrc/omnibase_core/nodes/__init__.py
📚 Learning: 2025-12-27T15:46:10.813Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-27T15:46:10.813Z
Learning: Applies to src/omnibase_core/nodes/**/*.py : Distinguish between ModelContainer[T] (value wrapper) and ModelONEXContainer (DI container) - use ModelONEXContainer in node constructors
Applied to files:
src/omnibase_core/infrastructure/node_base.pytests/unit/utils/test_util_contract_loader_part2.pysrc/omnibase_core/nodes/__init__.pytests/unit/utils/test_util_contract_loader_part1.py
📚 Learning: 2025-12-27T15:46:10.813Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-27T15:46:10.813Z
Learning: Applies to src/omnibase_core/infrastructure/**/*.py : Always call `super().__init__(container)` in node constructors - this is mandatory and eliminates 80+ lines of boilerplate
Applied to files:
src/omnibase_core/infrastructure/node_base.py
📚 Learning: 2025-12-27T15:46:10.813Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-27T15:46:10.813Z
Learning: Applies to src/omnibase_core/nodes/**/*.py : Do NOT share node instances across threads without explicit synchronization - use thread-local instances or create separate instances per thread
Applied to files:
src/omnibase_core/infrastructure/node_base.py
📚 Learning: 2025-12-27T15:46:10.813Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-27T15:46:10.813Z
Learning: Applies to src/omnibase_core/nodes/node_orchestrator.py : ModelOrchestrator uses lease-based single-writer semantics with ModelAction pattern for workflow coordination
Applied to files:
src/omnibase_core/nodes/__init__.py
📚 Learning: 2025-12-27T15:46:10.813Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-27T15:46:10.813Z
Learning: Applies to src/omnibase_core/nodes/**/*.py : Implement nodes by inheriting from appropriate base class (NodeEffect, NodeCompute, NodeReducer, NodeOrchestrator) - never implement nodes without base classes
Applied to files:
src/omnibase_core/nodes/__init__.py
📚 Learning: 2025-12-27T15:46:10.813Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-27T15:46:10.813Z
Learning: Applies to src/omnibase_core/**/*.py : Use EnumNodeKind for architectural routing (EFFECT, COMPUTE, REDUCER, ORCHESTRATOR) and EnumNodeType for implementation discovery (COMPUTE_GENERIC, TRANSFORMER, AGGREGATOR, etc.)
Applied to files:
src/omnibase_core/nodes/__init__.py
🧬 Code graph analysis (22)
src/omnibase_core/__init__.py (1)
src/omnibase_core/services/service_validation_suite.py (1)
ServiceValidationSuite(23-109)
src/omnibase_core/models/reducer/__init__.py (2)
src/omnibase_core/utils/util_conflict_resolver.py (1)
UtilConflictResolver(51-163)src/omnibase_core/utils/util_streaming_window.py (1)
UtilStreamingWindow(49-134)
tests/unit/utils/test_util_cli_result_formatter.py (2)
src/omnibase_core/utils/util_cli_result_formatter.py (4)
UtilCliResultFormatter(14-114)format_output(23-49)format_error(52-81)format_summary(84-114)src/omnibase_core/models/cli/model_cli_output_data.py (1)
ModelCliOutputData(21-182)
src/omnibase_core/models/cli/model_cli_result.py (1)
src/omnibase_core/utils/util_cli_result_formatter.py (2)
UtilCliResultFormatter(14-114)format_output(23-49)
src/omnibase_core/utils/__init__.py (2)
src/omnibase_core/utils/util_conflict_resolver.py (1)
UtilConflictResolver(51-163)src/omnibase_core/utils/util_contract_loader.py (1)
UtilContractLoader(36-502)
tests/unit/validation/test_contract_validator.py (1)
src/omnibase_core/services/service_contract_validator.py (1)
ServiceContractValidator(60-872)
src/omnibase_core/services/service_protocol_migrator.py (2)
src/omnibase_core/models/core/model_capability.py (1)
validation(230-232)src/omnibase_core/types/typed_dict_migration_step_dict.py (1)
TypedDictMigrationStepDict(17-28)
src/omnibase_core/__init___fast.py (2)
src/omnibase_core/services/service_validation_suite.py (1)
ServiceValidationSuite(23-109)src/omnibase_core/validation/__init__.py (1)
validate_all(265-272)
tests/unit/test_init_exports.py (1)
src/omnibase_core/services/service_validation_suite.py (1)
ServiceValidationSuite(23-109)
examples/contract_validator_usage.py (1)
src/omnibase_core/services/service_contract_validator.py (1)
ServiceContractValidator(60-872)
src/omnibase_core/infrastructure/node_base.py (3)
src/omnibase_core/utils/util_contract_loader.py (1)
UtilContractLoader(36-502)tests/unit/utils/test_util_contract_loader_part1.py (1)
contract_loader(146-148)tests/unit/utils/test_util_contract_loader_part2.py (1)
contract_loader(115-117)
tests/unit/validation/test_migrator_protocol.py (1)
src/omnibase_core/services/service_protocol_migrator.py (1)
ServiceProtocolMigrator(34-468)
src/omnibase_core/models/cli/__init__.py (1)
src/omnibase_core/utils/util_cli_result_formatter.py (1)
UtilCliResultFormatter(14-114)
tests/unit/test_init_fast.py (3)
src/omnibase_core/__init___fast.py (1)
get_validation_suite(79-92)src/omnibase_core/services/service_validation_suite.py (1)
ServiceValidationSuite(23-109)src/omnibase_core/validation/__init__.py (1)
validate_all(265-272)
src/omnibase_core/models/services/model_external_service_config.py (1)
src/omnibase_core/utils/util_security.py (2)
UtilSecurity(17-390)mask_dict_credentials(90-136)
tests/unit/validation/test_auditor_protocol_extended.py (3)
src/omnibase_core/models/validation/model_audit_result.py (1)
ModelAuditResult(16-34)src/omnibase_core/models/validation/model_duplication_report.py (1)
ModelDuplicationReport(21-30)src/omnibase_core/services/service_protocol_auditor.py (1)
ServiceProtocolAuditor(47-584)
tests/unit/validation/test_protocol_auditor.py (3)
src/omnibase_core/errors/exception_configuration_error.py (1)
ExceptionConfigurationError(21-27)src/omnibase_core/services/service_protocol_auditor.py (1)
ServiceProtocolAuditor(47-584)src/omnibase_core/models/validation/model_audit_result.py (1)
ModelAuditResult(16-34)
src/omnibase_core/utils/util_cli_result_formatter.py (1)
src/omnibase_core/models/cli/model_cli_output_data.py (1)
ModelCliOutputData(21-182)
tests/unit/utils/test_util_contract_loader_part1.py (2)
src/omnibase_core/utils/util_contract_loader.py (1)
UtilContractLoader(36-502)tests/unit/utils/test_util_contract_loader_part2.py (3)
contract_loader(115-117)contract_loader_no_cache(121-123)valid_contract_yaml(48-63)
src/omnibase_core/models/security/__init__.py (1)
src/omnibase_core/utils/util_security.py (1)
UtilSecurity(17-390)
tests/unit/validation/test_cli.py (1)
src/omnibase_core/services/service_validation_suite.py (2)
ServiceValidationSuite(23-109)run_validation(50-71)
src/omnibase_core/validation/cli.py (1)
src/omnibase_core/services/service_validation_suite.py (1)
ServiceValidationSuite(23-109)
🪛 GitHub Actions: Test Suite
tests/unit/validation/test_architecture.py
[error] 3-3: I001 Import block is un-sorted or un-formatted
src/omnibase_core/services/service_protocol_migrator.py
[error] 9-31: I001 Import block is un-sorted or un-formatted
tests/unit/validation/test_auditor_protocol_extended.py
[error] 14-29: I001 Import block is un-sorted or un-formatted
tests/unit/validation/test_protocol_auditor.py
[error] 8-8: I001 Import block is un-sorted or un-formatted
src/omnibase_core/services/service_protocol_auditor.py
[error] 10-31: I001 Import block is un-sorted or un-formatted
src/omnibase_core/validation/cli.py
[error] 3-3: F401 Remove unused import: omnibase_core.models.errors.model_onex_error.ModelOnexError
[error] 23-23: F401 Remove unused import: omnibase_core.models.common.model_validation_metadata.ModelValidationMetadata
⏰ 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
- Fix unused imports in cli.py and import sorting in test files - Standardize backwards compatibility aliases to use __getattr__ with deprecation warnings - Consolidate duplicate imports in __getattr__ implementations - Add docstrings with previous name references to all 9 renamed classes - Add thread safety documentation to renamed classes - Add explanatory comments to all compatibility import sites - Replace generic exception handlers with specific exception types - Create validation script to check class names match ONEX naming conventions
Code Review - PR #261: refactor(naming): rename 9 non-conforming Model/Protocol classes (OMN-1071)SummaryThis is an excellent refactoring that systematically addresses naming convention violations across the codebase. The PR successfully renames 9 classes to follow ONEX naming conventions while maintaining full backwards compatibility. The implementation is thorough, well-tested, and includes valuable tooling for future validation. ✅ Strengths1. Comprehensive Backwards Compatibility ⭐
2. Excellent Documentation & Tooling ⭐
3. Correct Architectural Placement
4. Type Safety Maintained
5. Code Quality
🔍 Observations & Minor Suggestions1. Thread Safety Documentation ⭐The PR includes excellent thread safety documentation in class docstrings:
This aligns perfectly with CLAUDE.md's thread safety requirements. Well done! 2. Validation Script CompletenessThe validation script at
3. Error Code UsageIn raise ModelOnexError(
error_code=EnumCoreErrorCode.VALIDATION_ERROR,
message=f"Unknown validation type: {validation_type}",
)This correctly follows ONEX error handling patterns. ✅ However, consider adding raise ModelOnexError(
error_code=EnumCoreErrorCode.VALIDATION_ERROR,
message=f"Unknown validation type: {validation_type}",
context={"validation_type": validation_type, "available": list(self.validators.keys())}
)4. CI IntegrationThe -EXCLUDE_PATTERN="validation/migrator_protocol.py|validation/auditor_protocol.py"
+EXCLUDE_PATTERN="services/service_protocol_migrator.py|services/service_protocol_auditor.py"Good catch! Ensure the new 🔬 Testing ReviewCoverage
assert ModelCliResultFormatter is UtilCliResultFormatter, (
"ModelCliResultFormatter should be an alias for UtilCliResultFormatter"
)Test Organization
Suggestion: Consider adding integration tests that use the old names in realistic scenarios to ensure runtime behavior is preserved. 📋 Documentation ReviewUpdated Files (17 files)All documentation updates are consistent and accurate:
The documentation changes demonstrate thoroughness and attention to detail. 🛡️ Security Review
|
| Standard | Status |
|---|---|
| Poetry Usage | ✅ All examples use poetry run |
| PEP 604 Unions | ✅ Uses str | None syntax |
| Error Handling | ✅ Uses ModelOnexError with error codes |
| Type Checking | ✅ Mypy strict mode passes (0 errors) |
| Thread Safety | ✅ Documented for all renamed classes |
| Naming Conventions | ✅ PRIMARY GOAL OF THIS PR |
✨ Highlights
- Zero Breaking Changes: Full backwards compatibility maintained
- Proactive Tooling: Validation script prevents future violations
- Comprehensive Testing: 65 tests ensure aliases work correctly
- Documentation Excellence: 17 files updated consistently
- Architectural Improvement: Classes now in correct directories (
utils/,services/)
🚀 Recommendations for Merge
Pre-Merge Checklist
- ✅ Mypy passes (confirmed in PR description)
- ✅ All tests pass (65 new tests)
- ✅ Documentation updated
- ❓ Verify CI checks all pass (couldn't access
gh pr checks) - ❓ Ensure
validate_class_naming.pyis executable and complete
Post-Merge Actions
- Add validation to CI: Integrate
validate-class-naming.shinto GitHub Actions - Monitor performance: Track validation script runtime in CI
- Deprecation timeline: Add calendar reminder for v0.5.0 alias removal
- Communicate changes: Announce in changelog/release notes
🎓 Learning Opportunity
This PR serves as an excellent template for future refactoring work:
- Systematic approach to naming convention enforcement
- Automated validation tooling to prevent regressions
- Comprehensive backwards compatibility strategy
- Clear documentation of legacy exceptions
Consider creating a docs/guides/REFACTORING_CHECKLIST.md based on this PR's approach.
📝 Verdict
APPROVED ✅
This is high-quality refactoring work that:
- ✅ Solves the stated problem (OMN-1071)
- ✅ Maintains backwards compatibility
- ✅ Includes comprehensive testing
- ✅ Provides tooling for future prevention
- ✅ Updates all documentation
- ✅ Follows ONEX standards strictly
Impact: +2195 lines, -664 lines (net +1531)
Risk Level: Low (backwards compatible, well-tested)
Merge Recommendation: Approve and merge after CI passes
🙏 Great Work!
This PR demonstrates excellent software engineering practices:
- Attention to detail
- Comprehensive testing strategy
- Proactive tooling development
- Clear communication through documentation
Thank you for the thorough implementation! 🎉
Code Review - PR #261: Class Naming Convention Refactor (OMN-1071)SummaryThis PR implements a comprehensive refactoring to align 9 classes with ONEX naming conventions, moving them to appropriate directories and maintaining backwards compatibility. The changes are well-executed with strong attention to consistency and developer experience. ✅ Strengths1. Excellent Backwards Compatibility Strategy
2. Consistent Documentation
3. Strong Tooling Support
4. Architectural Alignment
5. Comprehensive Test Coverage
🔍 Areas for Consideration1. Allowed Exceptions in Validation ScriptLocation: The # Enums placed in model files (should move to enums/ directory)
("model_coercion_mode.py", "EnumCoercionMode"),
("model_status_protocol.py", "EnumStatusProtocol"),
("model_topic_parser.py", "EnumTopicStandard"),
# Protocols defined in model files (should move to protocols/ directory)
("model_protocol_action_payload.py", "ProtocolActionPayload"),
("model_protocol_intent_payload.py", "ProtocolIntentPayload"),
# Utility classes without Util prefix (legacy naming)
("util_field_converter.py", "FieldConverter"),
("util_tool_logger_code_block.py", "ToolLoggerCodeBlock"),Recommendation: Consider creating follow-up issues to address these legacy exceptions. The validation script correctly documents them as TODOs, but they should be tracked in the project backlog. 2. ServiceValidationSuite Error HandlingLocation: The except (ValueError, TypeError) as e:
# Data validation errors
results[validation_type] = ModelValidationResult(...)Consideration: This could mask unexpected errors. Consider:
Not a blocker - the current implementation is safe and functional, but enhanced error visibility would improve debuggability. 3. Documentation Update CompletenessFiles Updated: 10 documentation files Minor Gap: The PR updates most architecture docs but doesn't update:
Recommendation: Run a repository-wide search for the old class names in all grep -r "ModelCliResultFormatter\|ModelSecurityUtils\|ProtocolContractLoader\|ModelProtocolAuditor" docs/ --include="*.md"4. Thread Safety Documentation AccuracyLocation: The thread safety documentation states:
This is correct - the from typing import Final
def __init__(self) -> None:
self.validators: Final[dict[str, TypedDictValidatorInfo]] = {...}Optional enhancement - current code is already thread-safe. 🔒 Security Considerations✅ No security concerns identified
🎯 Performance Considerations✅ No performance regressions expected
📊 Test Coverage AnalysisTest Changes: +216 lines of backward compatibility tests
Assessment: ✅ Comprehensive test coverage for all renamed classes and backwards compatibility guarantees. 🚀 Migration Path QualityDeveloper Experience: ✅ Excellent
Example Quality: The # Updated to new canonical imports
from omnibase_core.services.service_contract_validator import (
ModelContractValidationResult,
ServiceContractValidator,
)📋 Checklist Review
🎓 RecommendationsMust Address Before Merge:None - PR is ready to merge pending CI green status. Should Consider (Non-Blocking):
Nice to Have:
🏆 Overall AssessmentRating: ⭐⭐⭐⭐⭐ (5/5) This is exemplary refactoring work:
The PR demonstrates exceptional attention to:
Recommendation: ✅ APPROVE (pending CI green) Great work on this refactor! The new validation tooling will prevent similar issues in the future, and the backwards compatibility strategy ensures zero disruption to existing code. CC: @jonahgabriel |
…-1071) - Fix import sorting in test_auditor_protocol_extended.py (CI ruff I001 failure) - Add get_validators() method to ServiceValidationSuite for programmatic access - Refactor list_validators() to use get_validators() internally
PR Review: refactor(naming): rename 9 non-conforming Model/Protocol classes (OMN-1071)Overall Assessment: EXCELLENTThis is a high-quality refactoring that significantly improves code consistency and maintainability. The PR successfully addresses naming convention violations while maintaining backwards compatibility. The implementation is thorough, well-documented, and follows best practices. Strengths1. Comprehensive Backwards Compatibility (5/5 stars)
2. Excellent Documentation
3. Automated Enforcement
4. Proper Service Organization
5. Code Quality Excellence
Code Quality AnalysisBest Practices Followed
Testing CoverageCOMPREHENSIVE TEST SUITE
Example test quality:
Potential Issues and RecommendationsMinor Issues
Enhancement Opportunities
Security ConsiderationsNO SECURITY CONCERNS
Performance ConsiderationsNO PERFORMANCE IMPACT
Architectural AlignmentPERFECTLY ALIGNED WITH ONEX PRINCIPLES
ONEX Naming Convention Compliance:
Final RecommendationsBefore Merge
Post-Merge Enhancements
SummaryThis PR represents exemplary refactoring work:
RECOMMENDATION: APPROVE The code quality, testing, and documentation standards are exceptional. This sets a strong precedent for future refactoring work in the omnibase_core codebase. Reviewed by Claude Code - Following CLAUDE.md conventions and ONEX architecture principles |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/omnibase_core/services/service_protocol_migrator.py (1)
1-7: Module docstring placement issue.The module docstring (lines 5-7) appears after the
ModelOnexErrorimport (line 3). In Python, the module docstring must be the first statement afterfrom __future__ import annotationsfor it to be recognized as__doc__. Currently, this docstring will be ignored.🔎 Proposed fix
from __future__ import annotations -from omnibase_core.models.errors.model_onex_error import ModelOnexError - """ Protocol migrator for safe migration of protocols to omnibase_spi. """ + +from omnibase_core.models.errors.model_onex_error import ModelOnexErrorsrc/omnibase_core/utils/util_contract_loader.py (2)
1-3: Module docstring placement issue.Similar to
service_protocol_migrator.py, the module docstring (lines 4-11) appears after the imports (lines 1-2). The docstring should come first to be recognized as the module's__doc__.🔎 Proposed fix
+""" +ContractLoader for ONEX Tool Generation Pattern Standardization. + +This module provides unified contract loading and resolution functionality +that supports the new contract architecture with subcontract references, +$ref resolution, and validation. This is part of PATTERN-005 NodeBase +implementation to eliminate duplicate node.py code. +""" + from omnibase_core.models.errors.model_onex_error import ModelOnexError from omnibase_core.models.primitives.model_semver import ModelSemVer - -""" -ContractLoader for ONEX Tool Generation Pattern Standardization. -... -"""
189-190: Inconsistent timezone usage in datetime objects.Line 189 uses
datetime.now()(naive datetime) while line 190 usesdatetime.fromtimestamp(..., tz=UTC)(aware datetime). Comparing naive and aware datetimes raisesTypeErrorin Python. Usedatetime.now(tz=UTC)for consistency.🔎 Proposed fix
- cached_at=datetime.now(), + cached_at=datetime.now(tz=UTC),tests/unit/validation/test_protocol_auditor.py (1)
238-249: Log message assertion may fail.At line 247, the test asserts
"ProtocolAuditor initialized" in log_message, but the service now logs"ServiceProtocolAuditor initialized"(see line 97 ofservice_protocol_auditor.py). This assertion will fail.🔎 Proposed fix
- assert "ProtocolAuditor initialized" in log_message + assert "ServiceProtocolAuditor initialized" in log_message
🧹 Nitpick comments (6)
src/omnibase_core/utils/util_conflict_resolver.py (1)
172-192: Minor inconsistency in deprecation documentation.Line 179 states
"(removed in v0.5.0)"for theModelConflictResolveralias, but other__getattr__implementations in this PR don't specify a removal version. Consider aligning the messaging across all deprecated aliases for consistency.src/omnibase_core/utils/__init__.py (1)
61-93: Return type annotation inconsistency.Line 61 declares
-> typebut other__getattr__implementations in this PR use-> Any. Whiletypeis more specific for class returns, using-> Anywould be consistent with the pattern elsewhere (e.g.,util_conflict_resolver.py,util_streaming_window.py).🔎 Proposed fix for consistency
-def __getattr__(name: str) -> type: +def __getattr__(name: str) -> Any:Note: This requires adding
Anyto the imports (e.g.,from typing import Any).src/omnibase_core/validation/__init__.py (1)
108-207: Lazy loading implementation is sound, but consider consolidating the deprecation check.The
__getattr__implementation correctly handles:
- Lazy imports to break circular dependencies
- Deprecation warnings for old names (stacklevel=2 is correct for
__getattr__)- Returning the canonical service classes
Minor observation: Lines 165-171 emit the deprecation warning for ALL deprecated aliases, including "ModelValidationSuite" (line 158-161), even though
ServiceValidationSuitewas already imported at the top of the module. This is correct and works as intended, but the dual mechanism (direct import + lazy alias) could be mentioned in the inline comments for clarity.💡 Optional: Add inline comment for ModelValidationSuite special case
# ModelValidationSuite (alias) - ServiceValidationSuite already imported at top if name == "ModelValidationSuite": + # Note: ServiceValidationSuite was imported directly at line 69, + # but we still need this branch to return it for the alias. return ServiceValidationSuitescripts/validate_class_naming.py (3)
227-243: Type variable check may be overly permissive.Line 234 allows any all-caps identifier ≤ 4 characters as a type variable exception. This could incorrectly allow 3-4 character acronyms (e.g.,
HTTP,JSON) that should follow naming conventions. Standard type variables are typically 1-2 characters (T, KT, VT, T_co).🔎 Tighten the type variable exception
- # Single-letter type variables are allowed - if len(class_name) <= 4 and class_name.isupper(): + # Type variables (T, KT, VT, T_co, etc.) are allowed + if len(class_name) <= 2 or (len(class_name) <= 4 and class_name.endswith("_co")): return TrueThis allows:
- 1-2 character all-caps names (T, KT, VT)
- Covariant type vars (T_co, up to 4 chars)
But rejects potential acronyms like HTTP, JSON, UUID as class names.
246-281: Hardcoded prefix list creates maintenance burden.Lines 272-273 hardcode the list of known prefixes for suggestion logic. This duplicates information from
NAMING_RULES(lines 56-112). If a new prefix is added to the rules, this list must also be updated manually, risking inconsistency.🔎 Derive prefix list from NAMING_RULES
At the module level, compute the known prefixes once:
# After NAMING_RULES definition (after line 112) KNOWN_PREFIXES = [rule.expected_prefix for rule in NAMING_RULES if not rule.is_suffix]Then update the check_class_name function:
# Try to detect existing prefix - for known_prefix in ["Model", "Service", "Util", "Protocol", "Enum", "Node", "Mixin"]: + for known_prefix in KNOWN_PREFIXES: if class_name.startswith(known_prefix):
284-345: Broad exception handler reduces error specificity.Lines 306-309 use a catch-all
except Exceptionafter handlingSyntaxError. While this prevents the script from crashing, it loses valuable debugging information. Consider handling specific exceptions (e.g.,UnicodeDecodeError,OSError) or at least logging the exception type.🔎 Add more specific exception handlers
try: source = file_path.read_text(encoding="utf-8") tree = ast.parse(source, filename=str(file_path)) except SyntaxError as e: return [], 0, f"Syntax error: {e}" + except (OSError, UnicodeDecodeError) as e: + return [], 0, f"Error reading file: {type(e).__name__}: {e}" except Exception as e: - return [], 0, f"Error reading file: {e}" + return [], 0, f"Unexpected error ({type(e).__name__}): {e}"This preserves the catch-all safety net while providing better error messages.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
scripts/README.mdscripts/validate-class-naming.shscripts/validate_class_naming.pysrc/omnibase_core/__init__.pysrc/omnibase_core/errors/__init__.pysrc/omnibase_core/models/cli/__init__.pysrc/omnibase_core/models/reducer/__init__.pysrc/omnibase_core/models/security/__init__.pysrc/omnibase_core/services/service_contract_validator.pysrc/omnibase_core/services/service_protocol_auditor.pysrc/omnibase_core/services/service_protocol_migrator.pysrc/omnibase_core/services/service_validation_suite.pysrc/omnibase_core/types/__init__.pysrc/omnibase_core/utils/__init__.pysrc/omnibase_core/utils/util_cli_result_formatter.pysrc/omnibase_core/utils/util_conflict_resolver.pysrc/omnibase_core/utils/util_contract_loader.pysrc/omnibase_core/utils/util_security.pysrc/omnibase_core/utils/util_streaming_window.pysrc/omnibase_core/validation/__init__.pysrc/omnibase_core/validation/cli.pytests/unit/validation/test_architecture.pytests/unit/validation/test_auditor_protocol_extended.pytests/unit/validation/test_protocol_auditor.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/unit/validation/test_architecture.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/services/service_validation_suite.pysrc/omnibase_core/errors/__init__.pytests/unit/validation/test_protocol_auditor.pysrc/omnibase_core/utils/util_conflict_resolver.pysrc/omnibase_core/services/service_protocol_auditor.pysrc/omnibase_core/utils/__init__.pysrc/omnibase_core/utils/util_security.pysrc/omnibase_core/services/service_contract_validator.pysrc/omnibase_core/models/cli/__init__.pysrc/omnibase_core/types/__init__.pyscripts/validate-class-naming.shsrc/omnibase_core/models/reducer/__init__.pysrc/omnibase_core/services/service_protocol_migrator.pysrc/omnibase_core/validation/cli.pysrc/omnibase_core/utils/util_contract_loader.pysrc/omnibase_core/validation/__init__.pyscripts/validate_class_naming.pysrc/omnibase_core/utils/util_streaming_window.pysrc/omnibase_core/__init__.pysrc/omnibase_core/utils/util_cli_result_formatter.pytests/unit/validation/test_auditor_protocol_extended.pysrc/omnibase_core/models/security/__init__.py
src/omnibase_core/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/omnibase_core/**/*.py: Use Protocol-driven dependency injection withcontainer.get_service('ProtocolName')- never use concrete class names or isinstance checks
Use ModelOnexError with EnumCoreErrorCode for all error handling - never use generic Exception
Use @standard_error_handling decorator to eliminate try/catch boilerplate in async operations
Use PEP 604 union syntax (str | None) instead of Optional/Union - enforce via ruff UP007
Use FileRegistry for loading YAML contracts:FileRegistry().load(Path('config/runtime_host.yaml'))
Use duck typing with protocols instead of isinstance checks for type validation
Maintain minimum 60% code coverage - enforce via pytest --cov in CI
Both mypy AND pyright must pass in CI with strict type checking (0 errors required for mypy)
Use EnumNodeKind for architectural routing (EFFECT, COMPUTE, REDUCER, ORCHESTRATOR) and EnumNodeType for implementation discovery (COMPUTE_GENERIC, TRANSFORMER, AGGREGATOR, etc.)
Use Protocol abstractions for all service interfaces (ProtocolLogger, ProtocolEventBus, etc.) - never use concrete implementations
Files:
src/omnibase_core/services/service_validation_suite.pysrc/omnibase_core/errors/__init__.pysrc/omnibase_core/utils/util_conflict_resolver.pysrc/omnibase_core/services/service_protocol_auditor.pysrc/omnibase_core/utils/__init__.pysrc/omnibase_core/utils/util_security.pysrc/omnibase_core/services/service_contract_validator.pysrc/omnibase_core/models/cli/__init__.pysrc/omnibase_core/types/__init__.pysrc/omnibase_core/models/reducer/__init__.pysrc/omnibase_core/services/service_protocol_migrator.pysrc/omnibase_core/validation/cli.pysrc/omnibase_core/utils/util_contract_loader.pysrc/omnibase_core/validation/__init__.pysrc/omnibase_core/utils/util_streaming_window.pysrc/omnibase_core/__init__.pysrc/omnibase_core/utils/util_cli_result_formatter.pysrc/omnibase_core/models/security/__init__.py
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Use ruff for linting and black for formatting - run pre-commit hooks before commit
Use isort for import sorting to maintain consistent import order
Files:
src/omnibase_core/services/service_validation_suite.pysrc/omnibase_core/errors/__init__.pytests/unit/validation/test_protocol_auditor.pysrc/omnibase_core/utils/util_conflict_resolver.pysrc/omnibase_core/services/service_protocol_auditor.pysrc/omnibase_core/utils/__init__.pysrc/omnibase_core/utils/util_security.pysrc/omnibase_core/services/service_contract_validator.pysrc/omnibase_core/models/cli/__init__.pysrc/omnibase_core/types/__init__.pysrc/omnibase_core/models/reducer/__init__.pysrc/omnibase_core/services/service_protocol_migrator.pysrc/omnibase_core/validation/cli.pysrc/omnibase_core/utils/util_contract_loader.pysrc/omnibase_core/validation/__init__.pyscripts/validate_class_naming.pysrc/omnibase_core/utils/util_streaming_window.pysrc/omnibase_core/__init__.pysrc/omnibase_core/utils/util_cli_result_formatter.pytests/unit/validation/test_auditor_protocol_extended.pysrc/omnibase_core/models/security/__init__.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Add test markers (@pytest.mark.unit, @pytest.mark.integration, @pytest.mark.slow, etc.) to all test functions
Test timeout protection: 60 seconds per test - use--timeout=60flag in pytest
Files:
tests/unit/validation/test_protocol_auditor.pytests/unit/validation/test_auditor_protocol_extended.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/cli/__init__.pysrc/omnibase_core/models/reducer/__init__.pysrc/omnibase_core/models/security/__init__.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.813Z
Learning: Applies to src/omnibase_core/**/*.py : Use Protocol abstractions for all service interfaces (ProtocolLogger, ProtocolEventBus, etc.) - never use concrete implementations
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-27T15:46:10.813Z
Learning: Applies to src/omnibase_core/**/*.py : Use Protocol-driven dependency injection with `container.get_service('ProtocolName')` - never use concrete class names or isinstance checks
📚 Learning: 2025-12-27T15:46:10.813Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-27T15:46:10.813Z
Learning: Applies to src/omnibase_core/**/*.py : Use duck typing with protocols instead of isinstance checks for type validation
Applied to files:
scripts/README.mdsrc/omnibase_core/services/service_protocol_migrator.pytests/unit/validation/test_auditor_protocol_extended.py
📚 Learning: 2025-12-27T15:46:10.813Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-27T15:46:10.813Z
Learning: Applies to **/*.{py,sh} : Always use `poetry run` prefix for Python commands (pytest, mypy, python -m, etc.) - never use pip or direct python execution
Applied to files:
scripts/README.md
📚 Learning: 2025-12-27T15:46:10.813Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-27T15:46:10.813Z
Learning: Applies to src/omnibase_core/**/*.py : Use ModelOnexError with EnumCoreErrorCode for all error handling - never use generic Exception
Applied to files:
src/omnibase_core/errors/__init__.pysrc/omnibase_core/utils/util_conflict_resolver.pysrc/omnibase_core/validation/cli.pysrc/omnibase_core/utils/util_contract_loader.pysrc/omnibase_core/__init__.py
📚 Learning: 2025-12-27T15:46:10.813Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-27T15:46:10.813Z
Learning: Applies to src/omnibase_core/**/*.py : Use Protocol abstractions for all service interfaces (ProtocolLogger, ProtocolEventBus, etc.) - never use concrete implementations
Applied to files:
tests/unit/validation/test_protocol_auditor.pysrc/omnibase_core/services/service_protocol_auditor.pysrc/omnibase_core/services/service_protocol_migrator.pytests/unit/validation/test_auditor_protocol_extended.py
📚 Learning: 2025-12-27T15:46:10.813Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-27T15:46:10.813Z
Learning: Applies to **/*.py : Use isort for import sorting to maintain consistent import order
Applied to files:
tests/unit/validation/test_protocol_auditor.py
📚 Learning: 2025-12-27T15:46:10.813Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-27T15:46:10.813Z
Learning: Applies to src/omnibase_core/**/*.py : Use Protocol-driven dependency injection with `container.get_service('ProtocolName')` - never use concrete class names or isinstance checks
Applied to files:
src/omnibase_core/services/service_protocol_auditor.pysrc/omnibase_core/services/service_protocol_migrator.py
📚 Learning: 2025-12-27T15:46:10.813Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-27T15:46:10.813Z
Learning: Applies to src/omnibase_core/nodes/node_reducer.py : ModelReducer uses FSM-driven state machines with ModelIntent pattern for pure state transitions
Applied to files:
src/omnibase_core/models/reducer/__init__.py
📚 Learning: 2025-12-27T15:46:10.813Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-27T15:46:10.813Z
Learning: Applies to src/omnibase_core/**/*.py : Use FileRegistry for loading YAML contracts: `FileRegistry().load(Path('config/runtime_host.yaml'))`
Applied to files:
src/omnibase_core/utils/util_contract_loader.py
🧬 Code graph analysis (12)
tests/unit/validation/test_protocol_auditor.py (4)
src/omnibase_core/errors/exception_configuration_error.py (1)
ExceptionConfigurationError(21-27)src/omnibase_core/models/validation/model_audit_result.py (1)
ModelAuditResult(16-34)src/omnibase_core/services/service_protocol_auditor.py (1)
ServiceProtocolAuditor(46-597)src/omnibase_core/protocols/event_bus/protocol_event_message.py (1)
value(33-35)
src/omnibase_core/utils/__init__.py (2)
src/omnibase_core/utils/util_conflict_resolver.py (1)
UtilConflictResolver(51-169)src/omnibase_core/utils/util_contract_loader.py (1)
UtilContractLoader(37-549)
src/omnibase_core/models/cli/__init__.py (1)
src/omnibase_core/utils/util_cli_result_formatter.py (1)
UtilCliResultFormatter(14-141)
src/omnibase_core/services/service_protocol_migrator.py (1)
src/omnibase_core/types/typed_dict_migration_step_dict.py (1)
TypedDictMigrationStepDict(17-28)
src/omnibase_core/validation/cli.py (1)
src/omnibase_core/services/service_validation_suite.py (1)
ServiceValidationSuite(23-184)
src/omnibase_core/utils/util_contract_loader.py (2)
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-168)
src/omnibase_core/validation/__init__.py (2)
src/omnibase_core/services/service_validation_suite.py (1)
ServiceValidationSuite(23-184)src/omnibase_core/services/service_contract_validator.py (1)
ServiceContractValidator(60-893)
scripts/validate_class_naming.py (4)
src/omnibase_core/protocols/event_bus/protocol_event_bus_headers.py (1)
source(50-52)src/omnibase_core/models/primitives/model_semver.py (1)
parse(448-480)src/omnibase_core/validation/patterns.py (1)
visit(35-37)src/omnibase_core/protocols/event_bus/protocol_event_message.py (1)
key(28-30)
src/omnibase_core/__init__.py (1)
src/omnibase_core/services/service_validation_suite.py (1)
ServiceValidationSuite(23-184)
src/omnibase_core/utils/util_cli_result_formatter.py (1)
src/omnibase_core/models/cli/model_cli_output_data.py (1)
ModelCliOutputData(21-182)
tests/unit/validation/test_auditor_protocol_extended.py (3)
src/omnibase_core/models/validation/model_audit_result.py (1)
ModelAuditResult(16-34)src/omnibase_core/models/validation/model_duplication_report.py (1)
ModelDuplicationReport(21-30)src/omnibase_core/services/service_protocol_auditor.py (1)
ServiceProtocolAuditor(46-597)
src/omnibase_core/models/security/__init__.py (1)
src/omnibase_core/utils/util_security.py (1)
UtilSecurity(17-401)
⏰ 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). (18)
- GitHub Check: Tests (Split 18/20)
- GitHub Check: Tests (Split 15/20)
- GitHub Check: Tests (Split 13/20)
- GitHub Check: Tests (Split 10/20)
- GitHub Check: Tests (Split 17/20)
- GitHub Check: Tests (Split 9/20)
- GitHub Check: Tests (Split 6/20)
- GitHub Check: Tests (Split 12/20)
- GitHub Check: Tests (Split 16/20)
- GitHub Check: Tests (Split 7/20)
- GitHub Check: Tests (Split 14/20)
- GitHub Check: Tests (Split 20/20)
- GitHub Check: Tests (Split 19/20)
- GitHub Check: Tests (Split 2/20)
- GitHub Check: Tests (Split 4/20)
- GitHub Check: Tests (Split 3/20)
- GitHub Check: Tests (Split 5/20)
- GitHub Check: claude-review
🔇 Additional comments (64)
src/omnibase_core/utils/util_cli_result_formatter.py (6)
11-11: LGTM on the absolute import path.Using absolute imports (
omnibase_core.models.cli.model_cli_output_data) instead of relative imports improves clarity and avoids potential circular import issues.
14-47: Well-documented class with comprehensive docstring.The rename from
ModelCliResultFormattertoUtilCliResultFormatterproperly follows ONEX naming conventions. The docstring appropriately documents:
- Methods available
- Usage example
- Thread safety guarantees
- Historical context with version reference
The thread safety claim is accurate since all methods are static and stateless.
49-76: LGTM onformat_outputmethod.The method correctly handles three cases with appropriate fallback logic. The exception handling is appropriately scoped to
TypeErrorandValueErrorwhich are the specific exceptions thatjson.dumpscan raise for serialization issues.
78-108: LGTM onformat_errormethod.Clean implementation with proper handling of optional parameters.
110-141: LGTM onformat_summarymethod.Correct implementation with proper formatting of execution summary.
144-144: LGTM on__all__export.The export list correctly reflects the renamed class.
src/omnibase_core/models/cli/__init__.py (4)
1-17: Well-documented backwards compatibility section.The module docstring clearly explains the deprecation strategy and provides guidance for migration, which is helpful for users encountering the deprecated alias.
19-19: LGTM on theAnyimport.Required for the
__getattr__return type annotation.
60-61: LGTM on deprecated export in__all__.Including the deprecated alias in
__all__with a clear deprecation comment maintains IDE autocomplete and documentation discoverability while signaling the deprecation intent.
65-91: LGTM on lazy-loading deprecation pattern.The implementation correctly:
- Uses
stacklevel=2to point the warning at the caller's code- Defers the import to avoid circular dependencies and ensure lazy loading
- Raises
AttributeErrorfor unknown attributes (required for proper module behavior)- Uses
DeprecationWarningwhich is the appropriate category for this use caseOne minor observation: the
import warningsis inside the function, which means it's imported on each access. This is fine for a deprecated code path where performance isn't critical, and it keeps the module's top-level imports clean.src/omnibase_core/types/__init__.py (2)
660-665: Clear documentation of the lazy loading rationale.The header comment block effectively explains the purpose of the
__getattr__implementation and distinguishes it from the backwards-compatibility alias pattern used elsewhere in the codebase (OMN-1071). This helps prevent future maintainers from confusing the two patterns.
666-676: LGTM!The docstring updates provide helpful clarity:
- ASCII arrows improve portability across different terminal environments
- The explicit note that this is not a BC mechanism (with cross-reference to
validation/__init__.pyfor that pattern) prevents developers from mistakenly copying this code for backwards-compatibility alias purposessrc/omnibase_core/utils/util_streaming_window.py (1)
1-163: LGTM! Clean implementation of the rename with proper backwards compatibility.The
__getattr__deprecation pattern is consistent with the PR's approach. Thread safety documentation and examples are thorough and helpful.src/omnibase_core/services/service_protocol_migrator.py (1)
33-67: Well-documented thread safety considerations.The class docstring clearly explains the threading model: read operations are safe for concurrent access, but
execute_migration()requires external coordination. The example usage is helpful.src/omnibase_core/utils/util_conflict_resolver.py (1)
51-169: LGTM! Proper rename with ModelOnexError usage.The error handling in
resolve()correctly usesModelOnexErrorwithEnumCoreErrorCode.VALIDATION_ERRORas per coding guidelines.src/omnibase_core/utils/util_contract_loader.py (2)
134-147: Good: Specific exception handling with appropriate error codes.The distinction between
OSError→OPERATION_FAILEDand(TypeError, ValueError, KeyError)→VALIDATION_ERRORcorrectly maps filesystem errors vs. data errors to their semantic error codes.
488-548: YAML security validation is thorough.The
_validate_yaml_content_securitymethod provides good defense-in-depth with size limits, suspicious pattern detection, and nesting depth checks. The warning logs for suspicious patterns (without failing) allow for monitoring without blocking legitimate edge cases.src/omnibase_core/utils/__init__.py (1)
20-54: LGTM! Clear separation of direct imports vs lazy-loaded utilities.The documentation clearly explains why some utilities are lazy-loaded (heavy model dependencies, circular import avoidance) while others are imported directly.
src/omnibase_core/models/reducer/__init__.py (2)
38-52: LGTM! Clean backwards compatibility implementation.The module correctly imports the canonical
Util*classes directly and provides deprecatedModel*aliases via__getattr__. The__all__list properly includes both for discoverability.
59-89: Consistent deprecation pattern.The
__getattr__implementation follows the same pattern as other modules in this PR, with appropriateDeprecationWarningandstacklevel=2.scripts/validate-class-naming.sh (1)
1-38: LGTM! Well-structured shell wrapper.The script follows best practices:
- Uses
poetry runprefix as required by coding guidelines- Enables strict mode with
set -euo pipefail- Documents exit codes and usage clearly
- Uses
execto replace the shell process, avoiding an extra process layerscripts/README.md (2)
167-172: LGTM! Documentation follows established patterns.The new class naming validation section uses
poetry run pythonprefix as required by coding guidelines and provides clear examples with--fix-suggestionsand--jsonoptions.
222-224: Good addition to Protocol Checking section.Adding the class naming validator here makes sense as it complements the other protocol/naming checks in this section.
src/omnibase_core/services/service_protocol_auditor.py (3)
23-29: LGTM!The import path update to an absolute path (
omnibase_core.validation.validation_utils) is correct and maintains consistency with the module structure.
46-70: LGTM!The class rename to
ServiceProtocolAuditorfollows ONEX naming conventions correctly. The docstring enhancements are valuable:
- Thread safety documentation clearly explains the non-thread-safe nature and mutable state concerns
- The historical note about the previous name (
ModelProtocolAuditor) with version reference helps maintainability
254-256: LGTM!The recursive instantiation of
ServiceProtocolAuditorinaudit_ecosystemcorrectly uses the new class name, maintaining consistency within the ecosystem auditing flow.src/omnibase_core/services/service_contract_validator.py (2)
60-92: LGTM!The rename from
ProtocolContractValidatortoServiceContractValidatorcorrectly follows ONEX naming conventions. The enhanced docstring provides excellent documentation:
- Thread safety section clearly describes mutable state (
custom_rules,onex_standards,architecture_rules)- Usage example demonstrates the new import path and API
- Historical note references the previous name for traceability
896-899: LGTM!The
__all__export list correctly includes the newServiceContractValidatorname.src/omnibase_core/services/service_validation_suite.py (6)
1-21: LGTM!The imports are well-organized and include all necessary dependencies. The module docstring clearly states its purpose.
23-57: LGTM!Excellent class documentation:
- Thread safety claim is accurate - the validators dictionary is populated once during
__init__and never modified thereafter- The historical note correctly references the previous name (
ModelValidationSuite) with version- The example demonstrates the new import path from
omnibase_core.services
59-81: LGTM!The validator registry is well-structured with each entry containing the function reference, description, and list of accepted arguments. This design supports the kwargs filtering in
run_validation.
83-104: LGTM!The
run_validationmethod correctly:
- Validates the validation type exists
- Filters kwargs to only pass relevant arguments to each validator
- Uses
ModelOnexErrorwithEnumCoreErrorCodeper coding guidelines
106-149: LGTM!The error handling in
run_all_validationsis comprehensive:
- Catches
ModelOnexErrorfor ONEX framework errors- Catches
OSErrorfor filesystem errors (includesFileNotFoundError,PermissionError)- Catches
ValueError/TypeErrorfor data validation errorsEach error path creates a proper
ModelValidationResultwith appropriate metadata.
151-184: LGTM!Good separation of concerns:
get_validators()returns structured data for programmatic accesslist_validators()provides formatted CLI output and reusesget_validators()internallyThe CLI examples in
list_validators()correctly show the module path.src/omnibase_core/errors/__init__.py (3)
80-105: LGTM!Excellent documentation clarifying that this lazy loading is for circular import avoidance, not backwards compatibility aliases. The distinction from OMN-1071 pattern is clearly stated, which helps maintainers understand the different lazy-loading use cases in the codebase.
106-126: LGTM!The consolidated import logic for
models.commonclasses is well-structured. The mapping approach avoids repeated import statements while maintaining readability.
134-174: LGTM!The grouped lazy loading for runtime errors and declarative errors is clean:
- Uses sets for O(1) membership checks
- Uses
getattron the imported module to retrieve the requested class- Maintains consistent structure across error class categories
src/omnibase_core/__init__.py (3)
51-69: LGTM!The documentation clearly distinguishes this lazy loading mechanism (for breaking circular import chains) from the backwards compatibility alias pattern in OMN-1071. This consistency with the errors module helps maintainers understand the codebase.
79-96: LGTM!The lazy import set correctly includes
ServiceValidationSuite(replacingModelValidationSuite) along with the validation functions. The import block properly retrieves all validation exports.
129-141: LGTM!The
__all__export list correctly reflects the public API:
ServiceValidationSuitereplacesModelValidationSuite- All validation functions remain exported
tests/unit/validation/test_protocol_auditor.py (5)
15-17: LGTM!Import paths correctly updated:
ExceptionConfigurationErrorfromomnibase_core.errors.exceptionsModelAuditResultfromomnibase_core.models.validation.model_audit_resultServiceProtocolAuditorfromomnibase_core.services.service_protocol_auditor
30-31: LGTM!Test correctly instantiates
ServiceProtocolAuditorwith the new class name.
74-75: LGTM!The patch target correctly references the new module path
omnibase_core.services.service_protocol_auditor.logger.
207-209: LGTM!The patch decorator correctly targets
omnibase_core.services.service_protocol_auditor.extract_protocols_from_directory.
283-285: LGTM!Patch decorator correctly uses the new module path for
extract_protocols_from_directory.tests/unit/validation/test_auditor_protocol_extended.py (3)
1-2: LGTM!Module docstring correctly updated to reference
ServiceProtocolAuditor.
21-27: LGTM!Import paths correctly updated:
ModelAuditResultfromomnibase_core.models.validation.model_audit_resultModelDuplicationReportfromomnibase_core.models.validation.model_duplication_reportServiceProtocolAuditorfromomnibase_core.services.service_protocol_auditor
43-44: LGTM!All test instantiations consistently use
ServiceProtocolAuditorthroughout the test file.Also applies to: 67-68, 90-91, 116-117, 144-145, 194-195, 243-244, 285-286, 352-353, 372-373, 384-385, 396-397, 414-415, 428-429, 447-448, 468-469, 490-491, 534-535, 560-561, 585-586, 611-612
src/omnibase_core/validation/cli.py (3)
19-25: LGTM!The comment clearly explains the rationale for the direct module import to avoid circular imports. This is a good practice and well-documented.
30-50: LGTM!The backwards compatibility alias implementation is correct:
- Uses
DeprecationWarning(appropriate for deprecation)stacklevel=2correctly points to the caller's code- Clear deprecation message with migration guidance
- Proper
AttributeErrorfor unknown attributes
170-171: LGTM!CLI correctly instantiates
ServiceValidationSuitefor the main entry point.src/omnibase_core/validation/__init__.py (5)
41-45: LGTM! Import addition is clean.The explicit import of
ModelContractValidationResultproperly exposes this model in the validation module's public API.
66-69: Direct import is appropriate here.Importing
ServiceValidationSuitedirectly from.cliis the right choice, as the comment block below explains. The cli module doesn't create circular imports, making this simpler than lazy loading.
71-105: Comprehensive documentation justifies the architectural decision.This extended comment block thoroughly explains the rationale for choosing between direct aliases and lazy
__getattr__loading. While verbose, it provides essential context for future maintainers who might otherwise be confused by the mixed approach.
301-301: Direct instantiation diverges from DI pattern.Line 301 directly instantiates
ServiceValidationSuite()rather than using the Protocol-driven dependency injection pattern recommended in the coding guidelines. While this may be acceptable for a convenience function in the public API, consider whether this should usecontainer.get_service('ServiceValidationSuite')for consistency with the broader architecture.Based on coding guidelines: "Use Protocol-driven dependency injection with
container.get_service('ProtocolName')- never use concrete class names or isinstance checks."Is this convenience function intentionally bypassing DI, or should it follow the container pattern?
314-323: all exports are correctly updated.All canonical service classes (
ServiceContractValidator,ServiceProtocolAuditor,ServiceProtocolMigrator,ServiceValidationSuite) and their backward-compatibility aliases are properly exported. The lazy__getattr__mechanism will handle the imports when accessed.scripts/validate_class_naming.py (4)
1-27: LGTM! Documentation and usage follow guidelines.The docstring clearly explains the script's purpose, ONEX naming conventions, usage examples, and exit codes. The usage examples correctly use
poetry runas required by the coding guidelines.
40-112: Naming rules are comprehensive and well-structured.The
NamingRuleNamedTuple andNAMING_RULESlist cover all major ONEX conventions (Model, Service, Util, Protocol, Enum, Error, Node, Mixin) with clear descriptions. The suffix handling for*Errorclasses is correctly distinguished.
120-147: Allowed exceptions are well-documented with rationale.The explicit documentation of legacy exceptions (with TODO notes) provides clarity for future refactoring. Each exception is justified, making it clear these are intentional deviations rather than oversights.
372-513: CLI implementation is robust and user-friendly.The main function provides:
- Comprehensive argument parsing with verbose, suggestions, custom source dir, and JSON output
- Clear error messages and exit codes
- Grouped violation output by rule
- Summary statistics
The implementation handles both success and failure cases appropriately.
src/omnibase_core/utils/util_security.py (2)
29-46: Excellent documentation additions!The thread-safety documentation is thorough and the deprecation note clearly guides users to the new naming convention. The example update and explicit reasoning about the rename (OMN-1071) provide valuable context.
118-383: Renaming toUtilSecurityis complete and properly verified.All internal references throughout the file have been updated to use
UtilSecurity. No stray references to the oldModelSecurityUtilsclass exist in production code—only in the test filetests/unit/test_backwards_compatibility_omn1071.py, which explicitly tests the backwards-compatibility layer. The deprecation mechanism insrc/omnibase_core/models/security/__init__.pyis properly configured to warn users of the old name.src/omnibase_core/models/security/__init__.py (2)
3-82: Clear backwards compatibility documentation.The module docstring effectively communicates the deprecation strategy and migration path. Including
ModelSecurityUtilsin__all__with a deprecation comment maintains the public API while guiding users toward the new naming.
89-111: Proper lazy-loading implementation with deprecation.The
__getattr__implementation follows Python best practices:
- Uses
DeprecationWarning(appropriate for API deprecation)stacklevel=2correctly points to the caller's code- Clear migration message with the new import path
- Proper
AttributeErrorfor unknown attributes
| logger.info( | ||
| f"ModelProtocolAuditor initialized for repository '{self.repository_name}' " | ||
| f"ServiceProtocolAuditor initialized for repository '{self.repository_name}' " | ||
| f"at {self.repository_path}" | ||
| ) |
There was a problem hiding this comment.
Minor inconsistency in log message.
The log message uses ServiceProtocolAuditor but the test at line 247 in test_protocol_auditor.py expects ProtocolAuditor initialized (without the Service prefix). This will cause the test assertion to fail.
🔎 Proposed fix
Either update the log message to be consistent with what tests expect, or update the test. Given the rename, updating the test assertion seems more appropriate (which appears to be already done correctly - the test checks for "initialized" and "repository" in lowercase, so it should pass).
Actually, re-checking line 247: assert "ProtocolAuditor initialized" in log_message - this expects the old class name format. The log now says ServiceProtocolAuditor initialized.
Update the test expectation:
- assert "ProtocolAuditor initialized" in log_message
+ assert "ServiceProtocolAuditor initialized" in log_messageCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/omnibase_core/services/service_protocol_auditor.py around lines 96 to 99,
the info log uses "ServiceProtocolAuditor initialized…" but tests expect
"ProtocolAuditor initialized"; update the log message to "ProtocolAuditor
initialized for repository '{self.repository_name}' at {self.repository_path}'"
(maintain same repository info and formatting) so the runtime log matches the
test assertion; run the tests to confirm the assertion passes.
1 participant
Summary
Model/Protocolprefixes to follow ONEX naming conventionsutils/for Util*,services/for Service*)ONEX Naming Conventions
ModelBaseModelProtocoltyping.ProtocolinterfaceServiceUtilChanges
Util classes (moved to
utils/):ModelCliResultFormatterUtilCliResultFormatterModelConflictResolverUtilConflictResolverModelStreamingWindowUtilStreamingWindowModelSecurityUtilsUtilSecurityProtocolContractLoaderUtilContractLoaderService classes (moved to
services/):ModelProtocolAuditorServiceProtocolAuditorModelValidationSuiteServiceValidationSuiteProtocolContractValidatorServiceContractValidatorProtocolMigratorServiceProtocolMigratorTest plan
Success: no issues found in 2372 source filesSummary by CodeRabbit
New Features
API Changes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.