Repository navigation
feat(contracts): Phase 0 stabilization + test fixes [OMN-151, OMN-155, OMN-156, OMN-157] - #143
Conversation
…MN-155, OMN-156, OMN-157] Implements foundational infrastructure for Phase 0 repository stabilization: ## Contracts Module (OMN-155) - ContractHashRegistry: Deterministic SHA256 fingerprinting for ONEX contracts - compute_contract_fingerprint: Generates fingerprints in format `<semver>:<hash>` - normalize_contract: Canonical JSON normalization for deterministic hashing - Drift detection between declarative and legacy versions ## Contract Models - ModelContractFingerprint: Fingerprint with version, hash, and metadata - ModelContractMeta: Meta-model for cross-node contract consistency - ModelContractNodeMetadata: Typed metadata for node contracts - ModelNodeExtensions: Typed extension data (replaces dict[str, Any]) - ModelDriftResult/ModelDriftDetails: Drift detection results - ModelContractNormalizationConfig: Normalization pipeline config ## Node Purity Guarantees (OMN-156) - NODE_PURITY_GUARANTEES.md: Documentation with forbidden patterns - check_node_purity.py: AST-based purity checker for CI enforcement - test_node_purity.py: Comprehensive pattern detection tests ## Import Compatibility Matrix (OMN-157) - IMPORT_COMPATIBILITY_MATRIX.md: Documents all public import paths - Stability levels: STABLE, RECOMMENDED, DEPRECATED, INTERNAL - Migration guidance for v0.4.0 consumers ## ONEX Compliance - All models follow single-class-per-file rule - Proper Model* prefix naming conventions - Files in correct directories (models/, contracts/) - No dict[str, Any] anti-patterns - 131 new tests with 100% pass rate
- Fix 26 pytest collection warnings by renaming Test* helper classes to Mock* or Sample* prefix across 20+ test files - Fix test_validate_yaml_file_permission_denied test to properly skip when running as root (e.g., in Docker containers) - Fix PytestRemovedIn9Warning by converting cleanup_service_tasks from async to sync fixture - Fix DeprecationWarning by removing deprecated asyncio.get_event_loop() call in event_loop_cleanup fixture - Revert ServiceContractHashRegistry -> ContractHashRegistry to comply with ONEX naming conventions (Service* classes must be in services/) Files renamed Test* -> Mock*/Sample*: - TestNode -> MockNode (mixins) - TestServiceImplementation -> MockServiceImplementation (container) - TestEventModel -> MockEventModel (mixins) - TestInputState/TestOutputState -> MockInputState/MockOutputState - TestResultModel -> SampleResultModel (models) - TestTypedModel -> SampleTypedModel (models) - TestCredentials -> SampleCredentials (security) - TestStatus -> SampleStatus (utils) - And more... This eliminates all pytest collection warnings and deprecation warnings.
… [OMN-151]
Add architectural exemption for ContractHashRegistry in contracts/ directory.
The class follows the established {Domain}Registry naming pattern (like
ServiceRegistry) but was being flagged because no Contract prefix pattern
exists in the naming validator.
This is a stateful registry service for contract hash management, not a
Protocol or Pydantic Model, so it correctly doesn't use those prefixes.
WalkthroughAdds deterministic contract fingerprinting and registry, a set of new contract Pydantic models, an AST-based node purity checker and tests, expanded naming/validation tooling, extensive docs, and many test renames and fixture tweaks. Includes minor YAML formatting edits and small script path adjustment. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
|
@claude review please |
|
Claude finished @jonahgabriel's task in 2m 26s —— View job Code Review Complete ✅I've completed a thorough review of this Phase 0 stabilization PR. Overall, this is excellent work with strong architecture, comprehensive test coverage, and proper attention to detail. Below are my findings organized by category. 🎯 Summary AssessmentRecommendation: ✅ Approve with minor suggestions This PR successfully implements Phase 0 stabilization infrastructure with:
✅ Strengths1. Excellent Architecture & DesignThe contract fingerprinting infrastructure is well-designed:
2. Comprehensive Test CoverageContract Tests (tests/unit/contracts/):
Node Purity Tests (tests/unit/nodes/test_node_purity.py):
3. Proper Datetime MigrationClean migration from deprecated
Example (tests/conftest.py:117-126): # Clean approach: only get running loop, not deprecated get_event_loop()
try:
loop = asyncio.get_running_loop()
except RuntimeError:
return # No running loop - nothing to clean up4. Documentation ExcellenceNew architecture docs are comprehensive:
💡 Suggestions for Improvement1. Thread Safety Documentation (Minor)Issue: Location: src/omnibase_core/contracts/hash_registry.py:246-268 Suggestion: Consider: class ContractHashRegistry:
"""Registry for storing and retrieving contract fingerprints.
Thread Safety:
- Reads (lookup, verify): Thread-safe after initialization
- Writes (register): Requires external synchronization
This matches the pattern of ServiceRegistry and ModelONEXContainer
which are read-safe after initialization.
"""Rationale: Most registry usage is read-heavy after initial loading. Documenting read-safety would align with ONEX patterns. 2. Normalization Config Defaults (Minor)Issue: Location: src/omnibase_core/models/contracts/model_contract_normalization_config.py Suggestion: Add docstring explaining:
3. Error Context Enhancement (Nice-to-have)Issue: Some validation errors could include more context for debugging. Example (hash_registry.py:160-164): if not isinstance(converted, dict):
raise ModelOnexError(
message="Contract must be a dictionary",
error_code=EnumCoreErrorCode.VALIDATION_ERROR,
)Suggestion: Add actual type to error context: raise ModelOnexError(
message=f"Contract must be a dictionary, got {type(converted).__name__}",
error_code=EnumCoreErrorCode.VALIDATION_ERROR,
actual_type=type(converted).__name__,
)4. Test Timeout Consistency (Minor)Issue: Some test classes use Location: tests/unit/contracts/test_hash_registry.py:90, and others Suggestion: Either:
|
…N-151] Add Google-style docstrings to Phase 0 stabilization infrastructure: - Contract core modules (contracts/__init__.py, hash_registry.py) - Contract models (8 model files in models/contracts/) - Validation scripts (check_node_purity.py, validate_naming.py) Fix mypy errors in validate_naming.py: - Add explicit type annotation to NAMING_PATTERNS class variable - Update method signatures to use dict[str, str | None] - Add None checks before using optional values - All 13 pre-existing mypy errors resolved Improve documentation for synthetic hash placeholder in ModelContractFingerprint.from_string() to clarify design intent.
Pull Request Review: Phase 0 Stabilization InfrastructureSummaryThis PR introduces foundational infrastructure for Phase 0 repository stabilization, including contract fingerprinting, hash registry, node purity guarantees, and comprehensive test coverage. Overall, this is high-quality work that follows ONEX architectural patterns and conventions. ✅ Strengths1. Excellent Architecture & Design
2. ONEX Compliance
3. Test Quality
4. Code Quality
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/conftest.py (1)
304-366: Task cancellation incleanup_service_tasksis incomplete: awaits are missing.This fixture cancels health monitor and service loop tasks but never awaits the cancellation, unlike
event_loop_cleanup(lines 108–173) which properly usesloop.run_until_complete(asyncio.gather(..., return_exceptions=True)). During pytest-asyncio async test teardown,asyncio.get_running_loop()succeeds and the fixture reaches the cancellation logic, but the fire-and-forget approach leaves tasks in a cancelled-but-pending state.Update
cleanup_service_tasksto await task cancellation with a timeout similar toevent_loop_cleanup, or clarify if incomplete cancellation is intentional.
🧹 Nitpick comments (20)
tests/unit/container/test_service_registry.py (1)
22-34: MockServiceImplementation helper is well-structured; duplication with extended tests could be centralized (optional).The renamed helper is clear, and the “Mock” prefix correctly avoids pytest auto‑collecting it as a test class while signalling intent. The execute/
execution_countbehavior remains appropriate for exercising registry semantics.Given that
ITestServiceandMockServiceImplementationare defined identically intests/unit/container/test_service_registry_extended.pyas well, you might eventually want a small shared helper module or fixture to avoid keeping two copies in sync, but this is strictly optional for now.src/omnibase_core/models/contracts/model_drift_result.py (1)
42-106: Drift result model is solid; consider tighteningdrift_typeand double‑check the extra exportThe
ModelDriftResultshape and config (immutable,extra="forbid", structured references toModelContractFingerprintandModelDriftDetails) look consistent with the rest of the contract model layer.Two optional tweaks to consider:
drift_typeis documented as'version' | 'content' | 'both' | Nonebut typed asstr | None; usingLiteral["version", "content", "both"] | None(or an enum) would give stronger type safety without changing runtime behaviour.__all__re‑exports"ModelDriftDetails"from this module even though it lives inmodel_drift_details.py; if that re‑export is not intentional (e.g., you already re‑export from the package__init__), you might want to keep__all__scoped to"ModelDriftResult"here to avoid surprising import paths.Also applies to: 109-109
scripts/validation/validate_naming.py (2)
469-489:_contains_relevant_classesignores itspatternargument and may over‑flag files
_contains_relevant_classesaccepts apatternbut currently returnsTruefor any file that has a non‑exception class, without considering the pattern at all. That both makes the signature/docstring misleading and broadens the “relevant file” heuristic for file‑prefix warnings.You might want to either:
- Incorporate
patterninto the check (e.g., only returnTrueif at least one class name could/should match the pattern), or- Drop the
patternparameter and adjust the docstring if the broader behaviour is intentional.Tightening this now will avoid surprising warnings as the codebase grows.
735-781: Consider reconcilingmain()’s return annotation with itssys.exitbehaviour
main()is annotated and documented as returning anint, but it only ever callssys.exit(...)and never actually returns. For clarity and easier reuse in tests/tools, you could either:
- Have
main()compute andreturnthe exit code, and let the__main__guard callsys.exit(main()), or- Drop the return annotation and “Returns” section from the docstring if it’s meant to be CLI‑only.
This is purely a polish/readability tweak; behaviour today is consistent for CLI usage.
docs/architecture/IMPORT_COMPATIBILITY_MATRIX.md (1)
605-607: Add a language to the fenced dependency diagram block (MD040)The fenced block around the dependency arrow diagram currently has no language spec, which triggers markdownlint MD040. Consider marking it explicitly, for example:
-``` +```text omnibase_infra ──implements──► omnibase_spi ──imports──► omnibase_coreThis keeps rendering unchanged while satisfying the linter. </blockquote></details> <details> <summary>src/omnibase_core/models/contracts/model_contract_normalization_config.py (1)</summary><blockquote> `31-80`: **Normalization config model is well‑designed; minor optional cleanup** `ModelContractNormalizationConfig` cleanly captures the normalization toggles and constrained `hash_length` with an immutable, extra‑forbid Pydantic model. This is aligned with deterministic fingerprinting needs. One tiny optional nit: with `frozen=True`, `validate_assignment=True` is redundant because assignments are already blocked; you could drop `validate_assignment` for marginal simplification if desired. Also applies to: 82-86 </blockquote></details> <details> <summary>tests/unit/mixins/test_mixin_intent_publisher.py (1)</summary><blockquote> `25-35`: **Intent publisher test double renames are correct; consider stronger metadata checks later** `MockEventModel` and `MockNodeWithIntentPublisher` replace the previous Test* names consistently, including assertions that depend on `__class__.__name__`. This keeps the suite aligned with the new naming convention without altering behavior. If you revisit these tests in the future, you might consider asserting the actual `created_by` and `target_event_type` values from the published intent/envelope instead of only checking the class names, but that’s optional. Also applies to: 76-88, 90-101, 103-117, 127-132, 369-378, 381-395 </blockquote></details> <details> <summary>docs/architecture/NODE_PURITY_GUARANTEES.md (1)</summary><blockquote> `47-50`: **Doc content is solid; two tiny wording nits are optional** The purity guarantees and enforcement description are clear and actionable. Two extremely small polish tweaks, if you care about LanguageTool cleanliness: - Consider changing “can fail, timeout, or return different results” to “can fail, time out, or return different results” to avoid the verb‑form “timeout”. - In the CI section, you might phrase it as “Add to the GitHub workflow `.github/workflows/test.yml`:” so “GitHub” is capitalized while keeping the actual path unchanged. These are purely editorial and won’t affect behavior. Also applies to: 229-235 </blockquote></details> <details> <summary>src/omnibase_core/models/contracts/model_contract_meta.py (2)</summary><blockquote> `308-337`: **Cross-node consistency validator is a no-op.** The `validate_cross_node_consistency` method contains two conditionals that do nothing (just `pass`). While the docstring mentions "warnings in logging contexts," no logging is actually performed. This is dead code that adds complexity without value. Consider either implementing the warning logic or removing this validator entirely until it's needed. ```diff @model_validator(mode="after") def validate_cross_node_consistency(self) -> ModelContractMeta: """Validate cross-node consistency requirements. Performs additional validation to ensure the meta-model adheres to ONEX standards. Currently validates that input/output schemas follow ONEX naming patterns (allowing external models with warnings). Returns: The validated ModelContractMeta instance. Note: External models (not prefixed with 'omnibase_') are allowed but may generate warnings in logging contexts. """ - # Validate input_schema follows ONEX patterns - if not self.input_schema.startswith( - "omnibase_core." - ) and not self.input_schema.startswith("omnibase_"): - # Allow external models but log a warning pattern - pass - - # Validate output_schema follows ONEX patterns - if not self.output_schema.startswith( - "omnibase_core." - ) and not self.output_schema.startswith("omnibase_"): - # Allow external models but log a warning pattern - pass - + # Future: Add validation for ONEX naming patterns + # External models are currently allowed without warnings return self
403-458:validate_meta_modelperforms redundant checks on already-validated fields.Since
ModelContractMetais a Pydantic model withmin_length=1constraints and astrip_and_validate_non_emptyvalidator, these fields cannot be empty or falsy when the model is successfully instantiated. The checks formeta.name,meta.description,meta.input_schema, andmeta.output_schemaare redundant.The
node_idcheck is also effectively redundant since the model requires it (non-optional UUID field).def validate_meta_model(meta: ModelContractMeta) -> None: """ Validate a ModelContractMeta for cross-node consistency. This function performs additional validation beyond Pydantic's built-in validation to ensure the meta-model adheres to ONEX standards. Args: meta: The ModelContractMeta to validate Raises: ModelOnexError: If validation fails Example: >>> from uuid import uuid4 >>> meta = ModelContractMeta( ... node_id=uuid4(), ... node_kind=EnumNodeKind.COMPUTE, ... version="1.0.0", ... name="TestNode", ... description="A test node", ... input_schema="omnibase_core.models.ModelInput", ... output_schema="omnibase_core.models.ModelOutput", ... ) >>> validate_meta_model(meta) # No error raised """ - # Validate required fields are present and non-empty - if not meta.node_id: - raise ModelOnexError( - message="node_id is required and cannot be empty", - error_code=EnumCoreErrorCode.VALIDATION_ERROR, - ) - - if not meta.name: - raise ModelOnexError( - message="name is required and cannot be empty", - error_code=EnumCoreErrorCode.VALIDATION_ERROR, - ) - - if not meta.description: - raise ModelOnexError( - message="description is required and cannot be empty", - error_code=EnumCoreErrorCode.VALIDATION_ERROR, - ) - - if not meta.input_schema: - raise ModelOnexError( - message="input_schema is required and cannot be empty", - error_code=EnumCoreErrorCode.VALIDATION_ERROR, - ) - - if not meta.output_schema: - raise ModelOnexError( - message="output_schema is required and cannot be empty", - error_code=EnumCoreErrorCode.VALIDATION_ERROR, - ) - - # All validations passed + # Pydantic already validates required fields and non-empty constraints. + # This function is reserved for cross-node consistency checks that + # require runtime context beyond Pydantic's capabilities. + # Currently a no-op; extend as needed for future ONEX standards. + passtests/unit/contracts/test_node_meta_model.py (1)
24-26: Unused import:timezone.The import
timezonefromdatetimeis not used in this file. OnlyUTCis used (e.g.,datetime.now(UTC)).-from datetime import UTC, datetime, timezone +from datetime import UTC, datetimesrc/omnibase_core/models/contracts/model_contract_fingerprint.py (2)
162-169: Unreachable code aftersplit(":", 1).When using
split(":", 1), the result will always have exactly 1 or 2 parts (never more). Since you already check for:presence on line 154, this second check on line 164 is unreachable.# Split on first colon only (version may contain hyphens but not colons) parts = fingerprint_str.split(":", 1) - if len(parts) != 2: - raise ModelOnexError( - message=f"Invalid fingerprint format: '{fingerprint_str}'. Expected exactly one colon separator.", - error_code=EnumCoreErrorCode.VALIDATION_ERROR, - fingerprint=fingerprint_str, - ) - - version_str, hash_prefix = parts + version_str, hash_prefix = parts # Always 2 parts since ":" presence is already verified
183-202: Consider validating hash_prefix length infrom_string.The field constraint requires
min_length=8forhash_prefix, butfrom_stringaccepts any non-empty hex string. Pydantic will catch this during model construction, but an explicit length check could provide a clearer error message.# Validate hash prefix format if not hash_prefix or not all( c in "0123456789abcdef" for c in hash_prefix.lower() ): raise ModelOnexError( message=f"Invalid hash prefix in fingerprint: '{hash_prefix}'. Must be hexadecimal.", error_code=EnumCoreErrorCode.VALIDATION_ERROR, fingerprint=fingerprint_str, hash_prefix=hash_prefix, ) + + if len(hash_prefix) < 8: + raise ModelOnexError( + message=f"Hash prefix too short: '{hash_prefix}'. Must be at least 8 characters.", + error_code=EnumCoreErrorCode.VALIDATION_ERROR, + fingerprint=fingerprint_str, + hash_prefix=hash_prefix, + )tests/unit/contracts/test_hash_registry.py (3)
90-92: Missing@pytest.mark.timeouton other test classes.
TestNormalizationPipelinehas@pytest.mark.timeout(30)but other test classes don't. Consider applying consistently or removing if not needed.
181-189: Test assertion may not verify intended behavior.The test
test_normalize_with_custom_config_no_sortingonly assertsisinstance(result, str)but the comment notes thatjson.dumpsstill usessort_keys=True. This test doesn't actually verify that local canonical ordering is skipped. Consider clarifying the test's purpose or adjusting the assertion.def test_normalize_with_custom_config_no_sorting(self) -> None: """Test normalization without key sorting in local processing.""" contract = {"zebra": 1, "apple": 2} config = ModelContractNormalizationConfig(sort_keys=False) result = normalize_contract(contract, config) - # Note: json.dumps still uses sort_keys=True by default in normalize_contract - # This tests the local canonical ordering step - assert isinstance(result, str) + # Note: The normalize_contract implementation always uses sort_keys=True + # in json.dumps, so this config option only affects the local + # canonical ordering step before serialization. Keys will still be + # sorted in the final output. + parsed = json.loads(result) + assert "zebra" in parsed and "apple" in parsed
938-943: Consider using more specific exception type assertion.The tests use generic
Exceptionfor validation errors. Usingpytest.raises(ValidationError)from Pydantic would be more precise.+ from pydantic import ValidationError + # Invalid hash_length (too small) - with pytest.raises(Exception): + with pytest.raises(ValidationError): ModelContractNormalizationConfig(hash_length=4) # Invalid hash_length (too large) - with pytest.raises(Exception): + with pytest.raises(ValidationError): ModelContractNormalizationConfig(hash_length=100)tests/unit/nodes/test_node_purity.py (2)
23-28: sys.path manipulation for script imports is acceptable but consider alternatives.This pattern works but couples tests to filesystem layout. Consider making
check_node_purity.pya proper package module or using relative imports if the scripts directory becomes a package.
441-458: Test name and assertion may be misleading.The test
test_allows_concurrent_futures_in_node_compute_baseexpects no threading violations for a class namedNodeCompute, but the comment says "NodeCompute base class". If this is testing an exception for the base class specifically, the test should clarify this. Otherwise, the name should reflect testing a compute node, not specifically the base class.def test_allows_concurrent_futures_in_node_compute_base(self, analyze_source): - """Test that concurrent.futures is allowed in NodeCompute base class.""" + """Test that concurrent.futures is allowed in the NodeCompute base class definition. + + Note: This exemption is specifically for the base class, not derived compute nodes. + """ source = """ from concurrent.futures import ThreadPoolExecutor from omnibase_core.infrastructure.node_core_base import NodeCoreBase class NodeCompute(NodeCoreBase): pass """ analyzer = analyze_source(source) - # Should NOT have threading violation for NodeCompute base class + # NodeCompute base class is exempted from threading violations threading_violations = [ v for v in analyzer.violations if v.violation_type == ViolationType.THREADING_IMPORT ] assert len(threading_violations) == 0scripts/check_node_purity.py (1)
785-864: Consider extracting duplicated methods to a shared mixin.
_is_node_class,_determine_node_type, and_get_base_nameare duplicated verbatim betweenNodeTypeFinderandPurityAnalyzer. Extract these to a shared mixin class to reduce maintenance burden.class NodeClassMixin: """Shared methods for identifying ONEX node classes in AST.""" def _is_node_class(self, node: ast.ClassDef) -> bool: # ... shared implementation def _determine_node_type(self, node: ast.ClassDef) -> str | None: # ... shared implementation def _get_base_name(self, base: ast.expr) -> str | None: # ... shared implementation class NodeTypeFinder(NodeClassMixin, ast.NodeVisitor): ... @dataclass class PurityAnalyzer(NodeClassMixin, ast.NodeVisitor): ...src/omnibase_core/contracts/hash_registry.py (1)
256-259: Misleading variable name:hash_bytesis a hash object, not bytes.
hashlib.sha256()returns a hash object, not bytes. Consider renaming for clarity.# Compute SHA256 hash - hash_bytes = hashlib.sha256(normalized.encode("utf-8")) - full_hash = hash_bytes.hexdigest() + hash_obj = hashlib.sha256(normalized.encode("utf-8")) + full_hash = hash_obj.hexdigest() hash_prefix = full_hash[: config.hash_length]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (39)
contracts/runtime/runtime_orchestrator.yaml(2 hunks)docs/architecture/IMPORT_COMPATIBILITY_MATRIX.md(1 hunks)docs/architecture/NODE_PURITY_GUARANTEES.md(1 hunks)scripts/check_node_purity.py(1 hunks)scripts/validation/validate_naming.py(19 hunks)src/omnibase_core/contracts/__init__.py(1 hunks)src/omnibase_core/contracts/hash_registry.py(1 hunks)src/omnibase_core/models/contracts/__init__.py(4 hunks)src/omnibase_core/models/contracts/model_contract_fingerprint.py(1 hunks)src/omnibase_core/models/contracts/model_contract_meta.py(1 hunks)src/omnibase_core/models/contracts/model_contract_node_metadata.py(1 hunks)src/omnibase_core/models/contracts/model_contract_normalization_config.py(1 hunks)src/omnibase_core/models/contracts/model_drift_details.py(1 hunks)src/omnibase_core/models/contracts/model_drift_result.py(1 hunks)src/omnibase_core/models/contracts/model_node_extensions.py(1 hunks)tests/conftest.py(3 hunks)tests/integration/test_intent_publisher_integration.py(27 hunks)tests/unit/container/test_service_registry.py(18 hunks)tests/unit/container/test_service_registry_extended.py(21 hunks)tests/unit/contracts/test_hash_registry.py(1 hunks)tests/unit/contracts/test_model_contract_base.py(30 hunks)tests/unit/contracts/test_node_meta_model.py(1 hunks)tests/unit/mixins/test_mixin_caching.py(28 hunks)tests/unit/mixins/test_mixin_event_listener.py(36 hunks)tests/unit/mixins/test_mixin_fsm_execution.py(9 hunks)tests/unit/mixins/test_mixin_health_check.py(25 hunks)tests/unit/mixins/test_mixin_intent_publisher.py(6 hunks)tests/unit/mixins/test_mixin_introspection_publisher.py(27 hunks)tests/unit/mixins/test_mixin_metrics.py(29 hunks)tests/unit/mixins/test_mixin_service_registry.py(33 hunks)tests/unit/mixins/test_mixin_workflow_execution.py(17 hunks)tests/unit/models/core/test_model_result_factory.py(18 hunks)tests/unit/models/core/test_model_typed_accessor.py(31 hunks)tests/unit/models/security/test_model_secure_credentials.py(52 hunks)tests/unit/nodes/test_node_purity.py(1 hunks)tests/unit/utils/test_model_field_converter.py(7 hunks)tests/unit/utils/test_model_field_converter_registry.py(7 hunks)tests/unit/utils/test_safe_yaml_loader.py(3 hunks)tests/unit/validation/test_contracts_extended.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Always use Poetry for Python package management and task execution (poetry run pytest, poetry run mypy, poetry run python, etc.). Never use pip or python directly.
Use 100% strict mypy type checking compliance. All functions must have type annotations (disallow_untyped_defs = true), and avoid Any type returns (warn_return_any = true).
Always use ModelOnexError instead of generic Exception for error handling. Include error_code from EnumCoreErrorCode and optional context dictionary.
Format code using black and sort imports with isort. Run 'poetry run black src/ tests/' and 'poetry run isort src/ tests/' before committing.
Never share node instances (NodeCompute, NodeEffect, NodeReducer, NodeOrchestrator) across threads without explicit synchronization. Use thread-local instances or separate instances per thread.
Maintain minimum 60% code coverage in test runs. Run 'poetry run pytest tests/ --cov=src/omnibase_core --cov-report=term-missing' to verify coverage.
Lint code using ruff. Run 'poetry run ruff check src/ tests/' and use '--fix' flag for auto-fixable issues.
Files:
src/omnibase_core/models/contracts/model_contract_meta.pysrc/omnibase_core/models/contracts/model_drift_details.pytests/unit/mixins/test_mixin_service_registry.pytests/unit/validation/test_contracts_extended.pysrc/omnibase_core/models/contracts/model_contract_normalization_config.pytests/conftest.pytests/unit/mixins/test_mixin_workflow_execution.pytests/unit/utils/test_model_field_converter.pytests/unit/contracts/test_model_contract_base.pytests/unit/models/core/test_model_typed_accessor.pyscripts/check_node_purity.pysrc/omnibase_core/models/contracts/model_node_extensions.pytests/unit/mixins/test_mixin_event_listener.pysrc/omnibase_core/models/contracts/model_contract_fingerprint.pytests/unit/mixins/test_mixin_metrics.pytests/unit/contracts/test_hash_registry.pysrc/omnibase_core/contracts/__init__.pysrc/omnibase_core/models/contracts/__init__.pytests/unit/contracts/test_node_meta_model.pytests/unit/mixins/test_mixin_introspection_publisher.pytests/unit/models/security/test_model_secure_credentials.pytests/integration/test_intent_publisher_integration.pytests/unit/mixins/test_mixin_health_check.pytests/unit/mixins/test_mixin_fsm_execution.pytests/unit/utils/test_safe_yaml_loader.pytests/unit/container/test_service_registry.pytests/unit/nodes/test_node_purity.pysrc/omnibase_core/models/contracts/model_contract_node_metadata.pytests/unit/container/test_service_registry_extended.pysrc/omnibase_core/contracts/hash_registry.pytests/unit/mixins/test_mixin_caching.pytests/unit/utils/test_model_field_converter_registry.pysrc/omnibase_core/models/contracts/model_drift_result.pytests/unit/mixins/test_mixin_intent_publisher.pyscripts/validation/validate_naming.pytests/unit/models/core/test_model_result_factory.py
src/omnibase_core/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/omnibase_core/**/*.py: Use protocol-based dependency injection by calling container.get_service('ProtocolName'), never use isinstance checks or concrete class names for DI resolution.
Use @standard_error_handling decorator from omnibase_core.decorators.error_handling to eliminate 6+ lines of try/catch boilerplate in methods.
Use ModelEventEnvelope (from omnibase_core.models.event.model_event_envelope) for inter-service event-driven communication instead of direct method calls.
Use available mixins (MixinDiscoveryResponder, MixinEventHandler, MixinEventListener, MixinNodeExecutor, MixinNodeLifecycle, MixinRequestResponseIntrospection, MixinWorkflowSupport) to add cross-cutting concerns to node classes.
Files:
src/omnibase_core/models/contracts/model_contract_meta.pysrc/omnibase_core/models/contracts/model_drift_details.pysrc/omnibase_core/models/contracts/model_contract_normalization_config.pysrc/omnibase_core/models/contracts/model_node_extensions.pysrc/omnibase_core/models/contracts/model_contract_fingerprint.pysrc/omnibase_core/contracts/__init__.pysrc/omnibase_core/models/contracts/__init__.pysrc/omnibase_core/models/contracts/model_contract_node_metadata.pysrc/omnibase_core/contracts/hash_registry.pysrc/omnibase_core/models/contracts/model_drift_result.py
tests/unit/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Unit tests must follow the naming convention of test_ prefix for functions and match the source module structure. Organize tests in tests/unit/ mirroring src/omnibase_core/ structure.
Files:
tests/unit/mixins/test_mixin_service_registry.pytests/unit/validation/test_contracts_extended.pytests/unit/mixins/test_mixin_workflow_execution.pytests/unit/utils/test_model_field_converter.pytests/unit/contracts/test_model_contract_base.pytests/unit/models/core/test_model_typed_accessor.pytests/unit/mixins/test_mixin_event_listener.pytests/unit/mixins/test_mixin_metrics.pytests/unit/contracts/test_hash_registry.pytests/unit/contracts/test_node_meta_model.pytests/unit/mixins/test_mixin_introspection_publisher.pytests/unit/models/security/test_model_secure_credentials.pytests/unit/mixins/test_mixin_health_check.pytests/unit/mixins/test_mixin_fsm_execution.pytests/unit/utils/test_safe_yaml_loader.pytests/unit/container/test_service_registry.pytests/unit/nodes/test_node_purity.pytests/unit/container/test_service_registry_extended.pytests/unit/mixins/test_mixin_caching.pytests/unit/utils/test_model_field_converter_registry.pytests/unit/mixins/test_mixin_intent_publisher.pytests/unit/models/core/test_model_result_factory.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Run tests with pytest using 'poetry run pytest tests/' with default 4 parallel workers using pytest-xdist. Use markers: @pytest.mark.unit, @pytest.mark.integration, @pytest.mark.slow, @pytest.mark.smoke, @pytest.mark.performance.
Files:
tests/unit/mixins/test_mixin_service_registry.pytests/unit/validation/test_contracts_extended.pytests/conftest.pytests/unit/mixins/test_mixin_workflow_execution.pytests/unit/utils/test_model_field_converter.pytests/unit/contracts/test_model_contract_base.pytests/unit/models/core/test_model_typed_accessor.pytests/unit/mixins/test_mixin_event_listener.pytests/unit/mixins/test_mixin_metrics.pytests/unit/contracts/test_hash_registry.pytests/unit/contracts/test_node_meta_model.pytests/unit/mixins/test_mixin_introspection_publisher.pytests/unit/models/security/test_model_secure_credentials.pytests/integration/test_intent_publisher_integration.pytests/unit/mixins/test_mixin_health_check.pytests/unit/mixins/test_mixin_fsm_execution.pytests/unit/utils/test_safe_yaml_loader.pytests/unit/container/test_service_registry.pytests/unit/nodes/test_node_purity.pytests/unit/container/test_service_registry_extended.pytests/unit/mixins/test_mixin_caching.pytests/unit/utils/test_model_field_converter_registry.pytests/unit/mixins/test_mixin_intent_publisher.pytests/unit/models/core/test_model_result_factory.py
🧠 Learnings (11)
📓 Common learnings
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-09T04:39:59.692Z
Learning: The omnibase_core project provides foundational ONEX framework components: 4-Node Architecture (EFFECT→COMPUTE→REDUCER→ORCHESTRATOR), Protocol-Driven DI with container.get_service(), zero boilerplate via base classes, structured error handling with ModelOnexError, event-driven communication via ModelEventEnvelope, and 12,000+ tests with 60%+ coverage.
📚 Learning: 2025-12-09T04:39:59.691Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-09T04:39:59.691Z
Learning: Applies to src/omnibase_core/models/container/**/*.py : Use ModelONEXContainer (from omnibase_core.models.container.model_onex_container) for dependency injection in node constructors. Use ModelContainer[T] (from omnibase_core.models.core.model_container) only for wrapping single values with metadata, never in node constructors.
Applied to files:
src/omnibase_core/models/contracts/model_contract_meta.pydocs/architecture/NODE_PURITY_GUARANTEES.mdsrc/omnibase_core/models/contracts/model_node_extensions.pysrc/omnibase_core/contracts/__init__.pydocs/architecture/IMPORT_COMPATIBILITY_MATRIX.mdsrc/omnibase_core/models/contracts/__init__.pytests/unit/contracts/test_node_meta_model.pysrc/omnibase_core/models/contracts/model_contract_node_metadata.py
📚 Learning: 2025-12-09T04:39:59.691Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-09T04:39:59.691Z
Learning: Applies to src/omnibase_core/infrastructure/**/*.py : All node classes must call super().__init__(container) in their __init__ method with ModelONEXContainer. This eliminates 80+ lines of boilerplate initialization code.
Applied to files:
src/omnibase_core/models/contracts/model_contract_meta.pysrc/omnibase_core/models/contracts/model_node_extensions.pysrc/omnibase_core/contracts/__init__.pydocs/architecture/IMPORT_COMPATIBILITY_MATRIX.mdsrc/omnibase_core/models/contracts/model_contract_node_metadata.py
📚 Learning: 2025-12-09T04:39:59.692Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-09T04:39:59.692Z
Learning: The omnibase_core project provides foundational ONEX framework components: 4-Node Architecture (EFFECT→COMPUTE→REDUCER→ORCHESTRATOR), Protocol-Driven DI with container.get_service(), zero boilerplate via base classes, structured error handling with ModelOnexError, event-driven communication via ModelEventEnvelope, and 12,000+ tests with 60%+ coverage.
Applied to files:
docs/architecture/NODE_PURITY_GUARANTEES.mdsrc/omnibase_core/contracts/__init__.pydocs/architecture/IMPORT_COMPATIBILITY_MATRIX.mdsrc/omnibase_core/models/contracts/__init__.py
📚 Learning: 2025-12-09T04:39:59.691Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-09T04:39:59.691Z
Learning: Applies to src/omnibase_core/nodes/**/*.py : Import and use the four primary node types: NodeCompute (data processing), NodeEffect (external I/O), NodeReducer (FSM-driven state), and NodeOrchestrator (workflow-driven coordination) from omnibase_core.nodes.
Applied to files:
docs/architecture/NODE_PURITY_GUARANTEES.mdtests/unit/mixins/test_mixin_workflow_execution.pydocs/architecture/IMPORT_COMPATIBILITY_MATRIX.mdsrc/omnibase_core/models/contracts/__init__.py
📚 Learning: 2025-12-09T04:39:59.692Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-09T04:39:59.692Z
Learning: Applies to **/*.py : Never share node instances (NodeCompute, NodeEffect, NodeReducer, NodeOrchestrator) across threads without explicit synchronization. Use thread-local instances or separate instances per thread.
Applied to files:
docs/architecture/NODE_PURITY_GUARANTEES.md
📚 Learning: 2025-12-09T04:39:59.692Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-09T04:39:59.692Z
Learning: Applies to src/omnibase_core/**/*.py : Use available mixins (MixinDiscoveryResponder, MixinEventHandler, MixinEventListener, MixinNodeExecutor, MixinNodeLifecycle, MixinRequestResponseIntrospection, MixinWorkflowSupport) to add cross-cutting concerns to node classes.
Applied to files:
tests/unit/mixins/test_mixin_workflow_execution.pytests/unit/mixins/test_mixin_metrics.pytests/unit/mixins/test_mixin_introspection_publisher.pytests/unit/mixins/test_mixin_health_check.pytests/unit/mixins/test_mixin_caching.py
📚 Learning: 2025-12-09T04:39:59.692Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-09T04:39:59.692Z
Learning: Applies to src/omnibase_core/**/*.py : Use ModelEventEnvelope (from omnibase_core.models.event.model_event_envelope) for inter-service event-driven communication instead of direct method calls.
Applied to files:
tests/unit/mixins/test_mixin_event_listener.py
📚 Learning: 2025-12-09T04:39:59.692Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-09T04:39:59.692Z
Learning: No backwards compatibility is guaranteed until v0.4.0 release. Breaking changes may occur between commits, which is acceptable during the pre-release development phase.
Applied to files:
docs/architecture/IMPORT_COMPATIBILITY_MATRIX.md
📚 Learning: 2025-12-09T04:39:59.692Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-09T04:39:59.692Z
Learning: Use NodeReducer for FSM-driven state management with ModelIntent for pure state transitions. Use NodeOrchestrator for workflow-driven coordination with ModelAction for lease-based single-writer semantics and distributed resource locking.
Applied to files:
tests/integration/test_intent_publisher_integration.py
📚 Learning: 2025-12-09T04:39:59.691Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-09T04:39:59.691Z
Learning: Applies to src/omnibase_core/**/*.py : Use protocol-based dependency injection by calling container.get_service('ProtocolName'), never use isinstance checks or concrete class names for DI resolution.
Applied to files:
tests/unit/container/test_service_registry.pytests/unit/container/test_service_registry_extended.py
🧬 Code graph analysis (21)
tests/unit/mixins/test_mixin_service_registry.py (1)
src/omnibase_core/models/core/model_group_manifest.py (1)
get_tool_by_name(174-179)
tests/unit/mixins/test_mixin_workflow_execution.py (1)
src/omnibase_core/mixins/mixin_workflow_execution.py (1)
MixinWorkflowExecution(26-184)
tests/unit/contracts/test_model_contract_base.py (3)
src/omnibase_core/mixins/mixin_node_setup.py (1)
contract(61-64)src/omnibase_core/models/security/model_security_policy_data.py (1)
data(27-34)src/omnibase_core/enums/enum_node_type.py (1)
EnumNodeType(96-320)
tests/unit/models/core/test_model_typed_accessor.py (1)
src/omnibase_core/models/core/model_typed_accessor.py (1)
ModelTypedAccessor(20-115)
tests/unit/mixins/test_mixin_event_listener.py (1)
src/omnibase_core/mixins/mixin_event_listener.py (1)
MixinEventListener(34-1070)
tests/unit/mixins/test_mixin_metrics.py (1)
src/omnibase_core/mixins/mixin_metrics.py (1)
MixinMetrics(17-123)
src/omnibase_core/contracts/__init__.py (8)
src/omnibase_core/contracts/hash_registry.py (3)
ContractHashRegistry(269-550)compute_contract_fingerprint(211-266)normalize_contract(152-208)src/omnibase_core/models/contracts/model_contract_fingerprint.py (1)
ModelContractFingerprint(37-219)src/omnibase_core/models/contracts/model_contract_meta.py (3)
ModelContractMeta(43-400)is_valid_meta_model(463-494)validate_meta_model(403-460)src/omnibase_core/models/contracts/model_contract_node_metadata.py (1)
ModelContractNodeMetadata(40-95)src/omnibase_core/models/contracts/model_contract_normalization_config.py (1)
ModelContractNormalizationConfig(31-86)src/omnibase_core/models/contracts/model_drift_details.py (1)
ModelDriftDetails(33-98)src/omnibase_core/models/contracts/model_drift_result.py (1)
ModelDriftResult(42-106)src/omnibase_core/models/contracts/model_node_extensions.py (1)
ModelNodeExtensions(39-76)
src/omnibase_core/models/contracts/__init__.py (7)
src/omnibase_core/models/contracts/model_contract_fingerprint.py (1)
ModelContractFingerprint(37-219)src/omnibase_core/models/contracts/model_contract_meta.py (1)
ModelContractMeta(43-400)src/omnibase_core/models/contracts/model_contract_node_metadata.py (1)
ModelContractNodeMetadata(40-95)src/omnibase_core/models/contracts/model_contract_normalization_config.py (1)
ModelContractNormalizationConfig(31-86)src/omnibase_core/models/contracts/model_drift_details.py (1)
ModelDriftDetails(33-98)src/omnibase_core/models/contracts/model_drift_result.py (1)
ModelDriftResult(42-106)src/omnibase_core/models/contracts/model_node_extensions.py (1)
ModelNodeExtensions(39-76)
tests/unit/contracts/test_node_meta_model.py (5)
src/omnibase_core/enums/enum_node_kind.py (1)
EnumNodeKind(13-118)src/omnibase_core/models/contracts/model_contract_meta.py (3)
ModelContractMeta(43-400)validate_meta_model(403-460)is_valid_meta_model(463-494)src/omnibase_core/models/contracts/model_contract_version.py (1)
ModelContractVersion(21-281)src/omnibase_core/models/contracts/model_node_extensions.py (1)
ModelNodeExtensions(39-76)src/omnibase_core/models/contracts/model_contract_node_metadata.py (1)
ModelContractNodeMetadata(40-95)
tests/unit/mixins/test_mixin_introspection_publisher.py (1)
src/omnibase_core/mixins/mixin_introspection_publisher.py (2)
MixinIntrospectionPublisher(32-393)_extract_node_actions(258-295)
tests/integration/test_intent_publisher_integration.py (1)
src/omnibase_core/mixins/mixin_intent_publisher.py (1)
MixinIntentPublisher(56-255)
tests/unit/mixins/test_mixin_health_check.py (5)
tests/unit/mixins/test_mixin_caching.py (1)
MockNode(12-13)tests/unit/mixins/test_mixin_event_listener.py (1)
MockNode(40-51)tests/unit/mixins/test_mixin_fsm_execution.py (1)
MockNode(81-86)tests/unit/mixins/test_mixin_introspection_publisher.py (1)
MockNode(28-38)tests/unit/mixins/test_mixin_metrics.py (1)
MockNode(10-11)
tests/unit/mixins/test_mixin_fsm_execution.py (7)
tests/unit/mixins/test_mixin_caching.py (1)
MockNode(12-13)tests/unit/mixins/test_mixin_event_listener.py (1)
MockNode(40-51)tests/unit/mixins/test_mixin_health_check.py (1)
MockNode(16-25)tests/unit/mixins/test_mixin_introspection_publisher.py (1)
MockNode(28-38)tests/unit/mixins/test_mixin_metrics.py (1)
MockNode(10-11)src/omnibase_core/mixins/mixin_fsm_execution.py (2)
MixinFSMExecution(23-236)validate_fsm_contract(117-141)src/omnibase_core/utils/fsm_executor.py (1)
validate_fsm_contract(214-306)
tests/unit/utils/test_safe_yaml_loader.py (1)
src/omnibase_core/utils/util_safe_yaml_loader.py (1)
load_and_validate_yaml_model(33-116)
tests/unit/container/test_service_registry.py (1)
tests/unit/container/test_service_registry_extended.py (3)
MockServiceImplementation(23-34)ITestService(15-19)registry(60-63)
tests/unit/nodes/test_node_purity.py (1)
scripts/check_node_purity.py (6)
PurityAnalyzer(478-1028)PurityCheckResult(110-118)Severity(75-79)ViolationType(59-72)analyze_file(1036-1121)NodeTypeFinder(362-474)
src/omnibase_core/contracts/hash_registry.py (6)
src/omnibase_core/enums/enum_core_error_code.py (1)
EnumCoreErrorCode(9-154)src/omnibase_core/models/contracts/model_contract_fingerprint.py (3)
ModelContractFingerprint(37-219)from_string(142-203)matches(205-219)src/omnibase_core/models/contracts/model_contract_normalization_config.py (1)
ModelContractNormalizationConfig(31-86)src/omnibase_core/models/contracts/model_contract_version.py (1)
ModelContractVersion(21-281)src/omnibase_core/models/contracts/model_drift_details.py (1)
ModelDriftDetails(33-98)src/omnibase_core/models/contracts/model_drift_result.py (1)
ModelDriftResult(42-106)
tests/unit/mixins/test_mixin_caching.py (1)
src/omnibase_core/mixins/mixin_caching.py (1)
MixinCaching(28-122)
tests/unit/utils/test_model_field_converter_registry.py (2)
tests/unit/utils/test_model_field_converter.py (1)
SampleStatus(31-36)src/omnibase_core/enums/enum_core_error_code.py (1)
EnumCoreErrorCode(9-154)
src/omnibase_core/models/contracts/model_drift_result.py (2)
src/omnibase_core/models/contracts/model_contract_fingerprint.py (1)
ModelContractFingerprint(37-219)src/omnibase_core/models/contracts/model_drift_details.py (1)
ModelDriftDetails(33-98)
tests/unit/models/core/test_model_result_factory.py (1)
src/omnibase_core/models/core/model_result_factory.py (1)
ModelResultFactory(22-88)
🪛 LanguageTool
docs/architecture/NODE_PURITY_GUARANTEES.md
[grammar] ~47-~47: Ensure spelling is correct
Context: ...k calls are side effects that can fail, timeout, or return different results. **Altern...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[uncategorized] ~229-~229: The official name of this software platform is spelled with a capital “H”.
Context: ... not found) ### CI Integration Add to .github/workflows/test.yml: ```yaml - name: C...
(GITHUB)
🪛 markdownlint-cli2 (0.18.1)
docs/architecture/IMPORT_COMPATIBILITY_MATRIX.md
605-605: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Smoke Tests
- GitHub Check: claude-review
| if registered is None: | ||
| return ModelDriftResult( | ||
| contract_name=contract_name, | ||
| has_drift=True, | ||
| expected_fingerprint=None, | ||
| computed_fingerprint=computed, | ||
| drift_type="not_registered", | ||
| details=ModelDriftDetails(reason="Contract not found in registry"), | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find src -name "*.py" -type f | xargs grep -l "ModelDriftResult" | head -20Repository: OmniNode-ai/omnibase_core
Length of output: 315
🏁 Script executed:
rg -n "drift_type" src/omnibase_core/models/contracts/model_drift_result.py -A 2 -B 2Repository: OmniNode-ai/omnibase_core
Length of output: 1048
🏁 Script executed:
rg -n "class ModelDriftResult" src/omnibase_core/models/contracts/ -A 20Repository: OmniNode-ai/omnibase_core
Length of output: 2333
🏁 Script executed:
cat -n src/omnibase_core/models/contracts/model_drift_result.py | sed -n '85,105p'Repository: OmniNode-ai/omnibase_core
Length of output: 923
🏁 Script executed:
rg -n "drift_type=" src/omnibase_core/contracts/hash_registry.pyRepository: OmniNode-ai/omnibase_core
Length of output: 193
🏁 Script executed:
rg -n "@field_validator\|@validator" src/omnibase_core/models/contracts/model_drift_result.pyRepository: OmniNode-ai/omnibase_core
Length of output: 51
🏁 Script executed:
cat -n src/omnibase_core/contracts/hash_registry.py | sed -n '420,450p'Repository: OmniNode-ai/omnibase_core
Length of output: 1381
drift_type="not_registered" is not a documented drift type value.
The drift_type field in ModelDriftResult is typed as str | None and documented to accept only 'version', 'content', 'both', or None. Using "not_registered" violates this documented constraint and may cause issues in downstream code expecting only these four values.
For an unregistered contract (no baseline to compare), use drift_type=None instead:
if registered is None:
return ModelDriftResult(
contract_name=contract_name,
has_drift=True,
expected_fingerprint=None,
computed_fingerprint=computed,
- drift_type="not_registered",
+ drift_type=None,
details=ModelDriftDetails(reason="Contract not found in registry"),
)🤖 Prompt for AI Agents
In src/omnibase_core/contracts/hash_registry.py around lines 427 to 435, the
code sets drift_type="not_registered" for contracts missing from the registry
which violates the documented/typed allowed values ('version','content','both',
or None); change drift_type to None for the "no baseline" case so the
ModelDriftResult uses a permitted value, leaving
expected_fingerprint/computed_fingerprint and details as-is.
MAJOR fixes:
- Fix read-mode open() incorrectly flagged as violation in check_node_purity.py
- Add DriftType literal type to constrain drift_type values
- Add @pytest.mark.timeout decorators to 22 test classes
- Handle relative_to() failure gracefully with _safe_relative_path()
- Import ModelDriftDetails directly from definition module
MINOR fixes:
- Make test assertions specific for effect/orchestrator skip behavior
- Document cross-node consistency validator as intentionally permissive
- Remove redundant validate_meta_model field presence checks
- Use specific ModelOnexError exception type assertions
NITPICK fixes:
- Remove unreachable code after split(":", 1)
- Add hash_prefix length validation (8-64 chars)
- Rename misleading hash_bytes variable to hasher
- Document sys.path manipulation in scripts
- Add python language specifier to 16 code blocks in THREADING.md
- Enhance normalization config documentation with defaults rationale
- Add thread safety documentation to contracts module
Code Review - PR #143: Phase 0 Stabilization + Test FixesSummaryThis PR implements critical Phase 0 stabilization infrastructure with contract fingerprinting, hash registry, node purity validation, and comprehensive test fixes. The scope is large (8,641 additions / 710 deletions across 45 files) but well-organized and follows ONEX architectural patterns. ✅ Strengths1. Excellent Architecture & Design
2. Comprehensive Testing
3. Deprecated API Fixes
4. Documentation Quality
5. Naming Convention Compliance
|
…MN-151] Remove loose dict[str, object] type alias in favor of strongly-typed Pydantic models for contract fingerprinting functions. Changes: - Remove ContractData type alias from hash_registry.py - Update normalize_contract() to accept BaseModel - Update compute_contract_fingerprint() to accept BaseModel - Update register_from_contract() to accept BaseModel - Update detect_drift_from_contract() to accept BaseModel - Remove ContractData from __init__.py exports - Update docstrings to reflect Pydantic model usage - Add ModelTestContract with field_validator for version string conversion - Convert all 85 tests to use Pydantic models instead of raw dicts This enforces ONEX zero-tolerance policy for loose typing.
Code Review - PR #143: Phase 0 Stabilization + Test FixesOverviewThis PR introduces Phase 0 stabilization infrastructure with contract fingerprinting, hash registry, and comprehensive test improvements. The changes are well-structured and align with ONEX architectural patterns. ✅ Strengths1. Excellent Code Quality & Documentation
2. Robust Contract Fingerprinting Implementation# hash_registry.py - Clean separation of concerns
def normalize_contract() # Step 1: Canonical normalization
def compute_contract_fingerprint() # Step 2: SHA256 hashing
class ContractHashRegistry # Step 3: Registry + drift detectionStrong points:
3. Exceptional Test Coverage
4. Proper Architectural Exemptions
5. Test Fixes - datetime.utcnow() Deprecation
🔍 Areas for Consideration1. Thread Safety - Potential Race Condition
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
tests/unit/nodes/test_declarative_nodes.py (1)
299-299: Timeout marker added for test safety.The 60-second timeout provides a safety net for async workflow operations without affecting test behavior for normally-executing tests.
src/omnibase_core/contracts/__init__.py (1)
1-120: Contracts public API re-exports are coherent and address earlier ModelDriftDetails import feedback.The
contractspackage now cleanly surfaces the hash registry, fingerprinting utilities, meta-models, drift types/details/results, and normalization config from their defining modules, andModelDriftDetailsis imported directly frommodel_drift_detailsas previously suggested. This makes the external API predictable without adding new coupling.src/omnibase_core/contracts/hash_registry.py (1)
442-450:drift_type="not_registered"violates the documentedDriftTypeconstraint.The
DriftTypetype alias only allows'version','content','both', orNone. Using"not_registered"will cause type checking failures and may break downstream code expecting only these four values.For an unregistered contract (no baseline to compare), use
drift_type=Nonesince there's no actual "drift" to characterize—the contract simply doesn't exist in the registry.if registered is None: return ModelDriftResult( contract_name=contract_name, has_drift=True, expected_fingerprint=None, computed_fingerprint=computed, - drift_type="not_registered", + drift_type=None, details=ModelDriftDetails(reason="Contract not found in registry"), )
🧹 Nitpick comments (5)
tests/unit/nodes/test_retry_count_tracking.py (1)
107-131: Switch to expectingModelOnexErrorcorrectly reflects the error-wrapping contractUpdating the test to assert
ModelOnexErrorwithmatch="Always fails"aligns this suite with the project’s error-handling policy (no bareException) and verifies that the retry machinery surfaces the original failure message through the wrapper. IfModelOnexErroralso carries anerror_codeor structured context for these failures, consider extending the assertion to validate those fields as well for tighter coverage.tests/unit/nodes/test_declarative_nodes.py (1)
139-465: Consider differentiating timeout values for unit vs integration tests.All three test classes use the same 60-second timeout. You might consider using shorter timeouts (e.g., 10-30 seconds) for the unit test classes (TestNodeReducer, TestNodeOrchestrator) to catch performance regressions earlier, while keeping the 60-second timeout for the integration test class (TestDeclarativeNodesIntegration).
Example differentiation:
@pytest.mark.unit -@pytest.mark.timeout(60) +@pytest.mark.timeout(30) class TestNodeReducer:@pytest.mark.unit -@pytest.mark.timeout(60) +@pytest.mark.timeout(30) class TestNodeOrchestrator:@pytest.mark.unit @pytest.mark.timeout(60) class TestDeclarativeNodesIntegration:src/omnibase_core/models/contracts/model_contract_meta.py (1)
397-482: Meta-model helper validators are intentionally permissive; consider clarifying or extending behavior.
validate_meta_model()currently only collectsnon_onex_schemasand doesn’t use it, sois_valid_meta_model()will always returnTruefor any already-constructedModelContractMeta. If these helpers are meant purely as forward-compatible hooks, you might either (a) document that they’re intentionally no-op today, or (b) add an optionalstrictflag to start surfacing naming issues now (e.g., by returning the list or raising in strict mode).tests/unit/contracts/test_hash_registry.py (1)
1642-1680: Consider usingtracemallocfor more accurate memory measurements.The memory test uses
sys.getsizeofwhich doesn't account for referenced objects. While this is acknowledged in the comment, consider usingtracemallocfor more accurate total memory usage if precise measurements become important.# Alternative approach using tracemalloc for total memory: import tracemalloc tracemalloc.start() fingerprint = compute_contract_fingerprint(...) current, peak = tracemalloc.get_traced_memory() tracemalloc.stop()tests/unit/nodes/test_node_purity.py (1)
19-19: Remove unusedTYPE_CHECKINGimport.The
TYPE_CHECKINGimport fromtypingis not used anywhere in this file.-from typing import TYPE_CHECKING
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
docs/guides/THREADING.md(16 hunks)scripts/check_node_purity.py(1 hunks)scripts/validation/validate_optimization.py(1 hunks)src/omnibase_core/contracts/__init__.py(1 hunks)src/omnibase_core/contracts/hash_registry.py(1 hunks)src/omnibase_core/models/contracts/model_contract_fingerprint.py(1 hunks)src/omnibase_core/models/contracts/model_contract_meta.py(1 hunks)src/omnibase_core/models/contracts/model_contract_normalization_config.py(1 hunks)src/omnibase_core/models/contracts/model_drift_result.py(1 hunks)tests/comparison_schema_value_runtime_validation.py(1 hunks)tests/unit/contracts/test_hash_registry.py(1 hunks)tests/unit/contracts/test_node_meta_model.py(1 hunks)tests/unit/nodes/test_declarative_nodes.py(3 hunks)tests/unit/nodes/test_node_purity.py(1 hunks)tests/unit/nodes/test_retry_count_tracking.py(2 hunks)tests/unit/nodes/test_rollback_failure_handling.py(2 hunks)tests/validation/test_check_error_raising.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/omnibase_core/models/contracts/model_contract_normalization_config.py
- tests/unit/contracts/test_node_meta_model.py
🧰 Additional context used
📓 Path-based instructions (4)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Always use Poetry for Python package management and task execution (poetry run pytest, poetry run mypy, poetry run python, etc.). Never use pip or python directly.
Use 100% strict mypy type checking compliance. All functions must have type annotations (disallow_untyped_defs = true), and avoid Any type returns (warn_return_any = true).
Always use ModelOnexError instead of generic Exception for error handling. Include error_code from EnumCoreErrorCode and optional context dictionary.
Format code using black and sort imports with isort. Run 'poetry run black src/ tests/' and 'poetry run isort src/ tests/' before committing.
Never share node instances (NodeCompute, NodeEffect, NodeReducer, NodeOrchestrator) across threads without explicit synchronization. Use thread-local instances or separate instances per thread.
Maintain minimum 60% code coverage in test runs. Run 'poetry run pytest tests/ --cov=src/omnibase_core --cov-report=term-missing' to verify coverage.
Lint code using ruff. Run 'poetry run ruff check src/ tests/' and use '--fix' flag for auto-fixable issues.
Files:
scripts/validation/validate_optimization.pytests/unit/nodes/test_declarative_nodes.pysrc/omnibase_core/models/contracts/model_contract_meta.pytests/unit/nodes/test_retry_count_tracking.pysrc/omnibase_core/models/contracts/model_drift_result.pytests/validation/test_check_error_raising.pytests/unit/nodes/test_rollback_failure_handling.pytests/unit/contracts/test_hash_registry.pysrc/omnibase_core/models/contracts/model_contract_fingerprint.pytests/unit/nodes/test_node_purity.pysrc/omnibase_core/contracts/hash_registry.pyscripts/check_node_purity.pysrc/omnibase_core/contracts/__init__.pytests/comparison_schema_value_runtime_validation.py
tests/unit/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Unit tests must follow the naming convention of test_ prefix for functions and match the source module structure. Organize tests in tests/unit/ mirroring src/omnibase_core/ structure.
Files:
tests/unit/nodes/test_declarative_nodes.pytests/unit/nodes/test_retry_count_tracking.pytests/unit/nodes/test_rollback_failure_handling.pytests/unit/contracts/test_hash_registry.pytests/unit/nodes/test_node_purity.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Run tests with pytest using 'poetry run pytest tests/' with default 4 parallel workers using pytest-xdist. Use markers: @pytest.mark.unit, @pytest.mark.integration, @pytest.mark.slow, @pytest.mark.smoke, @pytest.mark.performance.
Files:
tests/unit/nodes/test_declarative_nodes.pytests/unit/nodes/test_retry_count_tracking.pytests/validation/test_check_error_raising.pytests/unit/nodes/test_rollback_failure_handling.pytests/unit/contracts/test_hash_registry.pytests/unit/nodes/test_node_purity.pytests/comparison_schema_value_runtime_validation.py
src/omnibase_core/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/omnibase_core/**/*.py: Use protocol-based dependency injection by calling container.get_service('ProtocolName'), never use isinstance checks or concrete class names for DI resolution.
Use @standard_error_handling decorator from omnibase_core.decorators.error_handling to eliminate 6+ lines of try/catch boilerplate in methods.
Use ModelEventEnvelope (from omnibase_core.models.event.model_event_envelope) for inter-service event-driven communication instead of direct method calls.
Use available mixins (MixinDiscoveryResponder, MixinEventHandler, MixinEventListener, MixinNodeExecutor, MixinNodeLifecycle, MixinRequestResponseIntrospection, MixinWorkflowSupport) to add cross-cutting concerns to node classes.
Files:
src/omnibase_core/models/contracts/model_contract_meta.pysrc/omnibase_core/models/contracts/model_drift_result.pysrc/omnibase_core/models/contracts/model_contract_fingerprint.pysrc/omnibase_core/contracts/hash_registry.pysrc/omnibase_core/contracts/__init__.py
🧠 Learnings (7)
📚 Learning: 2025-12-09T04:39:59.691Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-09T04:39:59.691Z
Learning: Applies to src/omnibase_core/models/container/**/*.py : Use ModelONEXContainer (from omnibase_core.models.container.model_onex_container) for dependency injection in node constructors. Use ModelContainer[T] (from omnibase_core.models.core.model_container) only for wrapping single values with metadata, never in node constructors.
Applied to files:
src/omnibase_core/models/contracts/model_contract_meta.pydocs/guides/THREADING.md
📚 Learning: 2025-12-09T04:39:59.691Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-09T04:39:59.691Z
Learning: Applies to src/omnibase_core/infrastructure/**/*.py : All node classes must call super().__init__(container) in their __init__ method with ModelONEXContainer. This eliminates 80+ lines of boilerplate initialization code.
Applied to files:
src/omnibase_core/models/contracts/model_contract_meta.py
📚 Learning: 2025-12-09T04:39:59.692Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-09T04:39:59.692Z
Learning: Applies to src/omnibase_core/**/*.py : Use available mixins (MixinDiscoveryResponder, MixinEventHandler, MixinEventListener, MixinNodeExecutor, MixinNodeLifecycle, MixinRequestResponseIntrospection, MixinWorkflowSupport) to add cross-cutting concerns to node classes.
Applied to files:
tests/unit/nodes/test_node_purity.py
📚 Learning: 2025-12-09T04:39:59.692Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-09T04:39:59.692Z
Learning: The omnibase_core project provides foundational ONEX framework components: 4-Node Architecture (EFFECT→COMPUTE→REDUCER→ORCHESTRATOR), Protocol-Driven DI with container.get_service(), zero boilerplate via base classes, structured error handling with ModelOnexError, event-driven communication via ModelEventEnvelope, and 12,000+ tests with 60%+ coverage.
Applied to files:
src/omnibase_core/contracts/__init__.py
📚 Learning: 2025-12-09T04:39:59.692Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-09T04:39:59.692Z
Learning: Applies to **/*.py : Never share node instances (NodeCompute, NodeEffect, NodeReducer, NodeOrchestrator) across threads without explicit synchronization. Use thread-local instances or separate instances per thread.
Applied to files:
docs/guides/THREADING.md
📚 Learning: 2025-12-09T04:39:59.691Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-09T04:39:59.691Z
Learning: Applies to src/omnibase_core/nodes/**/*.py : Import and use the four primary node types: NodeCompute (data processing), NodeEffect (external I/O), NodeReducer (FSM-driven state), and NodeOrchestrator (workflow-driven coordination) from omnibase_core.nodes.
Applied to files:
docs/guides/THREADING.md
📚 Learning: 2025-12-09T04:39:59.691Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-09T04:39:59.691Z
Learning: Applies to src/omnibase_core/**/*.py : Use protocol-based dependency injection by calling container.get_service('ProtocolName'), never use isinstance checks or concrete class names for DI resolution.
Applied to files:
docs/guides/THREADING.md
🧬 Code graph analysis (7)
src/omnibase_core/models/contracts/model_contract_meta.py (6)
src/omnibase_core/enums/enum_core_error_code.py (1)
EnumCoreErrorCode(9-154)src/omnibase_core/enums/enum_node_kind.py (1)
EnumNodeKind(13-118)src/omnibase_core/models/contracts/model_contract_node_metadata.py (1)
ModelContractNodeMetadata(40-95)src/omnibase_core/models/contracts/model_contract_version.py (1)
ModelContractVersion(21-281)src/omnibase_core/models/contracts/model_node_extensions.py (1)
ModelNodeExtensions(39-76)src/omnibase_core/models/primitives/model_semver.py (1)
ModelSemVer(24-143)
tests/unit/nodes/test_rollback_failure_handling.py (1)
src/omnibase_core/models/infrastructure/model_time_based.py (1)
timeout(288-310)
tests/unit/contracts/test_hash_registry.py (6)
src/omnibase_core/contracts/hash_registry.py (16)
ContractHashRegistry(283-566)compute_contract_fingerprint(215-280)normalize_contract(156-212)register(330-353)lookup(377-386)lookup_string(388-398)register_from_contract(355-375)verify(400-421)unregister(511-523)clear(525-527)count(537-543)list_contracts(529-535)to_dict(545-551)from_dict(554-566)detect_drift(423-488)detect_drift_from_contract(490-509)src/omnibase_core/models/contracts/model_contract_fingerprint.py (3)
ModelContractFingerprint(37-229)from_string(142-213)matches(215-229)src/omnibase_core/models/contracts/model_contract_normalization_config.py (1)
ModelContractNormalizationConfig(31-93)src/omnibase_core/models/contracts/model_drift_details.py (1)
ModelDriftDetails(33-98)src/omnibase_core/models/contracts/model_drift_result.py (1)
ModelDriftResult(47-111)src/omnibase_core/models/contracts/model_contract_version.py (1)
ModelContractVersion(21-281)
src/omnibase_core/models/contracts/model_contract_fingerprint.py (2)
src/omnibase_core/enums/enum_core_error_code.py (1)
EnumCoreErrorCode(9-154)src/omnibase_core/models/contracts/model_contract_version.py (1)
ModelContractVersion(21-281)
tests/unit/nodes/test_node_purity.py (1)
scripts/check_node_purity.py (7)
PurityAnalyzer(480-1030)PurityCheckResult(110-118)Severity(75-79)ViolationType(59-72)_safe_relative_path(1156-1170)analyze_file(1038-1123)NodeTypeFinder(364-476)
src/omnibase_core/contracts/hash_registry.py (5)
src/omnibase_core/models/contracts/model_contract_fingerprint.py (3)
ModelContractFingerprint(37-229)from_string(142-213)matches(215-229)src/omnibase_core/models/contracts/model_contract_normalization_config.py (1)
ModelContractNormalizationConfig(31-93)src/omnibase_core/models/contracts/model_contract_version.py (1)
ModelContractVersion(21-281)src/omnibase_core/models/contracts/model_drift_details.py (1)
ModelDriftDetails(33-98)src/omnibase_core/models/contracts/model_drift_result.py (1)
ModelDriftResult(47-111)
src/omnibase_core/contracts/__init__.py (8)
src/omnibase_core/contracts/hash_registry.py (3)
ContractHashRegistry(283-566)compute_contract_fingerprint(215-280)normalize_contract(156-212)src/omnibase_core/models/contracts/model_contract_fingerprint.py (1)
ModelContractFingerprint(37-229)src/omnibase_core/models/contracts/model_contract_meta.py (3)
ModelContractMeta(43-394)is_valid_meta_model(450-481)validate_meta_model(397-447)src/omnibase_core/models/contracts/model_contract_node_metadata.py (1)
ModelContractNodeMetadata(40-95)src/omnibase_core/models/contracts/model_contract_normalization_config.py (1)
ModelContractNormalizationConfig(31-93)src/omnibase_core/models/contracts/model_drift_details.py (1)
ModelDriftDetails(33-98)src/omnibase_core/models/contracts/model_drift_result.py (1)
ModelDriftResult(47-111)src/omnibase_core/models/contracts/model_node_extensions.py (1)
ModelNodeExtensions(39-76)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Smoke Tests
- GitHub Check: claude-review
🔇 Additional comments (27)
tests/unit/nodes/test_rollback_failure_handling.py (1)
23-25: Class-level timeouts look good; keep an eye on duration choiceAdding
@pytest.mark.timeout(60)at the class level for these rollback tests is a reasonable way to guard against hangs while keeping the per-test code clean. The markers are consistent with the existing@pytest.mark.unitusage and should work fine with the async tests.You may later want to tighten the 60s window (or move to more targeted timeouts) if these suites are known to be fast and you’re optimizing total CI runtime, but as a stabilization step this is perfectly fine.
Please just confirm that your pytest configuration (and any timeout plugin you rely on) already recognizes the
timeoutmark in CI so these tests don’t get flagged for unknown markers.Also applies to: 159-161
tests/unit/nodes/test_retry_count_tracking.py (1)
31-33: Class-level timeout marker is appropriate; confirm plugin availability in CIUsing
@pytest.mark.timeout(60)at the class level is a good safeguard against hanging async tests and the 60s budget is conservative for this suite. Just make surepytest-timeout(or equivalent) is installed/enabled in all environments running this suite so the marker is honored consistently.tests/unit/nodes/test_declarative_nodes.py (2)
465-465: Timeout appropriate for integration test complexity.The 60-second timeout is well-suited for integration tests that involve complete workflows and parallel execution scenarios.
139-139: No action needed — pytest-timeout is already configured.The pytest-timeout plugin is already installed as a development dependency (
pytest-timeout = "^2.4.0") in pyproject.toml, and global timeout settings are already configured in pytest.ini_options with--timeout=60. The timeout markers will work without any additional setup.tests/comparison_schema_value_runtime_validation.py (1)
25-30: Sys.path comment clearly documents the standalone script use-case.The new comments accurately explain why
srcis injected intosys.pathand match the repository layout for this comparison script; no code changes needed here.tests/validation/test_check_error_raising.py (1)
15-21: Scripts path explanation is accurate and helpful for maintainers.The added comments clearly justify adding
scripts/validationtosys.pathfor importingcheck_error_raising.pyand correctly reflect the directory layout.src/omnibase_core/models/contracts/model_contract_fingerprint.py (2)
37-99: Fingerprint model fields and validation constraints are well-structured.The combination of strict hex/length constraints on
hash_prefix/full_hash, frozen/extra-forbid config, and UTC-awarecomputed_atgives you a robust, immutable fingerprint model that matches the documented format and avoids untyped fields.
101-215: Confirm that equality and matching are intentionally based only on version + hash prefix.
__eq__(both object–object and object–string) andmatches()all compare usingversionandhash_prefix, whilefull_hashis ignored andfrom_string()synthesizes a paddedfull_hash. This keeps comparisons aligned with the configured prefix length but means distinct full hashes sharing the same prefix will be treated as equal. If that’s the intended collision trade-off for drift detection, this looks good; otherwise, consider either documenting this more explicitly or adding a separate helper for “full hash” equality.scripts/validation/validate_optimization.py (1)
17-23: Updated src path resolution correctly targets the project root.Using
Path(__file__).parent.parent.parent / "src"fromscripts/validationensures standalone runs can importomnibase_corewithout requiring installation, and the new comments make this intent clear.src/omnibase_core/models/contracts/model_drift_result.py (1)
47-112: Drift result model cleanly encapsulates detection state and detail.
DriftType, the optional fingerprint fields, UTCdetected_at, and a defaultModelDriftDetailsinstance align with howContractHashRegistry.detect_driftconstructs drift results, while frozen/extra-forbid config keeps these results immutable and predictable for CI/CD or audit workflows.docs/guides/THREADING.md (1)
32-508: Thread-safety guidance and examples align with the “no shared nodes across threads” rule.The new sections on
ThreadSafeComputeCache, thread-localNodeCompute/NodeEffect, transaction isolation, and container sharing give concrete, correct patterns that match the documented constraint to avoid sharing node instances across threads without synchronization, while clearly flagging where locks are required around mutable state. This is a solid, actionable threading guide.
Based on learnings, this matches the project’s concurrency expectations.src/omnibase_core/models/contracts/model_contract_meta.py (1)
43-395: Contract meta-model and validators are strongly typed and ONEX-aligned.Fields avoid
Any, config is strict and frozen, and the validators fornode_kind,version,node_id, and required strings all correctly normalize inputs and raiseModelOnexErrorwithEnumCoreErrorCode.VALIDATION_ERRORcontext on bad data. This matches the stated stability and “zero tolerance” goals for declarative node contract metadata.tests/unit/contracts/test_hash_registry.py (4)
1-42: Comprehensive test suite with well-documented categories.The module docstring clearly outlines the test categories and requirements from CONTRACT_STABILITY_SPEC.md. The imports are well-organized and include all necessary dependencies.
48-78: Well-designed test model with flexible version handling.The
ModelTestContractwith itsconvert_version_stringvalidator allows tests to use either string versions orModelContractVersionobjects, improving test ergonomics without sacrificing type safety.
999-1005: Using generic Exception for Pydantic validation errors is appropriate here.The test correctly uses
Exceptionas the base class since Pydantic'sValidationError(raised for invalidhash_lengthvalues) inherits from it. This is intentional for testing the model's validation constraints.
1119-1336: Thread safety documentation tests serve as valuable reference.The
TestContractHashRegistryConcurrencyclass effectively documents the thread safety requirements through executable tests. The extensive docstrings provide clear guidance for production usage patterns. The warning message at lines 1319-1332 is a nice touch for test output visibility.tests/unit/nodes/test_node_purity.py (3)
63-88: Well-implemented fixture correctly mirrors the two-pass analysis approach.The
analyze_sourcefixture properly replicates the two-pass analysis fromcheck_node_purity.py, usingNodeTypeFinderfor the first pass andPurityAnalyzerfor the second pass. This ensures tests exercise the same code paths as production.
979-1022: Addressed: Effect node skip behavior now properly assertsskip_reason is not None.The previous review comment about the assertion being too permissive has been addressed. The test now explicitly asserts that
result.skip_reason is not Nonewith clear error messages explaining the expected behavior.
1669-1693: Good test coverage for read vs write mode distinction.The
test_read_and_write_in_same_classtest effectively verifies that read-modeopen()is allowed while write-mode is flagged within the same class, providing confidence in the analyzer's mode detection logic.scripts/check_node_purity.py (4)
1-46: Comprehensive module documentation with clear purity guarantees.The docstring clearly documents forbidden patterns, allowed patterns, usage examples, and exit codes. This serves as excellent reference documentation for contributors.
602-634: Addressed:open()is now handled specially, avoiding false positives for read mode.The previous review comment about read-mode
open()being incorrectly flagged has been addressed. Theopen()function is not inFORBIDDEN_FILESYSTEM_FUNCTIONS(lines 203-229 document this), and the special handling invisit_Call(lines 602-612) only flags write-mode opens.
1156-1170: Addressed:_safe_relative_pathhelper handles paths outside base directory.The previous review comment about
relative_to()failure has been properly addressed with this helper function that catchesValueErrorand falls back to the full path.
1011-1017: Boundary check is correct for 1-based line numbers.The condition
line_number > 0 and line_number <= len(self.source_lines)is correct because AST line numbers are 1-based, and accessingsource_lines[line_number - 1]withline_number == len(source_lines)correctly accesses the last element.src/omnibase_core/contracts/hash_registry.py (4)
1-46: Excellent module documentation with ONEX compliance context.The docstring provides clear examples, cross-references to related documentation, and explicitly states the stability guarantees (deterministic fingerprinting, idempotent normalization). This is production-quality documentation.
72-154: Well-implemented normalization helpers with proper recursive handling.The helper functions
_convert_to_json_serializable,_remove_nulls_recursive, and_canonical_order_recursivecorrectly handle nested structures and edge cases like empty dicts after null removal.
156-213: Proper error handling usingModelOnexErroras per coding guidelines.The
normalize_contractfunction correctly wraps exceptions inModelOnexErrorwith appropriateEnumCoreErrorCode.VALIDATION_ERRORand preserves the original error message. This aligns with the coding guidelines for error handling.
283-323: Clear thread safety documentation in class docstring.The explicit "Thread Safety" section warning that the class is NOT thread-safe and referencing the THREADING.md guide is excellent practice for production code.
Addresses review comments on PR #143: 1. Fix incomplete task cancellation in cleanup_service_tasks fixture - Added await for cancelled tasks similar to event_loop_cleanup pattern - Ensures proper cleanup of health monitor tasks 2. Remove unused pattern parameter from _contains_relevant_classes() - Parameter was passed but never used in function body - Updated both function definition and call site 3. Fix main() function signature in validate_naming.py - Changed return type from `-> int` to `-> None` - Function always calls sys.exit() and never returns 4. Strengthen assertion in test_normalize_with_custom_config_no_sorting - Added JSON parsing validation - Added content verification (name field check) 5. Add language specification to fenced code block in IMPORT_COMPATIBILITY_MATRIX.md - Added `text` language spec to ASCII diagram code block - Fixes MD040 linting warning
1 participant
Summary
Changes
Phase 0 Stabilization Infrastructure
compute_contract_fingerprint()for deterministic contract hashingnormalize_contract()for consistent hash computationContractHashRegistryfor contract integrity verificationTest Fixes
datetime.utcnow()calls →datetime.now(UTC)across all mixin testsDeprecationWarningfor invalid escape sequences in regex patternsNaming Convention Fix
ContractHashRegistrytoARCHITECTURAL_EXEMPTIONSin naming validator{Domain}Registrypattern (likeServiceRegistry)Test plan
poetry run python scripts/validation/validate_naming.py src/Related Issues
Summary by CodeRabbit
New Features
Documentation
Development Tools
Tests
✏️ Tip: You can customize this high-level summary in your review settings.