Repository navigation
feat(compute): implement contract-driven NodeCompute v1.0 [OMN-465] - #115
Conversation
Implement the v1.0 contract-driven NodeCompute specification with sequential pipelines, abort-on-first-failure semantics, and 6 built-in transformation types. ## Core Components ### Enums (6 new) - EnumComputeStepType: VALIDATION, TRANSFORMATION, MAPPING - EnumTransformationType: IDENTITY, REGEX, CASE_CONVERSION, TRIM, NORMALIZE_UNICODE, JSON_PATH - EnumCaseMode, EnumRegexFlag, EnumTrimMode, EnumUnicodeForm ### Models - ModelComputeExecutionContext: Typed execution context with operation/correlation IDs - ModelComputeStepMetadata/Result: Step-level execution tracking - ModelComputePipelineResult: Pipeline result with abort-on-first-failure - ModelComputePipelineStep: Pipeline step with model validator - ModelComputeSubcontract: v1.0 contract definition - 7 transformation config models with discriminated union ### Execution - compute_transformations.py: 6 pure transformation functions - compute_executor.py: Pipeline executor with path resolution - MixinComputeExecution: Integration mixin for NodeCompute ## Key Features - Sequential-only pipelines (parallel deferred to v1.2) - Abort-on-first-failure semantics - Simple path expressions: $.input, $.steps.<name>.output - Immutable results (frozen=True) - IDENTITY transformation requires no config ## Testing - 95 unit tests covering all components - mypy strict: 0 errors in 25 source files
|
Warning Rate limit exceeded@jonahgabriel has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 9 minutes and 46 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughAdds a v1.0 contract-driven NodeCompute spec plus implementation: new enums, immutable compute and transformation models, mapping/validation configs, a stateless executor, path resolver, mixin for execution, ComputePipelineError, example contract, and extensive unit/integration tests for sequential, abort-on-first-failure pipelines. Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant Mixin as MixinComputeExecution
participant Executor as execute_compute_pipeline
participant Dispatcher as execute_pipeline_step
participant Transformer as compute_transformations
participant Resolver as compute_path_resolver
participant Result as ModelComputePipelineResult
Client->>Mixin: execute_contract_pipeline(contract, input)
Mixin->>Mixin: validate_compute_contract(contract)
Mixin->>Executor: execute_compute_pipeline(contract, input, context)
loop for each step
Executor->>Dispatcher: execute_pipeline_step(step, current_data, input, step_results)
alt step.type == TRANSFORMATION
Dispatcher->>Transformer: execute_transformation(data, transformation_type, config)
Transformer-->>Dispatcher: transformed_value
else step.type == MAPPING
Dispatcher->>Resolver: resolve_mapping_path(path, input, step_results)
Resolver-->>Dispatcher: resolved_value
else step.type == VALIDATION
Dispatcher-->>Dispatcher: passthrough/validate
end
Dispatcher-->>Executor: ModelComputeStepResult (with timing, success/error)
Note over Executor: record step_results, stop on first failure
end
Executor-->>Mixin: ModelComputePipelineResult
Mixin-->>Client: ModelComputePipelineResult
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas to focus review on:
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
Comment |
Code Review: Contract-Driven NodeCompute v1.0 ImplementationSummaryThis PR implements a well-architected, contract-driven transformation pipeline for Overall Assessment: ✅ APPROVE with minor suggestions for improvement Strengths 🎉1. Excellent Architecture & Design
2. Strong Type Safety
3. Comprehensive Testing
4. Excellent Documentation
Issues & Concerns 🔴CRITICAL - Must Fix Before Merge1. Missing CI Verification ❌From the PR description:
Issue: Cannot verify if this passes the project's strict quality gates:
Action Required: Run full CI pipeline and verify all checks pass. Potential Issues
|
Security: - Block private attribute access in path resolution (getattr security) Quality: - Add comprehensive Google-style docstrings with thread safety notes - Add type aliases (JsonType, TransformationHandler) for better type safety - Add type comments explaining intentional Any usage Features: - Add validation step warning logging for v1.0 pass-through mode - Add TODO(v1.1) comments for future schema validation - Add 19 integration tests for compute pipeline Files modified: - compute_executor.py: Security fix, docstrings, logging - compute_transformations.py: Security fix, docstrings, type aliases - mixin_compute_execution.py: Comprehensive docstrings - model_compute_execution_context.py: Enhanced docstrings - model_compute_pipeline_result.py: Enhanced docstrings Files added: - tests/integration/test_compute_pipeline_integration.py
PR Review: Contract-Driven NodeCompute v1.0 ImplementationSummaryThis PR implements contract-driven NodeCompute with sequential pipelines and 6 built-in transformation types. The implementation follows established patterns from NodeReducer and NodeOrchestrator, achieving the goal of zero custom Python code for compute node implementations. Overall Assessment: ✅ APPROVE with minor recommendations This is an excellent foundation for v1.0. The code quality is high, testing is comprehensive, and the architecture follows ONEX principles consistently. ✅ Strengths1. Architectural Excellence
2. Type Safety & Code Quality
3. Testing
4. Documentation
5. Error Handling
💡 Recommendations (Minor - Non-Blocking)1. Pipeline Timeout (Minor)Issue: pipeline_timeout_ms is defined in the contract but not enforced in v1.0. Recommendation: Add a TODO comment or implement basic timeout checking. Rationale: Without timeout enforcement, long-running pipelines could block indefinitely. 2. Validation Step Warning Frequency (Minor)Issue: Validation step logs a warning every execution (compute_executor.py:307-311). Recommendation: Consider using logger.info or logging once at contract load time to reduce log noise in production. 3. Missing Example Contract (Minor)Issue: No working example YAML contract in examples/contracts/compute/. Recommendation: Add the text_processor_v1.yaml example from the spec for reference. 🔒 Security Assessment✅ No security vulnerabilities identified
Note: Users can create slow pipelines with catastrophic backtracking regex patterns. Consider adding regex complexity analysis in v1.1. 🎯 Acceptance Criteria CheckFunctional Requirements
Type Safety Requirements
Testing Requirements
📝 Adherence to CLAUDE.md✅ Excellent adherence to project guidelines:
🎉 Final VerdictStatus: ✅ APPROVE This is a well-crafted, production-ready v1.0 implementation that:
The recommendations above are minor enhancements that can be addressed in follow-up PRs or v1.1. None are blocking for merge. Great work! This sets a strong precedent for contract-driven node implementations. Reviewer: Claude Code |
![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
🧹 Nitpick comments (22)
src/omnibase_core/enums/enum_unicode_form.py (1)
7-24: Well-structured enum with correct Unicode normalization forms.The enum correctly defines the four standard Unicode normalization forms (NFC, NFD, NFKC, NFKD) that align with Python's
unicodedata.normalize()function. The(str, Enum)base classes ensure JSON serialization compatibility.Consider adding
__all__for explicit public API declaration, consistent with other modules likemodel_compute_execution_context.py:from enum import Enum +__all__ = [ + "EnumUnicodeForm", +] + class EnumUnicodeForm(str, Enum):src/omnibase_core/models/compute/model_compute_execution_context.py (1)
91-95: Clean, immutable execution context model.The model is well-designed with proper type annotations and immutability via
frozen=True. The documentation is thorough.For consistency with other v1.0 models like
ModelTransformCaseConfigwhich useConfigDict(extra="forbid", frozen=True), consider addingextra="forbid"to reject unexpected fields:- model_config = ConfigDict(frozen=True) + model_config = ConfigDict(extra="forbid", frozen=True)This provides stricter schema validation and guards against typos in field names during instantiation.
src/omnibase_core/models/transformations/model_transform_case_config.py (1)
15-27: Well-structured discriminated union config model.The model correctly implements the discriminated union pattern with
config_typeas the discriminator. UsingConfigDict(extra="forbid", frozen=True)ensures strict validation and immutability, aligning with the coding guidelines.Consider adding
__all__for explicit public API declaration:from omnibase_core.enums.enum_case_mode import EnumCaseMode +__all__ = [ + "ModelTransformCaseConfig", +] + class ModelTransformCaseConfig(BaseModel):tests/unit/enums/test_compute_enums.py (1)
53-66: Consider adding member count tests for remaining enums.
TestEnumComputeStepTypeandTestEnumTransformationTypeincludetest_member_countto validate v1.0 contract boundaries. For consistency and to guard against accidental additions, consider adding similar tests for the remaining enums:def test_member_count(self) -> None: """Test v1.0 member count.""" assert len(EnumCaseMode) == 3 # Similarly for: # EnumRegexFlag: 3 members # EnumUnicodeForm: 4 members # EnumTrimMode: 3 memberstests/unit/models/transformations/test_transformation_configs.py (1)
1-24: Solid config coverage; add pytest markers/timeout for consistencyThe tests exercise creation, defaults, extra-field rejection, and immutability across all config models — this looks good. To align with the repo’s test conventions, consider adding module-level markers and a timeout:
import pytest from pydantic import ValidationError +pytestmark = [pytest.mark.unit, pytest.mark.timeout(5)] +This keeps the file concise while satisfying the requirement for
@pytest.mark.unitand@pytest.mark.timeout()on tests. As per coding guidelines, tests should consistently use these markers.src/omnibase_core/models/compute/model_compute_step_metadata.py (1)
14-26: Consider constrainingduration_msto non-negative valuesThe model is clean and immutable, matching the compute models pattern. If you want to guard against accidental negative durations, you could add a
ge=0constraint viaFieldor similar. Not required for correctness, but it would make the timing metadata stricter and self-validating.src/omnibase_core/models/transformations/model_transform_json_path_config.py (1)
13-25: Optional: add lightweight validation for JSONPath stringsThe config model is minimal, immutable, and uses a clear discriminator, which fits the transformation-union pattern well. If you want slightly stronger contracts, consider a small validator (e.g., ensuring
pathis non-empty and maybe starts with"$"), while still keeping parsing to the execution layer. Based on learnings, this stays aligned with the “immutable Pydantic models” guideline.docs/architecture/NODECOMPUTE_VERSIONING_ROADMAP.md (3)
106-112: Add language identifier to fenced code block.The code block should specify a language for syntax highlighting and better markdown compliance.
Apply this diff:
-``` +```text 1.x.y | | | | | +-- Patch: Bug fixes, performance improvements (no API changes) | +---- Minor: New features, new transformations (backward compatible) +------ Major: Breaking changes (new replay epoch)--- `132-138`: **Add language identifier to fenced code block.** Specify a language (e.g., `text` or `plaintext`) for the timeline code block. Apply this diff: ```diff -``` +```text Q1 2025: v1.0 Implementation and Production Validation Q2 2025: v1.1 Design and Implementation (if warranted) Q3 2025: v1.2 Design and Implementation (if warranted) Q4 2025: v1.3 Design and Implementation (if warranted)--- `927-931`: **Add language identifier to fenced code block.** Specify a language (e.g., `text`) for the compatibility promise code block. Apply this diff: ```diff -``` +```text Any contract valid in v1.0 MUST be valid in v1.3 Any contract valid in v1.0 MUST produce identical output in v1.3 Any ledger entry from v1.0 MUST be replayable in v1.3</blockquote></details> <details> <summary>src/omnibase_core/models/contracts/subcontracts/model_compute_subcontract.py (1)</summary><blockquote> `45-49`: **Consider adding validation for `pipeline_timeout_ms` and `pipeline`.** The `pipeline_timeout_ms` field accepts any integer including zero or negative values, which would be invalid. Similarly, an empty pipeline list may be semantically invalid. Consider adding a `model_validator` or `field_validator`: ```diff +from pydantic import BaseModel, ConfigDict, field_validator + class ModelComputeSubcontract(BaseModel): ... + + @field_validator("pipeline_timeout_ms") + @classmethod + def validate_timeout(cls, v: int | None) -> int | None: + if v is not None and v <= 0: + raise ValueError("pipeline_timeout_ms must be positive") + return vsrc/omnibase_core/models/transformations/model_mapping_config.py (1)
26-28: Consider validatingfield_mappingsis non-empty.An empty
field_mappingsdictionary would produce a no-op mapping step, which may not be the intended behavior.+from pydantic import BaseModel, ConfigDict, field_validator + class ModelMappingConfig(BaseModel): ... + + @field_validator("field_mappings") + @classmethod + def validate_field_mappings(cls, v: dict[str, str]) -> dict[str, str]: + if not v: + raise ValueError("field_mappings must not be empty") + return vAlternatively, if empty mappings are valid for passthrough semantics, this can be ignored.
docs/architecture/CONTRACT_DRIVEN_NODECOMPUTE_V1_0.md (1)
482-486: Add language specifier to fenced code blocks.The ASCII diagrams should have a language specifier for markdown linting compliance.
Apply this pattern to the diagram code blocks:
-``` +```text Step 1 → Step 2 → Step 3 → ... → Step N → OutputThis applies to the diagrams at lines 482-486, 503-510, and 677-691.
Also applies to: 503-510, 677-691
tests/unit/models/compute/test_compute_models.py (2)
1-9: Missing pytest marker for unit tests.Per coding guidelines, test files should use pytest markers. Add
@pytest.mark.unitto the test classes or configure it at the module level."""Unit tests for Contract-Driven NodeCompute v1.0 compute models.""" + import pytest from uuid import uuid4 + +pytestmark = pytest.mark.unit from omnibase_core.models.compute.model_compute_execution_context import ModelComputeExecutionContext
35-39: Use specific exception type for frozen model tests.
pytest.raises(Exception)is too broad. Pydantic frozen models raisepydantic.ValidationErrorwhen mutation is attempted. Using the specific exception type improves test reliability.+from pydantic import ValidationError + # In each test_is_frozen method: - with pytest.raises(Exception): # ValidationError for frozen model + with pytest.raises(ValidationError):Apply this pattern to all four
test_is_frozenmethods (lines 38, 63, 108, 173).src/omnibase_core/mixins/mixin_compute_execution.py (1)
37-47: Inconsistent UUID import pattern.
uuid4is imported at runtime whileUUIDis only imported underTYPE_CHECKING. SinceUUIDis used in the runtime type hint fornode_id(line 85), this should work but is inconsistent. Consider importingUUIDat runtime for clarity.-from typing import Any, TYPE_CHECKING +from typing import Any -from uuid import uuid4 +from uuid import UUID, uuid4 from omnibase_core.models.compute.model_compute_execution_context import ModelComputeExecutionContext from omnibase_core.models.compute.model_compute_pipeline_result import ModelComputePipelineResult from omnibase_core.models.contracts.subcontracts.model_compute_subcontract import ModelComputeSubcontract from omnibase_core.utils.compute_executor import execute_compute_pipeline -if TYPE_CHECKING: - from uuid import UUID -tests/integration/test_compute_pipeline_integration.py (3)
1-17: Missing pytest marker for integration tests.Per coding guidelines, add
@pytest.mark.integrationmarker for integration tests. Also consider extracting the duplicate_create_contexthelper.""" Integration tests for contract-driven NodeCompute pipeline execution. ... """ from uuid import uuid4 import pytest +pytestmark = pytest.mark.integration + from omnibase_core.enums.enum_case_mode import EnumCaseMode
476-478: Simplify regex test assertion.The compound assertion is confusing and potentially fragile. Since the regex with
IGNORECASEshould replace all variations of "hello", assert the exact expected output directly.assert result.success is True - # All variations of "hello" should be replaced - assert "hello" not in result.output.lower() or result.output == "hi World, hi friend, hi there" + # All variations of "hello" should be replaced (case-insensitive) + assert result.output == "hi World, hi friend, hi there"
55-60: Consider extracting duplicate helper to fixture.The
_create_contextmethod is duplicated inTestComputePipelineErrorScenarios(line 615). Consider using a module-level pytest fixture for DRY.+@pytest.fixture +def execution_context() -> ModelComputeExecutionContext: + """Create a test execution context.""" + return ModelComputeExecutionContext( + operation_id=uuid4(), + correlation_id=uuid4(), + ) + + class TestComputePipelineIntegration: """Integration tests for complete pipeline scenarios.""" - - def _create_context(self) -> ModelComputeExecutionContext: - """Create a test execution context.""" - return ModelComputeExecutionContext( - operation_id=uuid4(), - correlation_id=uuid4(), - )Then use
execution_contextfixture parameter in test methods instead ofself._create_context().src/omnibase_core/utils/compute_executor.py (1)
317-377: Step dispatcher is correct; minor redundancy onlyDispatching on
EnumComputeStepTypewith dedicated handlers for TRANSFORMATION, MAPPING, and VALIDATION is clean, and missing configuration cases correctly raiseModelOnexError. The extraif not step.enabledhere is redundant because disabled steps are already skipped in the main loop, but it’s harmless; you could drop it later to reduce duplication.docs/architecture/NODECOMPUTE_FULL_DESIGN_V1X_TARGET.md (1)
1-2473: Tidy up markdownlint issues (fence languages, emphasis headings, duplicate heading)markdownlint flags a few mechanical issues in this doc:
- Fenced code blocks without a language (e.g., generic triple‑backticks) – add
text,yaml, orpythonas appropriate.- Lines using emphasis as a pseudo‑heading (e.g.,
**Phase 1: Load**) – convert these to proper###/####headings.- Duplicate heading text (another “Transformation Registry” section later in the file) – rename or merge to avoid MD024.
These are minor, but fixing them will keep docs lint‑clean while preserving the existing v1.x design content.
src/omnibase_core/utils/compute_transformations.py (1)
76-99: Aligntransform_identitysignature with other handlers for safer registry useRight now
transform_identityonly acceptsdata, butTRANSFORMATION_REGISTRYis documented as a generic handler registry andexecute_transformationcalls other handlers as(data, config). If any caller ever uses the registry directly and assumes a uniform(data, config)signature, IDENTITY will raise aTypeError.To future‑proof the API, consider letting
transform_identityaccept an optional (ignored) config:-def transform_identity( - data: Any, # Any: intentionally polymorphic - accepts any input type unchanged -) -> Any: # Any: output type mirrors input type +def transform_identity( + data: Any, # Any: intentionally polymorphic - accepts any input type unchanged + config: ModelTransformationConfig | None = None, +) -> Any: # Any: output type mirrors input type
execute_transformationcan keep calling it as it does now, and registry lookups remain safe for uniform(data, config)calls.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (36)
docs/architecture/CONTRACT_DRIVEN_NODECOMPUTE_V1_0.md(1 hunks)docs/architecture/NODECOMPUTE_FULL_DESIGN_V1X_TARGET.md(1 hunks)docs/architecture/NODECOMPUTE_VERSIONING_ROADMAP.md(1 hunks)src/omnibase_core/enums/__init__.py(5 hunks)src/omnibase_core/enums/enum_case_mode.py(1 hunks)src/omnibase_core/enums/enum_compute_step_type.py(1 hunks)src/omnibase_core/enums/enum_regex_flag.py(1 hunks)src/omnibase_core/enums/enum_transformation_type.py(1 hunks)src/omnibase_core/enums/enum_trim_mode.py(1 hunks)src/omnibase_core/enums/enum_unicode_form.py(1 hunks)src/omnibase_core/mixins/__init__.py(2 hunks)src/omnibase_core/mixins/mixin_compute_execution.py(1 hunks)src/omnibase_core/models/compute/__init__.py(1 hunks)src/omnibase_core/models/compute/model_compute_execution_context.py(1 hunks)src/omnibase_core/models/compute/model_compute_pipeline_result.py(1 hunks)src/omnibase_core/models/compute/model_compute_step_metadata.py(1 hunks)src/omnibase_core/models/compute/model_compute_step_result.py(1 hunks)src/omnibase_core/models/contracts/subcontracts/__init__.py(2 hunks)src/omnibase_core/models/contracts/subcontracts/model_compute_pipeline_step.py(1 hunks)src/omnibase_core/models/contracts/subcontracts/model_compute_subcontract.py(1 hunks)src/omnibase_core/models/transformations/__init__.py(1 hunks)src/omnibase_core/models/transformations/model_mapping_config.py(1 hunks)src/omnibase_core/models/transformations/model_transform_case_config.py(1 hunks)src/omnibase_core/models/transformations/model_transform_json_path_config.py(1 hunks)src/omnibase_core/models/transformations/model_transform_regex_config.py(1 hunks)src/omnibase_core/models/transformations/model_transform_trim_config.py(1 hunks)src/omnibase_core/models/transformations/model_transform_unicode_config.py(1 hunks)src/omnibase_core/models/transformations/model_validation_step_config.py(1 hunks)src/omnibase_core/models/transformations/types.py(1 hunks)src/omnibase_core/utils/compute_executor.py(1 hunks)src/omnibase_core/utils/compute_transformations.py(1 hunks)tests/integration/test_compute_pipeline_integration.py(1 hunks)tests/unit/enums/test_compute_enums.py(1 hunks)tests/unit/models/compute/test_compute_models.py(1 hunks)tests/unit/models/transformations/test_transformation_configs.py(1 hunks)tests/unit/utils/test_compute_pipeline.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Always use Poetry for Python package management and task execution. Usepoetry runfor pytest, mypy, black, isort, and other tools. Never use pip directly or execute python commands without the poetry run prefix.
Enforce 100% strict mypy compliance. All functions must have complete type annotations. Usepoetry run mypy src/omnibase_core/to verify strict mode compliance with no errors.
Format code with black, sort imports with isort, and lint with ruff. Runpoetry run black src/ tests/,poetry run isort src/ tests/, andpoetry run ruff check --fix src/ tests/
Files:
src/omnibase_core/models/transformations/model_transform_json_path_config.pysrc/omnibase_core/models/compute/model_compute_execution_context.pysrc/omnibase_core/models/transformations/model_transform_case_config.pysrc/omnibase_core/models/compute/__init__.pysrc/omnibase_core/enums/enum_compute_step_type.pytests/unit/models/compute/test_compute_models.pysrc/omnibase_core/enums/enum_regex_flag.pysrc/omnibase_core/enums/enum_transformation_type.pysrc/omnibase_core/models/transformations/model_validation_step_config.pysrc/omnibase_core/models/transformations/types.pysrc/omnibase_core/models/contracts/subcontracts/model_compute_pipeline_step.pytests/unit/enums/test_compute_enums.pysrc/omnibase_core/models/transformations/model_transform_unicode_config.pysrc/omnibase_core/models/contracts/subcontracts/model_compute_subcontract.pysrc/omnibase_core/models/transformations/model_transform_regex_config.pysrc/omnibase_core/utils/compute_executor.pysrc/omnibase_core/models/compute/model_compute_step_metadata.pysrc/omnibase_core/enums/enum_trim_mode.pytests/unit/models/transformations/test_transformation_configs.pytests/unit/utils/test_compute_pipeline.pysrc/omnibase_core/enums/__init__.pysrc/omnibase_core/models/compute/model_compute_step_result.pysrc/omnibase_core/enums/enum_case_mode.pysrc/omnibase_core/mixins/mixin_compute_execution.pytests/integration/test_compute_pipeline_integration.pysrc/omnibase_core/enums/enum_unicode_form.pysrc/omnibase_core/models/transformations/model_transform_trim_config.pysrc/omnibase_core/models/contracts/subcontracts/__init__.pysrc/omnibase_core/mixins/__init__.pysrc/omnibase_core/models/compute/model_compute_pipeline_result.pysrc/omnibase_core/utils/compute_transformations.pysrc/omnibase_core/models/transformations/__init__.pysrc/omnibase_core/models/transformations/model_mapping_config.py
src/omnibase_core/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/omnibase_core/**/*.py: Use protocol names withcontainer.get_service()for dependency injection, never resolve services by concrete class names. Example:container.get_service('ProtocolEventBus')instead ofcontainer.get_service('EventBusService')
Always use ModelONEXContainer for dependency injection in node constructors and service resolution. Never confuse it with ModelContainer[T], which is a generic value wrapper, not a DI container.
Use structured error handling with ModelOnexError and EnumCoreErrorCode, never raise generic Exception. Always chain exceptions withfrom ewhen catching and re-raising.
Use the @standard_error_handling decorator on methods to eliminate 6+ lines of try/catch boilerplate. Example:@standard_error_handlingabove async methods.
Respect thread safety constraints: Do NOT share node instances (NodeCompute, NodeEffect), cache, or circuit breaker instances across threads without synchronization. Use thread-local instances or locks.
Use duck typing with protocols. Do not use isinstance checks with protocol types. Rely on the protocol interface directly.
Use EnumNodeKind for high-level architectural classification (EFFECT, COMPUTE, REDUCER, ORCHESTRATOR) when routing data through the ONEX pipeline. Use EnumNodeType for specific implementation discovery and capability matching.
Use ModelEventEnvelope for inter-service event-driven communication. Process events through the envelope pattern for consistent event handling.
Files:
src/omnibase_core/models/transformations/model_transform_json_path_config.pysrc/omnibase_core/models/compute/model_compute_execution_context.pysrc/omnibase_core/models/transformations/model_transform_case_config.pysrc/omnibase_core/models/compute/__init__.pysrc/omnibase_core/enums/enum_compute_step_type.pysrc/omnibase_core/enums/enum_regex_flag.pysrc/omnibase_core/enums/enum_transformation_type.pysrc/omnibase_core/models/transformations/model_validation_step_config.pysrc/omnibase_core/models/transformations/types.pysrc/omnibase_core/models/contracts/subcontracts/model_compute_pipeline_step.pysrc/omnibase_core/models/transformations/model_transform_unicode_config.pysrc/omnibase_core/models/contracts/subcontracts/model_compute_subcontract.pysrc/omnibase_core/models/transformations/model_transform_regex_config.pysrc/omnibase_core/utils/compute_executor.pysrc/omnibase_core/models/compute/model_compute_step_metadata.pysrc/omnibase_core/enums/enum_trim_mode.pysrc/omnibase_core/enums/__init__.pysrc/omnibase_core/models/compute/model_compute_step_result.pysrc/omnibase_core/enums/enum_case_mode.pysrc/omnibase_core/mixins/mixin_compute_execution.pysrc/omnibase_core/enums/enum_unicode_form.pysrc/omnibase_core/models/transformations/model_transform_trim_config.pysrc/omnibase_core/models/contracts/subcontracts/__init__.pysrc/omnibase_core/mixins/__init__.pysrc/omnibase_core/models/compute/model_compute_pipeline_result.pysrc/omnibase_core/utils/compute_transformations.pysrc/omnibase_core/models/transformations/__init__.pysrc/omnibase_core/models/transformations/model_mapping_config.py
src/omnibase_core/models/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use Pydantic 2.11+ models for all data validation and serialization. Ensure models are immutable after creation for thread safety.
Files:
src/omnibase_core/models/transformations/model_transform_json_path_config.pysrc/omnibase_core/models/compute/model_compute_execution_context.pysrc/omnibase_core/models/transformations/model_transform_case_config.pysrc/omnibase_core/models/compute/__init__.pysrc/omnibase_core/models/transformations/model_validation_step_config.pysrc/omnibase_core/models/transformations/types.pysrc/omnibase_core/models/contracts/subcontracts/model_compute_pipeline_step.pysrc/omnibase_core/models/transformations/model_transform_unicode_config.pysrc/omnibase_core/models/contracts/subcontracts/model_compute_subcontract.pysrc/omnibase_core/models/transformations/model_transform_regex_config.pysrc/omnibase_core/models/compute/model_compute_step_metadata.pysrc/omnibase_core/models/compute/model_compute_step_result.pysrc/omnibase_core/models/transformations/model_transform_trim_config.pysrc/omnibase_core/models/contracts/subcontracts/__init__.pysrc/omnibase_core/models/compute/model_compute_pipeline_result.pysrc/omnibase_core/models/transformations/__init__.pysrc/omnibase_core/models/transformations/model_mapping_config.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Write tests using pytest with async support via pytest-asyncio. Maintain minimum 60% code coverage. Use pytest markers: @pytest.mark.unit, @pytest.mark.integration, @pytest.mark.slow
Add @pytest.mark.timeout() to prevent test hangs. Run tests withpoetry run pytest tests/ --timeout=60for 60-second timeout protection.
Files:
tests/unit/models/compute/test_compute_models.pytests/unit/enums/test_compute_enums.pytests/unit/models/transformations/test_transformation_configs.pytests/unit/utils/test_compute_pipeline.pytests/integration/test_compute_pipeline_integration.py
src/omnibase_core/mixins/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Mixin classes should be composed into node classes for specific capabilities. Available mixins include: MixinDiscoveryResponder, MixinEventHandler, MixinEventListener, MixinNodeExecutor, MixinNodeLifecycle, MixinRequestResponseIntrospection, MixinWorkflowSupport.
Files:
src/omnibase_core/mixins/mixin_compute_execution.pysrc/omnibase_core/mixins/__init__.py
🧠 Learnings (8)
📓 Common learnings
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T23:34:11.788Z
Learning: Applies to src/omnibase_core/nodes/node_compute.py : NodeCompute is for data processing and transformation. Inherit from NodeCompute for COMPUTE nodes that perform calculations, validations, and data mapping.
📚 Learning: 2025-12-06T23:34:11.788Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T23:34:11.788Z
Learning: Applies to src/omnibase_core/models/**/*.py : Use Pydantic 2.11+ models for all data validation and serialization. Ensure models are immutable after creation for thread safety.
Applied to files:
src/omnibase_core/models/transformations/model_transform_json_path_config.pysrc/omnibase_core/models/transformations/model_validation_step_config.pysrc/omnibase_core/models/transformations/types.pysrc/omnibase_core/models/transformations/model_transform_unicode_config.pysrc/omnibase_core/models/transformations/model_mapping_config.py
📚 Learning: 2025-12-06T23:34:11.788Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T23:34:11.788Z
Learning: Applies to src/omnibase_core/nodes/node_compute.py : NodeCompute is for data processing and transformation. Inherit from NodeCompute for COMPUTE nodes that perform calculations, validations, and data mapping.
Applied to files:
src/omnibase_core/models/compute/__init__.pytests/unit/models/compute/test_compute_models.pysrc/omnibase_core/enums/__init__.pysrc/omnibase_core/mixins/mixin_compute_execution.py
📚 Learning: 2025-12-06T23:34:11.788Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T23:34:11.788Z
Learning: Applies to docs/guides/node-building/**/*.md : Node building guide is the critical starting point for developers and AI agents. Follow the sequence: What is a Node → Node Types → COMPUTE Tutorial → EFFECT Tutorial → REDUCER Tutorial → ORCHESTRATOR Tutorial.
Applied to files:
docs/architecture/NODECOMPUTE_VERSIONING_ROADMAP.mddocs/architecture/NODECOMPUTE_FULL_DESIGN_V1X_TARGET.md
📚 Learning: 2025-12-06T23:34:11.788Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T23:34:11.788Z
Learning: Applies to src/omnibase_core/**/*.py : Use EnumNodeKind for high-level architectural classification (EFFECT, COMPUTE, REDUCER, ORCHESTRATOR) when routing data through the ONEX pipeline. Use EnumNodeType for specific implementation discovery and capability matching.
Applied to files:
src/omnibase_core/enums/enum_compute_step_type.pysrc/omnibase_core/enums/enum_transformation_type.pysrc/omnibase_core/enums/__init__.pysrc/omnibase_core/enums/enum_case_mode.py
📚 Learning: 2025-09-23T22:28:00.333Z
Learnt from: jonahgabriel
Repo: OmniNode-ai/omnibase_core PR: 32
File: examples/practical_migration_example.py:125-134
Timestamp: 2025-09-23T22:28:00.333Z
Learning: The omnibase_core codebase extensively uses constrained TypeVars for primitive types with the pattern `T = TypeVar("T", str, int, float, bool)` across multiple models including ModelTypedMetrics, ModelGenericMetadata, and ModelExecutionResult. This is the preferred approach over hardcoded union types like `str | int | bool | float | None`.
Applied to files:
src/omnibase_core/models/transformations/types.py
📚 Learning: 2025-12-06T23:34:11.788Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T23:34:11.788Z
Learning: Applies to src/omnibase_core/mixins/**/*.py : Mixin classes should be composed into node classes for specific capabilities. Available mixins include: MixinDiscoveryResponder, MixinEventHandler, MixinEventListener, MixinNodeExecutor, MixinNodeLifecycle, MixinRequestResponseIntrospection, MixinWorkflowSupport.
Applied to files:
src/omnibase_core/mixins/mixin_compute_execution.pysrc/omnibase_core/mixins/__init__.py
📚 Learning: 2025-12-06T23:34:11.788Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T23:34:11.788Z
Learning: Respect the ONEX Four-Node Architecture: data flows unidirectionally from EFFECT → COMPUTE → REDUCER → ORCHESTRATOR with no backwards dependencies. This is an architectural principle, not tied to specific files.
Applied to files:
docs/architecture/NODECOMPUTE_FULL_DESIGN_V1X_TARGET.md
🧬 Code graph analysis (21)
src/omnibase_core/models/compute/model_compute_execution_context.py (1)
src/omnibase_core/models/metadata/model_metadata_field_info.py (1)
uuid(162-181)
src/omnibase_core/models/transformations/model_transform_case_config.py (1)
src/omnibase_core/enums/enum_case_mode.py (1)
EnumCaseMode(10-22)
src/omnibase_core/models/compute/__init__.py (4)
src/omnibase_core/models/compute/model_compute_execution_context.py (1)
ModelComputeExecutionContext(44-95)src/omnibase_core/models/compute/model_compute_pipeline_result.py (1)
ModelComputePipelineResult(53-122)src/omnibase_core/models/compute/model_compute_step_metadata.py (1)
ModelComputeStepMetadata(14-26)src/omnibase_core/models/compute/model_compute_step_result.py (1)
ModelComputeStepResult(23-43)
tests/unit/models/compute/test_compute_models.py (4)
src/omnibase_core/models/compute/model_compute_execution_context.py (1)
ModelComputeExecutionContext(44-95)src/omnibase_core/models/compute/model_compute_step_metadata.py (1)
ModelComputeStepMetadata(14-26)src/omnibase_core/models/compute/model_compute_step_result.py (1)
ModelComputeStepResult(23-43)src/omnibase_core/models/compute/model_compute_pipeline_result.py (1)
ModelComputePipelineResult(53-122)
src/omnibase_core/models/transformations/types.py (5)
src/omnibase_core/models/transformations/model_transform_case_config.py (1)
ModelTransformCaseConfig(15-27)src/omnibase_core/models/transformations/model_transform_json_path_config.py (1)
ModelTransformJsonPathConfig(13-25)src/omnibase_core/models/transformations/model_transform_regex_config.py (1)
ModelTransformRegexConfig(15-31)src/omnibase_core/models/transformations/model_transform_trim_config.py (1)
ModelTransformTrimConfig(15-27)src/omnibase_core/models/transformations/model_transform_unicode_config.py (1)
ModelTransformUnicodeConfig(15-27)
src/omnibase_core/models/contracts/subcontracts/model_compute_pipeline_step.py (4)
src/omnibase_core/enums/enum_compute_step_type.py (1)
EnumComputeStepType(11-27)src/omnibase_core/enums/enum_transformation_type.py (1)
EnumTransformationType(11-34)src/omnibase_core/models/transformations/model_mapping_config.py (1)
ModelMappingConfig(13-29)src/omnibase_core/models/transformations/model_validation_step_config.py (1)
ModelValidationStepConfig(13-30)
tests/unit/enums/test_compute_enums.py (6)
src/omnibase_core/enums/enum_compute_step_type.py (1)
EnumComputeStepType(11-27)src/omnibase_core/enums/enum_transformation_type.py (1)
EnumTransformationType(11-34)src/omnibase_core/enums/enum_case_mode.py (1)
EnumCaseMode(10-22)src/omnibase_core/enums/enum_regex_flag.py (1)
EnumRegexFlag(10-22)src/omnibase_core/enums/enum_unicode_form.py (1)
EnumUnicodeForm(10-24)src/omnibase_core/enums/enum_trim_mode.py (1)
EnumTrimMode(10-22)
src/omnibase_core/models/transformations/model_transform_unicode_config.py (1)
src/omnibase_core/enums/enum_unicode_form.py (1)
EnumUnicodeForm(10-24)
src/omnibase_core/models/contracts/subcontracts/model_compute_subcontract.py (1)
src/omnibase_core/models/contracts/subcontracts/model_compute_pipeline_step.py (1)
ModelComputePipelineStep(20-90)
src/omnibase_core/models/transformations/model_transform_regex_config.py (1)
src/omnibase_core/enums/enum_regex_flag.py (1)
EnumRegexFlag(10-22)
src/omnibase_core/utils/compute_executor.py (5)
src/omnibase_core/enums/enum_compute_step_type.py (1)
EnumComputeStepType(11-27)src/omnibase_core/models/compute/model_compute_execution_context.py (1)
ModelComputeExecutionContext(44-95)src/omnibase_core/models/compute/model_compute_pipeline_result.py (1)
ModelComputePipelineResult(53-122)src/omnibase_core/models/compute/model_compute_step_metadata.py (1)
ModelComputeStepMetadata(14-26)src/omnibase_core/models/compute/model_compute_step_result.py (1)
ModelComputeStepResult(23-43)
tests/unit/utils/test_compute_pipeline.py (10)
src/omnibase_core/models/compute/model_compute_execution_context.py (1)
ModelComputeExecutionContext(44-95)src/omnibase_core/models/compute/model_compute_step_metadata.py (1)
ModelComputeStepMetadata(14-26)src/omnibase_core/models/compute/model_compute_step_result.py (1)
ModelComputeStepResult(23-43)src/omnibase_core/models/transformations/model_mapping_config.py (1)
ModelMappingConfig(13-29)src/omnibase_core/models/transformations/model_transform_case_config.py (1)
ModelTransformCaseConfig(15-27)src/omnibase_core/models/transformations/model_transform_json_path_config.py (1)
ModelTransformJsonPathConfig(13-25)src/omnibase_core/models/transformations/model_transform_regex_config.py (1)
ModelTransformRegexConfig(15-31)src/omnibase_core/models/transformations/model_transform_trim_config.py (1)
ModelTransformTrimConfig(15-27)src/omnibase_core/models/transformations/model_transform_unicode_config.py (1)
ModelTransformUnicodeConfig(15-27)src/omnibase_core/utils/compute_transformations.py (7)
execute_transformation(392-452)transform_case(157-203)transform_identity(76-99)transform_json_path(298-377)transform_regex(102-154)transform_trim(206-253)transform_unicode(256-295)
src/omnibase_core/enums/__init__.py (6)
src/omnibase_core/enums/enum_case_mode.py (1)
EnumCaseMode(10-22)src/omnibase_core/enums/enum_compute_step_type.py (1)
EnumComputeStepType(11-27)src/omnibase_core/enums/enum_regex_flag.py (1)
EnumRegexFlag(10-22)src/omnibase_core/enums/enum_transformation_type.py (1)
EnumTransformationType(11-34)src/omnibase_core/enums/enum_trim_mode.py (1)
EnumTrimMode(10-22)src/omnibase_core/enums/enum_unicode_form.py (1)
EnumUnicodeForm(10-24)
src/omnibase_core/models/compute/model_compute_step_result.py (1)
src/omnibase_core/models/compute/model_compute_step_metadata.py (1)
ModelComputeStepMetadata(14-26)
src/omnibase_core/mixins/mixin_compute_execution.py (4)
src/omnibase_core/models/compute/model_compute_execution_context.py (1)
ModelComputeExecutionContext(44-95)src/omnibase_core/models/compute/model_compute_pipeline_result.py (1)
ModelComputePipelineResult(53-122)src/omnibase_core/models/contracts/subcontracts/model_compute_subcontract.py (1)
ModelComputeSubcontract(16-51)src/omnibase_core/utils/compute_executor.py (1)
execute_compute_pipeline(379-539)
src/omnibase_core/models/transformations/model_transform_trim_config.py (1)
src/omnibase_core/enums/enum_trim_mode.py (1)
EnumTrimMode(10-22)
src/omnibase_core/models/contracts/subcontracts/__init__.py (2)
src/omnibase_core/models/contracts/subcontracts/model_compute_pipeline_step.py (1)
ModelComputePipelineStep(20-90)src/omnibase_core/models/contracts/subcontracts/model_compute_subcontract.py (1)
ModelComputeSubcontract(16-51)
src/omnibase_core/mixins/__init__.py (1)
src/omnibase_core/mixins/mixin_compute_execution.py (1)
MixinComputeExecution(49-204)
src/omnibase_core/models/compute/model_compute_pipeline_result.py (1)
src/omnibase_core/models/compute/model_compute_step_result.py (1)
ModelComputeStepResult(23-43)
src/omnibase_core/utils/compute_transformations.py (11)
src/omnibase_core/enums/enum_case_mode.py (1)
EnumCaseMode(10-22)src/omnibase_core/enums/enum_core_error_code.py (1)
EnumCoreErrorCode(9-154)src/omnibase_core/enums/enum_regex_flag.py (1)
EnumRegexFlag(10-22)src/omnibase_core/enums/enum_transformation_type.py (1)
EnumTransformationType(11-34)src/omnibase_core/enums/enum_trim_mode.py (1)
EnumTrimMode(10-22)src/omnibase_core/enums/enum_unicode_form.py (1)
EnumUnicodeForm(10-24)src/omnibase_core/models/transformations/model_transform_case_config.py (1)
ModelTransformCaseConfig(15-27)src/omnibase_core/models/transformations/model_transform_json_path_config.py (1)
ModelTransformJsonPathConfig(13-25)src/omnibase_core/models/transformations/model_transform_regex_config.py (1)
ModelTransformRegexConfig(15-31)src/omnibase_core/models/transformations/model_transform_trim_config.py (1)
ModelTransformTrimConfig(15-27)src/omnibase_core/models/transformations/model_transform_unicode_config.py (1)
ModelTransformUnicodeConfig(15-27)
src/omnibase_core/models/transformations/__init__.py (7)
src/omnibase_core/models/transformations/model_mapping_config.py (1)
ModelMappingConfig(13-29)src/omnibase_core/models/transformations/model_transform_case_config.py (1)
ModelTransformCaseConfig(15-27)src/omnibase_core/models/transformations/model_transform_json_path_config.py (1)
ModelTransformJsonPathConfig(13-25)src/omnibase_core/models/transformations/model_transform_regex_config.py (1)
ModelTransformRegexConfig(15-31)src/omnibase_core/models/transformations/model_transform_trim_config.py (1)
ModelTransformTrimConfig(15-27)src/omnibase_core/models/transformations/model_transform_unicode_config.py (1)
ModelTransformUnicodeConfig(15-27)src/omnibase_core/models/transformations/model_validation_step_config.py (1)
ModelValidationStepConfig(13-30)
🪛 LanguageTool
docs/architecture/NODECOMPUTE_VERSIONING_ROADMAP.md
[style] ~55-~55: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...itions. Boring code is debuggable code. Boring APIs are learnable APIs. Stable: O...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🪛 markdownlint-cli2 (0.18.1)
docs/architecture/NODECOMPUTE_VERSIONING_ROADMAP.md
106-106: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
132-132: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
927-927: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
docs/architecture/CONTRACT_DRIVEN_NODECOMPUTE_V1_0.md
482-482: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
503-503: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
677-677: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
docs/architecture/NODECOMPUTE_FULL_DESIGN_V1X_TARGET.md
849-849: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
1318-1318: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1323-1323: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1328-1328: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1333-1333: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1338-1338: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1343-1343: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1348-1348: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1366-1366: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
1879-1879: Multiple headings with the same content
(MD024, no-duplicate-heading)
⏰ 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: claude-review
- GitHub Check: Smoke Tests
🔇 Additional comments (38)
src/omnibase_core/enums/enum_trim_mode.py (1)
7-22: LGTM!The enum correctly defines the three standard trim modes with lowercase string values appropriate for JSON serialization. The
(str, Enum)pattern ensures proper serialization.Same optional consistency note: consider adding
__all__to match other modules with explicit exports.tests/unit/enums/test_compute_enums.py (1)
12-111: Test implementations are correct and comprehensive.The test assertions correctly validate:
- Enum values match their string representations
- All members are string types (ensuring JSON serialization compatibility)
- Member counts for core step/transformation types
The tests effectively verify the v1.0 enum contracts.
src/omnibase_core/mixins/__init__.py (1)
27-79: MixinComputeExecution import and export wiring looks correctImporting
MixinComputeExecutionand exposing it via__all__is consistent with how other mixins are surfaced here, so projects can now depend on it through the standardomnibase_core.mixinsnamespace.src/omnibase_core/enums/enum_regex_flag.py (1)
10-22: EnumRegexFlag definition aligns well with regex transformation needsThe string-backed enum with
IGNORECASE,MULTILINE, andDOTALLis straightforward and readable, and the docstring clearly ties it to the REGEX transformation behavior.src/omnibase_core/enums/enum_case_mode.py (1)
10-22: EnumCaseMode is clear and consistent with case-conversion configsUsing human-readable values (
"uppercase","lowercase","titlecase") for the string enum keeps contracts easy to read while remaining type-safe forModelTransformCaseConfig.src/omnibase_core/enums/enum_compute_step_type.py (1)
11-27: EnumComputeStepType cleanly encodes the v1.0 step surfaceThe three published members (
VALIDATION,TRANSFORMATION,MAPPING) and the docstring notes about deferredCONDITIONAL/PARALLELsteps line up with the v1.0 contract design and keep the future roadmap visible in code.src/omnibase_core/models/transformations/model_transform_trim_config.py (1)
15-27: Trim config model matches the transformation-config pattern
ModelTransformTrimConfiguses a clear discriminator, sensible default (EnumTrimMode.BOTH), and an immutableConfigDict, consistent with the other v1.0 transformation configs and the Pydantic immutability guidance. Based on learnings, this fits the expected model pattern.src/omnibase_core/models/transformations/model_validation_step_config.py (1)
13-30: LGTM! Clean and well-documented validation config model.The model correctly implements the discriminated union pattern with proper immutability guarantees (
frozen=True) and strict validation (extra="forbid"). The docstring helpfully clarifies that VALIDATION is a step type, not a transformation.src/omnibase_core/models/transformations/model_transform_unicode_config.py (1)
15-27: LGTM! Properly configured Unicode normalization model.The model follows the established transformation config pattern with appropriate defaults (NFC is the recommended normalization form) and immutability guarantees.
src/omnibase_core/enums/enum_transformation_type.py (1)
11-34: LGTM! Well-documented transformation type enum for v1.0.The enum clearly defines the 6 supported transformation types with helpful comments indicating future expansion plans. The string-backed values and comprehensive docstrings make this easy to use.
docs/architecture/NODECOMPUTE_VERSIONING_ROADMAP.md (1)
1-993: Excellent roadmap documentation for NodeCompute versioning strategy.This comprehensive document clearly articulates the "boring first, earned complexity" philosophy and provides detailed versioning guarantees. The breakdown of what's included in each version (v1.0-v1.3) and the explicit "NOT in 1.x" section help set clear expectations.
src/omnibase_core/models/transformations/types.py (1)
19-28: LGTM! Properly implemented discriminated union for transformation configs.The union correctly excludes IDENTITY (which has no config) and uses Pydantic's discriminated union feature with the
config_typefield. This enables type-safe parsing and validation of transformation configurations.tests/unit/utils/test_compute_pipeline.py (1)
1-573: Excellent comprehensive test coverage for NodeCompute v1.0.The test suite thoroughly exercises all transformation types, error handling, pipeline execution, and path resolution. Tests are well-organized into logical classes with descriptive names and cover both success and failure scenarios.
src/omnibase_core/models/contracts/subcontracts/__init__.py (1)
37-38: LGTM! Clean addition of compute subcontract models to public API.The imports and exports are properly organized with a clear section comment, maintaining alphabetical ordering and consistency with the existing package structure.
Also applies to: 115-117
src/omnibase_core/models/transformations/model_transform_regex_config.py (1)
15-31: LGTM! Well-designed regex transformation config.The model correctly uses
Field(default_factory=list)for the mutable default, provides sensible defaults (empty replacement for deletion), and maintains immutability withfrozen=True. The integration withEnumRegexFlagprovides type-safe flag handling.src/omnibase_core/models/contracts/subcontracts/model_compute_subcontract.py (1)
1-51: LGTM overall!The model correctly implements v1.0 subcontract semantics with proper immutability (
frozen=True) and strict validation (extra="forbid"). The docstring clearly documents abort-on-first-failure semantics and all attributes. As per coding guidelines, the model follows Pydantic 2.11+ patterns.src/omnibase_core/models/compute/model_compute_step_result.py (2)
46-51: Forward reference resolution pattern is correct.The runtime import with
model_rebuild()is the standard Pydantic approach for resolving forward references whenTYPE_CHECKINGis used. Thenoqa: E402suppression is appropriate here.
36-43: Model structure looks good.The model correctly uses
frozen=Truefor immutability andextra="forbid"for strict validation. The v1.0 string-based error type aligns with the design decision documented in the specification.src/omnibase_core/models/compute/__init__.py (1)
1-29: Clean package initialization.The module correctly re-exports all compute models with a proper
__all__declaration. The docstring provides a helpful overview of the public API surface.src/omnibase_core/models/transformations/model_mapping_config.py (1)
13-29: Model structure is correct.The discriminator pattern with
config_type: Literal["mapping"]enables proper union type resolution. The model follows Pydantic 2.11+ patterns with immutability enforced viafrozen=True.docs/architecture/CONTRACT_DRIVEN_NODECOMPUTE_V1_0.md (1)
1-921: Comprehensive and well-structured specification.The document provides excellent coverage of v1.0 scope, design principles, and implementation plan. The explicit deferral of features (caching, parallelism, conditionals) to later versions demonstrates good incremental design. The abort-on-first-failure semantics and immutability guarantees are clearly documented.
tests/unit/models/compute/test_compute_models.py (1)
62-174: LGTM - Good test coverage for compute models.The tests comprehensively cover:
- Minimal and full construction scenarios
- Success and failure paths with appropriate field validation
- Immutability enforcement for all models
The test logic correctly validates the model contracts defined in the relevant code snippets.
src/omnibase_core/enums/__init__.py (2)
23-25: LGTM - Clean enum imports for NodeCompute v1.0.The new enum imports are correctly organized with appropriate domain comments. The alphabetical placement maintains consistency with the existing codebase structure.
339-346: LGTM - Proper public API exposure.The
__all__exports correctly match the new imports and are logically grouped under the v1.0 domain comment.src/omnibase_core/mixins/mixin_compute_execution.py (2)
87-144: Sync function called in async context may block event loop.The
execute_contract_pipelinemethod is markedasyncbut calls the synchronousexecute_compute_pipelinedirectly without usingasyncio.to_thread(). This can block the event loop during pipeline execution.For v1.0 with simple transformations this may be acceptable, but consider documenting this limitation or wrapping with
asyncio.to_threadfor true async compatibility.+import asyncio + async def execute_contract_pipeline( self, contract: ModelComputeSubcontract, input_data: Any, correlation_id: "UUID | None" = None, ) -> ModelComputePipelineResult: # ... docstring ... context = ModelComputeExecutionContext( operation_id=uuid4(), correlation_id=correlation_id, node_id=str(self.node_id) if hasattr(self, 'node_id') else None, ) - result = execute_compute_pipeline(contract, input_data, context) + # Use to_thread to avoid blocking the event loop + result = await asyncio.to_thread( + execute_compute_pipeline, contract, input_data, context + ) return result
183-202: LGTM - Solid static validation logic.The validation correctly detects:
- Duplicate step names using set comparison
- Forward references in mapping paths by tracking executed steps
The validation scope is appropriate for v1.0. Additional checks (empty step names, path syntax validation) could be added in future versions.
tests/integration/test_compute_pipeline_integration.py (1)
62-610: LGTM - Comprehensive integration test coverage.Excellent coverage of pipeline execution scenarios including:
- Multi-step transformations with chaining
- Abort-on-first-failure error semantics
- Mapping with cross-step references
- Disabled step handling
- Performance baselines with
@pytest.mark.slow- Timing verification
The tests effectively validate the v1.0 contract-driven pipeline behavior.
src/omnibase_core/models/contracts/subcontracts/model_compute_pipeline_step.py (2)
20-62: LGTM - Well-designed pipeline step model.The model correctly implements v1.0 requirements:
- Clear field documentation with step type requirements
- Appropriate use of optional fields for type-specific configs
frozen=Trueensures immutability as per coding guidelinesextra="forbid"prevents unrecognized fieldsThe docstring accurately describes the configuration requirements for each step type.
63-90: LGTM - Robust step configuration validation.The validator correctly enforces the v1.0 contract:
- TRANSFORMATION requires
transformation_type- Non-IDENTITY transformations require
transformation_config- IDENTITY transformations must NOT have
transformation_config(key design decision)- MAPPING requires
mapping_config- VALIDATION requires
validation_configThe validation will fail fast at model construction time, preventing invalid pipeline configurations from reaching runtime.
src/omnibase_core/models/compute/model_compute_pipeline_result.py (1)
53-123: Pipeline result model looks consistent and thread‑safeThe structure, field types, and frozen
model_configalign with the v1.0 executor behavior and step result model; the forward‑ref wiring viamodel_rebuild()is also correct for Pydantic 2.src/omnibase_core/utils/compute_executor.py (5)
75-121: Error type extraction is reasonable for v1.0 string error_type
_get_error_typereturning the underlyingerror_code.value(or"transformation_error"when absent) gives stable, debuggable identifiers for both step and pipelineerror_typefields, and fits the “string only in v1.0” contract.
141-211: Mapping path resolver matches v1.0 scope but is intentionally strictThe resolver correctly enforces
$‑prefixed paths, supports$.input.*and$.steps.<name>.output, and fails fast withModelOnexErroron invalid prefixes, missing keys, or private attributes. This matches the documented v1.0 mapping semantics and is appropriately defensive.
214-257: Mapping step execution is clear and defers all resolution errors to the pipelineThe mapping step handler validates
mapping_configpresence and then delegates each field toresolve_mapping_path, letting anyModelOnexErrorbubble to the main executor so failures are attributed to the correct step. This keeps the logic small and predictable.
260-315: Validation step’s v1.0 pass‑through behavior is well‑scopedRequiring
validation_configand logging a warning while returning the data unchanged makes the v1.0 “no real validation yet” behavior explicit without surprising callers; the TODO notes for v1.1 schema validation are clear.
379-539: Pipeline executor correctly implements abort‑on‑first‑failure semanticsThe executor:
- Skips disabled steps,
- Measures per‑step and total durations,
- Records a
ModelComputeStepResultfor every executed step (success or failure),- Aborts immediately on the first
ModelOnexErroror unexpectedException,- Returns a
ModelComputePipelineResultwhose contents match the model’s docstring.Catching generic
Exceptionand converting it into an"unexpected_error"result rather than re‑raising also aligns with the goal of returning structured failures from pure compute utilities.src/omnibase_core/utils/compute_transformations.py (2)
102-389: Transformation implementations and registry wiring look solid
- Each string‑based transform (regex, case, trim, unicode) validates input type and raises
ModelOnexErrorwith appropriateEnumCoreErrorCode.- Regex flags mapping is correct for
IGNORECASE,MULTILINE, andDOTALL, and invalid patterns are wrapped asOPERATION_FAILED.transform_json_pathgives a clear, documented v1.0 subset (dot‑notation only) with good error messages and private‑attribute blocking.TRANSFORMATION_REGISTRYenumerates exactly the six v1.0EnumTransformationTypevalues and maps them to the correct handlers.This module cleanly separates pure transformations from pipeline orchestration and matches the contract models shown in the PR.
392-453:execute_transformationdispatch and validation match the v1.0 contractSpecial‑casing IDENTITY, validating that a handler exists for the given
EnumTransformationType, and requiring non‑None config for all non‑IDENTITY types is exactly what the v1.0 contract describes. Errors are surfaced viaModelOnexErrorwith clear messages, keeping pipeline‑level error propagation straightforward.src/omnibase_core/models/transformations/__init__.py (1)
1-40: Transformation config package exports are clean and consistentThe
__init__nicely centralizes all v1.0 transformation and step config models plus the discriminated union, with documentation that matches the actual imports and__all__. This will make downstream imports (executor, mixins, tests) straightforward.
Address all PR #115 review feedback to prepare for release: Documentation: - Update ModelValidationConfig → ModelValidationStepConfig in docs - Add language specifiers to all code fences - Remove obsolete NODECOMPUTE_FULL_DESIGN_V1X_TARGET.md Model Validations: - Add duration_ms >= 0 constraint on ModelComputeStepMetadata - Add JSONPath validation (must start with '$') - Add pipeline_timeout_ms > 0 validation when provided - Add field_mappings non-empty validation Test Improvements: - Add @pytest.mark.unit to all unit test classes - Add @pytest.mark.integration to integration test classes - Use ValidationError instead of generic Exception for frozen model tests - Add test_member_count for all enum test classes - Add comprehensive ModelComputeSubcontract tests Code Quality: - Align transform_identity signature with other handlers - Add TODO(v1.1) comments for future TransformationError - Verify security checks for private attribute access
Code Review: Contract-Driven NodeCompute v1.0 [OMN-465]SummaryThis is an excellent, production-ready implementation that delivers a clean, well-architected foundation for contract-driven compute pipelines. The PR demonstrates strong adherence to ONEX principles and best practices. ✅ Strengths1. Architecture & Design
2. Code Quality
3. Testing
4. Developer Experience
🔍 Detailed ReviewCode Organization (37 files, +6,525/-0)Enums (6 files): ✅ Clean, well-documented enumerations
Models (16 files): ✅ Excellent Pydantic usage
Execution Layer (3 files): ✅ Pure, testable, thread-safe
Documentation (3 files): ✅ Comprehensive and actionable
🎯 Architectural Compliance✅ Adheres to CLAUDE.md Standards
✅ Follows ONEX Patterns
💡 Minor Observations (Not Blocking)1. Path Resolution DuplicationThe path resolution logic appears twice (compute_executor.py:100-211 and compute_transformations.py:310-389). Consider extracting to a shared utility in v1.1 to maintain DRY principle. Impact: Low - code is identical and well-tested 2. Validation Step is Pass-Throughv1.0 validation steps log warnings but don't validate (compute_executor.py:306-311). This is documented and intentional for v1.0. Impact: None - clearly documented limitation 3. Error Type Strings vs Enumsv1.0 uses string error types instead of enums. Per the spec, this is intentional. Impact: None - v1.1 migration path is documented 4. Timeout Enforcement
Impact: Low - sequential pipelines are typically fast 🧪 Testing AssessmentTest Coverage: ✅ ExcellentUnit Tests (1,261 lines across 5 files):
Integration Tests (817 lines):
Strengths:
Recommendation: Run 🔒 Security Review✅ No security concerns identified Positive findings:
📊 Performance Considerations✅ No performance issues expected Positive patterns:
Future optimizations (deferred appropriately):
🎓 Recommendations for Next Steps
✅ Final VerdictAPPROVED - This PR is ready to merge pending CI checks. Rationale:
Confidence Level: High - this is production-ready code. 📝 Nitpicks (Optional, Non-Blocking)
Great work on this foundation! 🎉 The disciplined scope, excellent documentation, and clean implementation make this a model PR for the ONEX framework. Generated with Claude Code |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
docs/architecture/NODECOMPUTE_VERSIONING_ROADMAP.md (2)
51-57: Consider rephrasing for improved readability (optional).The repetition of "Boring" at the start of three consecutive sentences is flagged by style checkers. While this appears intentional for emphasis, consider slight variation for improved flow.
106-112: Add language specifiers to fenced code blocks (optional).Per Markdown best practices, fenced code blocks should specify a language. For non-code content like version diagrams and text blocks, consider using
textorplaintext.Also applies to: 132-137, 927-931
tests/integration/test_compute_pipeline_integration.py (1)
520-562: Consider adding explicit timeout decorators to slow tests.Per coding guidelines, tests should use
@pytest.mark.timeout()to prevent hangs. While these tests have performance assertions, explicit timeouts would provide better protection in CI environments.Example:
+@pytest.mark.timeout(5) @pytest.mark.slow def test_large_data_performance_baseline(self) -> None:Based on learnings, adding timeouts prevents test hangs and improves CI reliability.
Also applies to: 564-610
src/omnibase_core/utils/compute_transformations.py (2)
310-390: Consider documenting the private attribute handling behavior.The implementation correctly handles path traversal with good error messages. However, the private attribute blocking (line 376-380) only applies to object attributes, not dictionary keys. This means:
transform_json_path({"_private": "value"}, ...)→ allowedtransform_json_path(obj_with_private_attr, ...)→ blockedThis is a reasonable design decision (dict keys are not "private attributes"), but the distinction could be documented in the docstring for clarity.
Consider adding a note to the docstring:
Raises: ModelOnexError: If the path cannot be resolved (key missing, attribute not found) - or if attempting to access private attributes (those starting with "_"). + or if attempting to access private object attributes (those starting with "_"). + Note: Dictionary keys starting with "_" are allowed, as they are not considered + private in the same way as object attributes.
404-465: Consider simplifying the IDENTITY special case.The function special-cases
IDENTITY(line 448-449) before using the registry, which means the registry entry forIDENTITY(line 395) is never used. This redundancy could be eliminated by:Option 1: Remove the special case and adjust config validation
Option 2: Remove IDENTITY from the registry since it's always special-casedThe current approach works correctly and makes the "no config required" behavior explicit, but it creates a maintenance burden (IDENTITY appears in two places with different code paths).
If you prefer to keep the special case for clarity, consider adding a comment:
if transformation_type == EnumTransformationType.IDENTITY: + # Special-cased to bypass config validation (IDENTITY requires config=None) return transform_identity(data, config=None)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
docs/architecture/CONTRACT_DRIVEN_NODECOMPUTE_V1_0.md(1 hunks)docs/architecture/NODECOMPUTE_VERSIONING_ROADMAP.md(1 hunks)examples/contracts/compute/user_profile_normalizer.yaml(1 hunks)src/omnibase_core/models/compute/model_compute_step_metadata.py(1 hunks)src/omnibase_core/models/contracts/subcontracts/model_compute_subcontract.py(1 hunks)src/omnibase_core/models/transformations/model_mapping_config.py(1 hunks)src/omnibase_core/models/transformations/model_transform_json_path_config.py(1 hunks)src/omnibase_core/utils/compute_transformations.py(1 hunks)tests/integration/test_compute_pipeline_integration.py(1 hunks)tests/unit/enums/test_compute_enums.py(1 hunks)tests/unit/models/compute/test_compute_models.py(1 hunks)tests/unit/models/contracts/subcontracts/test_model_compute_subcontract.py(1 hunks)tests/unit/models/transformations/test_transformation_configs.py(1 hunks)tests/unit/utils/test_compute_pipeline.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- tests/unit/enums/test_compute_enums.py
- docs/architecture/CONTRACT_DRIVEN_NODECOMPUTE_V1_0.md
- tests/unit/models/transformations/test_transformation_configs.py
- tests/unit/models/compute/test_compute_models.py
- src/omnibase_core/models/contracts/subcontracts/model_compute_subcontract.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. Usepoetry runfor pytest, mypy, black, isort, and other tools. Never use pip directly or execute python commands without the poetry run prefix.
Enforce 100% strict mypy compliance. All functions must have complete type annotations. Usepoetry run mypy src/omnibase_core/to verify strict mode compliance with no errors.
Format code with black, sort imports with isort, and lint with ruff. Runpoetry run black src/ tests/,poetry run isort src/ tests/, andpoetry run ruff check --fix src/ tests/
Files:
tests/unit/models/contracts/subcontracts/test_model_compute_subcontract.pytests/integration/test_compute_pipeline_integration.pysrc/omnibase_core/models/transformations/model_mapping_config.pytests/unit/utils/test_compute_pipeline.pysrc/omnibase_core/models/transformations/model_transform_json_path_config.pysrc/omnibase_core/models/compute/model_compute_step_metadata.pysrc/omnibase_core/utils/compute_transformations.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Write tests using pytest with async support via pytest-asyncio. Maintain minimum 60% code coverage. Use pytest markers: @pytest.mark.unit, @pytest.mark.integration, @pytest.mark.slow
Add @pytest.mark.timeout() to prevent test hangs. Run tests withpoetry run pytest tests/ --timeout=60for 60-second timeout protection.
Files:
tests/unit/models/contracts/subcontracts/test_model_compute_subcontract.pytests/integration/test_compute_pipeline_integration.pytests/unit/utils/test_compute_pipeline.py
src/omnibase_core/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/omnibase_core/**/*.py: Use protocol names withcontainer.get_service()for dependency injection, never resolve services by concrete class names. Example:container.get_service('ProtocolEventBus')instead ofcontainer.get_service('EventBusService')
Always use ModelONEXContainer for dependency injection in node constructors and service resolution. Never confuse it with ModelContainer[T], which is a generic value wrapper, not a DI container.
Use structured error handling with ModelOnexError and EnumCoreErrorCode, never raise generic Exception. Always chain exceptions withfrom ewhen catching and re-raising.
Use the @standard_error_handling decorator on methods to eliminate 6+ lines of try/catch boilerplate. Example:@standard_error_handlingabove async methods.
Respect thread safety constraints: Do NOT share node instances (NodeCompute, NodeEffect), cache, or circuit breaker instances across threads without synchronization. Use thread-local instances or locks.
Use duck typing with protocols. Do not use isinstance checks with protocol types. Rely on the protocol interface directly.
Use EnumNodeKind for high-level architectural classification (EFFECT, COMPUTE, REDUCER, ORCHESTRATOR) when routing data through the ONEX pipeline. Use EnumNodeType for specific implementation discovery and capability matching.
Use ModelEventEnvelope for inter-service event-driven communication. Process events through the envelope pattern for consistent event handling.
Files:
src/omnibase_core/models/transformations/model_mapping_config.pysrc/omnibase_core/models/transformations/model_transform_json_path_config.pysrc/omnibase_core/models/compute/model_compute_step_metadata.pysrc/omnibase_core/utils/compute_transformations.py
src/omnibase_core/models/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use Pydantic 2.11+ models for all data validation and serialization. Ensure models are immutable after creation for thread safety.
Files:
src/omnibase_core/models/transformations/model_mapping_config.pysrc/omnibase_core/models/transformations/model_transform_json_path_config.pysrc/omnibase_core/models/compute/model_compute_step_metadata.py
🧠 Learnings (3)
📚 Learning: 2025-12-06T23:34:11.788Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T23:34:11.788Z
Learning: Applies to tests/**/*.py : Write tests using pytest with async support via pytest-asyncio. Maintain minimum 60% code coverage. Use pytest markers: pytest.mark.unit, pytest.mark.integration, pytest.mark.slow
Applied to files:
tests/unit/utils/test_compute_pipeline.py
📚 Learning: 2025-12-06T23:34:11.788Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T23:34:11.788Z
Learning: Applies to tests/**/*.py : Add pytest.mark.timeout() to prevent test hangs. Run tests with `poetry run pytest tests/ --timeout=60` for 60-second timeout protection.
Applied to files:
tests/unit/utils/test_compute_pipeline.py
📚 Learning: 2025-12-06T23:34:11.788Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T23:34:11.788Z
Learning: Applies to docs/guides/node-building/**/*.md : Node building guide is the critical starting point for developers and AI agents. Follow the sequence: What is a Node → Node Types → COMPUTE Tutorial → EFFECT Tutorial → REDUCER Tutorial → ORCHESTRATOR Tutorial.
Applied to files:
docs/architecture/NODECOMPUTE_VERSIONING_ROADMAP.md
🧬 Code graph analysis (2)
tests/unit/models/contracts/subcontracts/test_model_compute_subcontract.py (2)
src/omnibase_core/models/contracts/subcontracts/model_compute_pipeline_step.py (1)
ModelComputePipelineStep(20-90)src/omnibase_core/models/contracts/subcontracts/model_compute_subcontract.py (1)
ModelComputeSubcontract(16-51)
tests/integration/test_compute_pipeline_integration.py (4)
src/omnibase_core/models/contracts/subcontracts/model_compute_pipeline_step.py (1)
ModelComputePipelineStep(20-90)src/omnibase_core/models/contracts/subcontracts/model_compute_subcontract.py (1)
ModelComputeSubcontract(16-51)src/omnibase_core/models/transformations/model_mapping_config.py (1)
ModelMappingConfig(13-37)src/omnibase_core/utils/compute_executor.py (1)
execute_compute_pipeline(379-539)
🪛 LanguageTool
docs/architecture/NODECOMPUTE_VERSIONING_ROADMAP.md
[style] ~55-~55: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...itions. Boring code is debuggable code. Boring APIs are learnable APIs. Stable: O...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🪛 markdownlint-cli2 (0.18.1)
docs/architecture/NODECOMPUTE_VERSIONING_ROADMAP.md
106-106: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
132-132: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
927-927: 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: claude-review
- GitHub Check: Smoke Tests
🔇 Additional comments (15)
src/omnibase_core/models/compute/model_compute_step_metadata.py (1)
14-26: LGTM! Clean and well-structured metadata model.The implementation correctly enforces immutability and non-negative duration constraints, aligning with the v1.0 design principles from the PR objectives.
src/omnibase_core/models/transformations/model_mapping_config.py (1)
13-37: LGTM! Well-designed mapping configuration model.The model correctly implements:
- Discriminated union pattern with
config_type- Non-empty validation for field_mappings
- Immutability via
frozen=True- Clear documentation of path expression syntax
tests/unit/utils/test_compute_pipeline.py (1)
60-582: LGTM! Comprehensive unit test coverage.The test suite thoroughly validates all transformation types, the dispatch mechanism, path resolution, and pipeline execution semantics. All test classes are properly marked with
@pytest.mark.unitper coding guidelines.tests/unit/models/contracts/subcontracts/test_model_compute_subcontract.py (1)
16-156: LGTM! Thorough validation test coverage.The test suite comprehensively validates:
- Minimal and full contract creation
- Timeout constraint enforcement (None, positive, zero, negative edge cases)
- Immutability guarantees
- Extra field rejection
All tests properly use
@pytest.mark.unitper coding guidelines.src/omnibase_core/models/transformations/model_transform_json_path_config.py (1)
13-35: LGTM! Correct JSONPath configuration implementation.The model properly validates JSONPath syntax requirements:
- Non-empty path
- Must start with "$" (JSONPath root)
- Immutable via
frozen=True- Follows discriminated union pattern
examples/contracts/compute/user_profile_normalizer.yaml (1)
1-358: LGTM! Exemplary documentation and contract example.This is an outstanding example contract that:
- Demonstrates all 3 step types (VALIDATION, TRANSFORMATION, MAPPING)
- Shows 5 of 6 transformation types in realistic scenarios
- Includes detailed inline documentation explaining each step
- Provides concrete input/output examples
- Shows proper path resolution patterns
- Demonstrates the sequential execution model with abort-on-first-failure
This will serve as an excellent reference for developers implementing NodeCompute contracts.
tests/integration/test_compute_pipeline_integration.py (1)
52-814: LGTM! Excellent integration test coverage.The test suite comprehensively validates:
- Multi-step pipeline execution and chaining
- Abort-on-first-failure semantics with error propagation
- Complex mapping scenarios referencing multiple prior steps
- Disabled step handling
- Regex flags and unicode normalization integration
- Performance baselines for large data and many steps
- Error scenarios (invalid regex, missing paths, type mismatches)
- Context propagation with operation_id and correlation_id
All test classes properly use
@pytest.mark.integrationper coding guidelines.src/omnibase_core/utils/compute_transformations.py (8)
1-82: LGTM! Well-structured module with clear documentation.The module structure is excellent:
- Comprehensive docstring with thread safety guarantees
- Well-documented type aliases for JSON structures and handler functions
- Proper import organization (stdlib then project imports)
- Helpful future planning comment about TransformationError
The decision to use
Callable[..., Any]forTransformationHandlerappropriately balances type safety with flexibility for handlers with varying signatures.
84-111: LGTM! Clean identity transformation.The intentional unused
configparameter is well-justified in the docstring and inline comment for registry handler signature consistency.
114-167: LGTM! Robust regex transformation with proper error handling.The implementation correctly:
- Validates string input type
- Maps enum flags to Python re flags (enum guarantees only valid values)
- Chains exceptions with
from efollowing coding guidelines- Uses structured error codes (VALIDATION_ERROR, OPERATION_FAILED)
169-216: LGTM! Clean case transformation with exhaustive mode handling.Proper input validation and comprehensive mode coverage with appropriate error handling for unexpected cases.
218-266: LGTM! Clean trim transformation following consistent patterns.Mirrors the case transformation structure with proper validation and mode handling.
268-308: LGTM! Unicode normalization with enum-guaranteed correctness.The implementation correctly delegates form validation to the enum type system, trusting that
EnumUnicodeForm.valueprovides valid Unicode normalization forms.
392-401: LGTM! Complete transformation registry.The registry correctly maps all transformation types to their handlers with proper type annotations.
467-481: LGTM! Complete and well-organized public API exports.The
__all__list properly exposes type aliases, transformation functions, and the registry with clear section comments.
- Add TODO(v1.1) comments for deferred features: - Path resolution duplication (extract to shared utility) - Pipeline timeout enforcement (currently declared but not enforced) - TransformationError custom exception consideration - Align transform_identity signature with registry pattern (Any = None) - Fix markdown formatting (MD031/MD032 blank line issues) All 993+ tests pass, mypy --strict clean.
Code Review: Contract-Driven NodeCompute v1.0 [OMN-465]OverviewThis is an excellent, well-architected implementation of the v1.0 contract-driven NodeCompute specification. The PR delivers exactly what it promises: a minimal, stable foundation for declarative compute pipelines with sequential execution, abort-on-first-failure semantics, and 6 built-in transformations. Summary: ✅ APPROVED with minor recommendations for future iterations. Strengths1. Architecture & Design Excellence ⭐
2. Code Quality ⭐
3. Testing ⭐
4. ONEX Compliance ⭐
Issues & RecommendationsCritical Issues: None ✅No blocking issues found. The implementation is production-ready for v1.0 scope. Minor Issues & Suggestions1. TODO Comments - Schema Validation Not ImplementedLocation: # TODO(v1.1): Implement actual schema validation against schema_ref
def execute_validation_step(
data: Any,
config: ModelValidationStepConfig,
context: ModelComputeExecutionContext,
) -> Any:
"""v1.0: Validation steps are pass-through."""
return dataConcern: The VALIDATION step type is currently a no-op pass-through. This means:
Recommendation:
Impact: Medium - Could cause confusion in production use 2. Pipeline Timeout Not EnforcedLocation: # TODO(v1.1): Enforce pipeline_timeout_ms - currently declared in contract but not enforced.Concern:
Recommendation:
Impact: Low-Medium - Timeout is listed in v1.0 scope table but not implemented 3. Path Resolution DuplicationLocation: # TODO(v1.1): Extract path resolution to shared utility - duplicated in compute_transformations.pyObservation: Path resolution logic appears in both Recommendation:
Impact: Low - Tech debt but doesn't affect functionality 4. Transformation Error HandlingLocation: # TODO(v1.1): Create TransformationError for more specific error handling.Observation: All transformation errors use generic Assessment: This is fine for v1.0. The TODO comment is good documentation. Recommendation: Keep as-is, but ensure error messages are clear and actionable. Performance Considerations✅ No concerns for v1.0:
Future (v1.2+): When adding parallelism, consider:
Security Review✅ No security concerns found:
Note: Regex DoS protection is implicit (Python's Test Coverage AssessmentCurrent: 95 unit tests + integration tests Coverage gaps (not blocking, but worth tracking):
Recommendation: Add these as test cases in v1.1 cleanup sprint. Documentation Quality ⭐⭐⭐Outstanding work on documentation: Architecture Docs
Code Documentation
Minor suggestion: Add a migration guide for existing NodeCompute users (code-driven → contract-driven). Alignment with ONEX Standards
File-by-File ReviewEnums (6 files) ✅
Models (16 files) ✅
Execution (3 files) ✅
Tests (5 files) ✅
CI/CD StatusObservation: PR description shows:
Recommendation: Wait for CI to pass before merge. Given the PR adds 6,542 lines, ensure:
Final RecommendationVerdict: ✅ APPROVE (with minor follow-ups)This is excellent work that demonstrates:
Pre-Merge Checklist
Post-Merge Follow-Ups (v1.1)
SummaryThis PR successfully delivers the v1.0 contract-driven NodeCompute specification with:
The implementation is production-ready for the defined v1.0 scope. The minor issues identified are primarily about ensuring declared features work as expected (validation, timeouts) rather than fundamental design flaws. Great work, @jonahgabriel! 🎉 Reviewed by: Claude Code (Sonnet 4.5) |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
src/omnibase_core/utils/compute_executor.py (4)
75-97: Default"transformation_error"label may be too specific for genericModelOnexErrorcases
_get_error_type(Line 75) uses"transformation_error"as the fallback whenerror.error_codeisNone. Since this helper is used at the pipeline level (Line 478 and Line 501) for anyModelOnexError, not just transformation failures, this label might be slightly misleading if a future caller raises aModelOnexErrorwithout an error_code from non-transformation paths.Consider using a more generic fallback (e.g.
"unknown_error"orerror.__class__.__name__) to avoid over-constraining semantics for future callers, or at least documenting that aNoneerror_code is interpreted as a transformation failure.
103-214: Path resolution semantics are clear and guarded; behavior for$.steps.<name>vs$.steps.<name>.outputis slightly asymmetric
resolve_mapping_path(Lines 103-214) is well-structured: it strictly enforces a leading$, blocks private attribute access (Line 170), and distinguishes validation vs operation errors cleanly.One nuance:
$.steps.<step_name>(with no.output) returnsresult.output(Lines 198-200), while$.steps.<step_name>.outputis the only explicitly documented form (Lines 19-24 and 118-123), and more complex suffixes like$.steps.<step_name>.output.fieldare rejected (Lines 202-209). That’s perfectly fine for v1.0, but it’s worth:
- Documenting this convenience behavior for bare
$.steps.<step_name>if you intend to keep it, or- Disallowing it in v1.x to keep the surface area minimal until nested step-output paths are supported.
Either option is small and non-blocking, but clarifying it will make contract authors’ expectations more predictable.
263-317: Validation pass-through warning may be noisy at scale
execute_validation_steplogs a warning every time a validation step runs (Lines 309-314), even though v1.0 intentionally treats validation as pass-through. In a high-throughput environment, this could spam logs for entirely expected behavior.Consider either:
- Downgrading this to
logger.infoorlogger.debug, or- Guarding with a feature flag / configuration so you can enable it only during rollout or debugging.
The strict requirement that
validation_configis present (Lines 297-301) is good and should remain.
385-545: Pipeline executor logic is solid; consider wiringcontextand unexpected-error observabilityThe main pipeline loop in
execute_compute_pipeline(Lines 441-537) cleanly enforces:
- Skip disabled steps (Lines 441-443),
- Abort-on-first-failure with structured
ModelOnexErrorhandling (Lines 473-505),- A last-resort catch for unexpected exceptions (Lines 506-535),
- Per-step and total timing via
ModelComputeStepMetadataandprocessing_time_ms.A couple of small, non-blocking refinements to consider:
contextis currently unused in this function. IfModelComputePipelineResultdoes not already carryoperation_id/correlation_id, consider adding them or at least usingcontextin structured logging around failures so callers can tie pipelines back to the broader ONEX traces.- For the generic
except Exceptionblock (Lines 506-535), you currently only capturestr(e). If you have a centralized logging layer, emitting the stack trace there (with the sameoperation_id) would improve debuggability while still returning a safe"unexpected_error"to callers.Functionally, the executor looks correct and aligns with the v1.0 design.
src/omnibase_core/utils/compute_transformations.py (3)
115-267: String transformation functions are consistent; consider DRY-ing the input-type validation
transform_regex,transform_case, andtransform_trim(Lines 115-267) all:
- Enforce
strinput withModelOnexError+EnumCoreErrorCode.VALIDATION_ERROR, and- Raise
OPERATION_FAILEDon configuration issues (invalid regex, unknown modes).This is correct and matches the error-handling guidelines. The only minor downside is the repeated
if not isinstance(data, str)pattern and near-identical error messages.If you find these functions growing further, consider a tiny internal helper like:
+def _ensure_str_input(name: str, data: Any) -> str: + if not isinstance(data, str): + raise ModelOnexError( + error_code=EnumCoreErrorCode.VALIDATION_ERROR, + message=f"{name} transformation requires string input, got {type(data).__name__}", + ) + return dataand then use
_ensure_str_input("REGEX", data),_ensure_str_input("CASE_CONVERSION", data), etc., to reduce repetition while keeping the error messages explicit. Purely a readability/maintenance win; current code is functionally fine.
269-309: Minor doc example nit intransform_unicode
transform_unicode(Lines 269-309) correctly delegates tounicodedata.normalize(config.form.value, data)after validating string input, which is the right approach.The example in the docstring (Lines 295-300) shows:
transform_unicode("cafe\\u0301", config)→'cafe' # Single precomposed characterThe normalized result would still contain the accented character (precomposed
'é'), so the string literal in the comment should reflect that accent. This is just a documentation typo and doesn’t affect behavior, but updating it will prevent confusion for anyone copy/pasting the example.
311-402: JSON path logic is solid; good candidate to share withresolve_mapping_pathin v1.1
transform_json_path(Lines 311-390) implements simple, well-bounded path traversal:
- Handles optional
$prefix (Lines 351-359),- Traverses dicts and attributes, blocking private attributes (Lines 369-381),
- Raises
ModelOnexErrorwith clear messages for missing keys/attributes (Lines 371-374 and 385-388).This behavior is nearly parallel to the mapping-path logic in
resolve_mapping_pathfromcompute_executor.py, which you’ve already flagged with a TODO for v1.1. When you get to that, this function is a good reference point:
- Extract a shared internal
_resolve_path(data: Any, path: str, source_label: str)utility that both JSON_PATH and mapping resolution can call.- Keep the error messages differentiated via
source_label(e.g. “in input” vs “in step ''”) while using shared traversal rules, especially around private attribute blocking.No changes are needed for v1.0; just reinforcing that the eventual refactor will reduce duplication and drift risk.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
docs/architecture/CONTRACT_DRIVEN_NODECOMPUTE_V1_0.md(1 hunks)src/omnibase_core/utils/compute_executor.py(1 hunks)src/omnibase_core/utils/compute_transformations.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Always use Poetry for Python package management and task execution. Usepoetry runfor pytest, mypy, black, isort, and other tools. Never use pip directly or execute python commands without the poetry run prefix.
Enforce 100% strict mypy compliance. All functions must have complete type annotations. Usepoetry run mypy src/omnibase_core/to verify strict mode compliance with no errors.
Format code with black, sort imports with isort, and lint with ruff. Runpoetry run black src/ tests/,poetry run isort src/ tests/, andpoetry run ruff check --fix src/ tests/
Files:
src/omnibase_core/utils/compute_transformations.pysrc/omnibase_core/utils/compute_executor.py
src/omnibase_core/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/omnibase_core/**/*.py: Use protocol names withcontainer.get_service()for dependency injection, never resolve services by concrete class names. Example:container.get_service('ProtocolEventBus')instead ofcontainer.get_service('EventBusService')
Always use ModelONEXContainer for dependency injection in node constructors and service resolution. Never confuse it with ModelContainer[T], which is a generic value wrapper, not a DI container.
Use structured error handling with ModelOnexError and EnumCoreErrorCode, never raise generic Exception. Always chain exceptions withfrom ewhen catching and re-raising.
Use the @standard_error_handling decorator on methods to eliminate 6+ lines of try/catch boilerplate. Example:@standard_error_handlingabove async methods.
Respect thread safety constraints: Do NOT share node instances (NodeCompute, NodeEffect), cache, or circuit breaker instances across threads without synchronization. Use thread-local instances or locks.
Use duck typing with protocols. Do not use isinstance checks with protocol types. Rely on the protocol interface directly.
Use EnumNodeKind for high-level architectural classification (EFFECT, COMPUTE, REDUCER, ORCHESTRATOR) when routing data through the ONEX pipeline. Use EnumNodeType for specific implementation discovery and capability matching.
Use ModelEventEnvelope for inter-service event-driven communication. Process events through the envelope pattern for consistent event handling.
Files:
src/omnibase_core/utils/compute_transformations.pysrc/omnibase_core/utils/compute_executor.py
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T23:34:11.788Z
Learning: Applies to src/omnibase_core/nodes/node_compute.py : NodeCompute is for data processing and transformation. Inherit from NodeCompute for COMPUTE nodes that perform calculations, validations, and data mapping.
📚 Learning: 2025-12-06T23:34:11.788Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T23:34:11.788Z
Learning: Applies to src/omnibase_core/nodes/node_compute.py : NodeCompute is for data processing and transformation. Inherit from NodeCompute for COMPUTE nodes that perform calculations, validations, and data mapping.
Applied to files:
src/omnibase_core/utils/compute_executor.py
⏰ 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 (2)
docs/architecture/CONTRACT_DRIVEN_NODECOMPUTE_V1_0.md (1)
1-930: Comprehensive specification document with past validation naming issue properly resolved.This documentation is well-structured and technically accurate. The v1.0 scope is clearly delineated from future versions, models and enums are thoroughly documented with code examples, and the example contract effectively demonstrates all step types and configuration patterns. The past issue flagging
ModelValidationConfigvs.ModelValidationStepConfig(addressed in commit 56e52a9) has been properly resolved—line 141 and the ModelValidationStepConfig section (lines 433–452) now use the correct terminology consistently.A few observations:
Lines 518–539 and 579–592 (pseudocode sections): These reference
ModelOnexErrorin transformation execution and path resolution contexts. Verify that this error class exists in the actual implementation and is the intended type for contract validation failures and runtime errors.Lines 598–609 and 155–156 (IDENTITY handling): The requirement that IDENTITY transformation must not have a config is clearly stated in both the spec and the validator logic. This is good—the constraint is reinforced in both places.
Lines 703–729 (schema resolution): The alias feature for
input_schemaandoutput_schemawithin validation steps is useful, though the current description could clarify whether these aliases are case-sensitive and whether unrecognized aliases fail validation at load time or runtime.Verification task: Please confirm that
ModelOnexErroris defined in the actual implementation and serves as the error type for transformation and path resolution failures. If it's a placeholder, consider aligning with the final error taxonomy.src/omnibase_core/utils/compute_transformations.py (1)
393-467: Transformation registry and dispatcher look correct and future-proofThe
TRANSFORMATION_REGISTRYandexecute_transformation(Lines 393-467) cleanly mapEnumTransformationTypevalues to handlers and centralize dispatch:
- All six v1.0 types are present in the registry (IDENTITY, REGEX, CASE_CONVERSION, TRIM, NORMALIZE_UNICODE, JSON_PATH) (Lines 395-402).
- IDENTITY is correctly treated as a special case where
configis optional and ignored (Lines 457-458).- All other transformations enforce
config is not Noneand raise aVALIDATION_ERRORotherwise (Lines 460-464).- Unknown transformation types are rejected with
OPERATION_FAILED(Lines 449-454).This matches the contract-driven design and should integrate cleanly with the executor. No changes needed here.
## Critical Fixes - Documentation already uses ModelValidationStepConfig correctly ## Major Fixes - Added @pytest.mark.unit to 28 test classes across 5 files - Fixed UUID import pattern in mixin_compute_execution.py - Changed error label from "transformation_error" to "compute_error" - Created ComputePipelineError custom exception with 12 new tests - Added TODO comments with version targets (v1.1) - Added pipeline_timeout_ms upper bound (1 hour max) - Added 18 new integration tests (36 total) ## Minor Fixes - Added comprehensive docstrings to 8 compute module files - Added language specifiers to markdown code blocks ## Nitpick Fixes - Created conftest.py with 14 shared test fixtures - Added timeout decorators to 23 test classes - Aligned transform_identity signature with other handlers - Documented path resolution behavior asymmetry - Simplified IDENTITY transformation special case - Changed logger.error with exc_info to logger.exception Files Modified: 22 files changed, 1754 insertions(+), 127 deletions(-)
Code Review: Contract-Driven NodeCompute v1.0 [OMN-465]SummaryThis PR implements a solid foundation for contract-driven NodeCompute with sequential pipelines, 6 built-in transformations, and abort-on-first-failure semantics. The implementation demonstrates excellent adherence to omnibase_core conventions and includes comprehensive documentation and testing. ✅ Strengths1. Architecture & Design
2. Type Safety & Validation
3. Error Handling
4. Documentation
5. Testing
|
| Metric | Value | Target | Status |
|---|---|---|---|
| Files Added | 48 | - | ✅ |
| Lines Added | 8,707 | - | ✅ |
| Test Files | 8 | - | ✅ |
| Test Cases | 95 | 60+ | ✅ |
| Documentation | 3 major docs | - | ✅ |
| mypy Strict | TBD | 0 errors | |
| CI Status | 2 failures | All pass | ❌ |
🚀 Conclusion
This is high-quality foundational work that demonstrates:
- Deep understanding of omnibase_core architecture
- Excellent separation of concerns
- Strong type safety and error handling
- Comprehensive documentation and testing
The v1.0 scope is appropriately minimal with clear upgrade paths. The implementation is production-ready pending CI fixes.
Recommendation: ✅ APPROVE after CI fixes
The architecture is sound, the code is clean, and the documentation is exemplary. Once the CI failures are resolved, this PR will be a strong foundation for future NodeCompute enhancements.
Next Steps
- Investigate Code Quality failure (likely mypy strict mode issues)
- Investigate Documentation Validation failure
- Run full test suite locally:
poetry run pytest tests/ -x - Verify
poetry run mypy src/omnibase_core/passes with 0 errors - Re-request review after CI is green
Great work on this foundational implementation! 🎉
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
tests/unit/errors/test_compute_pipeline_error.py (1)
12-206: Add@pytest.mark.unitto these test classes for consistencyThese tests do a good job exercising
ComputePipelineErrorcreation, serialization, and import behavior. To keep test categorization consistent with other unit suites (and prior guidance on this repo), consider adding@pytest.mark.unitaboveTestComputePipelineError,TestComputePipelineErrorImport, andTestComputePipelineErrorInvariantsin addition to the existing timeout marks.
🧹 Nitpick comments (12)
src/omnibase_core/models/model_compute_output.py (1)
53-101: Consider addingfrozen=Truefor consistency with other compute models.The docstring notes "If thread-safety is needed, treat instances as read-only after creation," but this isn't enforced. Per the coding guidelines, Pydantic 2.11+ models should be immutable after creation for thread safety.
Other compute models in this PR (e.g.,
ModelComputeStepResult,ModelComputePipelineResult) useConfigDict(frozen=True). Consider adding:+ model_config = ConfigDict(frozen=True)If mutability is intentionally required, consider documenting the specific use case.
src/omnibase_core/utils/compute_transformations.py (1)
319-399: Consider consistent handling of underscore-prefixed keys in dicts.The private attribute check (line 385) only applies to object attributes, not dict keys. Dict keys like
_privateare still accessible:# This is blocked (object attribute): data = SomeObject() # has _private attribute transform_json_path(data, config_with_path="_private") # Error # This is allowed (dict key): data = {"_private": "secret"} transform_json_path(data, config_with_path="_private") # Returns "secret"If this is intentional (dicts represent data, objects have real private state), consider adding a comment. If not, consider adding:
if isinstance(current, dict): + if part.startswith("_"): + raise ModelOnexError( + error_code=EnumCoreErrorCode.VALIDATION_ERROR, + message=f"Cannot access private key: '{part}'", + ) if part not in current:tests/unit/models/contracts/subcontracts/test_model_compute_subcontract.py (3)
16-17: Missing@pytest.mark.timeout()decorator.Per coding guidelines, test classes should have timeout markers to prevent test hangs. Consider adding
@pytest.mark.timeout(30)to this test class.@pytest.mark.unit +@pytest.mark.timeout(30) class TestModelComputeSubcontractCreation:
59-60: Missing@pytest.mark.timeout()decorator.@pytest.mark.unit +@pytest.mark.timeout(30) class TestModelComputeSubcontractTimeoutValidation:
154-155: Missing@pytest.mark.timeout()decorator.@pytest.mark.unit +@pytest.mark.timeout(30) class TestModelComputeSubcontractFrozen:tests/integration/test_compute_pipeline_integration.py (3)
464-499: Tighten assertion intest_regex_with_flagsThe assertion:
assert ( "hello" not in result.output.lower() or result.output == "hi World, hi friend, hi there" )passes for any output that simply lacks
"hello", even if the rest of the string is wrong. If you want to assert both full correctness and full replacement, consider changing this to require both conditions (or just the exact expected string), e.g.:assert result.output == "hi World, hi friend, hi there"or
assert "hello" not in result.output.lower() assert result.output == "hi World, hi friend, hi there"
545-590: Performance thresholds in integration tests may be brittleThe assertions that
processing_time_msis below fixed limits (e.g.,< 1000ms for large strings,< 500ms for many steps,< 1000ms for large lists) encode hardware‑dependent expectations. On slower or heavily loaded CI hosts these could occasionally fail even when the implementation is fine. If you see flaky behavior, consider either relaxing these bounds or converting them to sanity checks (e.g., “less than 10s”) rather than tight performance targets.Also applies to: 593-639, 677-708, 1677-1708
48-53: Consider sharing_create_contextvia a fixtureSeveral integration test classes define an identical
_create_contexthelper. If you want to DRY this up and centralize any future tweaks to context construction, you could move it into a module‑level@pytest.fixture(e.g.,compute_context) and inject it into tests instead. Not required, but it would slightly reduce duplication.Also applies to: 645-650, 853-858, 972-977, 1206-1211, 1370-1375, 1507-1512
src/omnibase_core/mixins/mixin_compute_execution.py (2)
137-147: Avoid serializingnode_id=Noneas the literal string"None"In the execution context construction you currently do:
context = ModelComputeExecutionContext( operation_id=uuid4(), correlation_id=correlation_id, node_id=str(self.node_id) if hasattr(self, "node_id") else None, )If a mixing class sets
self.node_id = None, this will produce"None"rather thanNone, which can be misleading in logs/metrics. A small tweak keeps semantics cleaner:node_id_value = getattr(self, "node_id", None) context = ModelComputeExecutionContext( operation_id=uuid4(), correlation_id=correlation_id, node_id=str(node_id_value) if node_id_value is not None else None, )
186-205: Minor optimization/readability for duplicate step detectionThe duplicate step check repeatedly calls
step_names.count(name), which is O(n²). Pipelines are small so this isn’t a practical issue, but you could make it clearer and linear using a counter or set:seen: set[str] = set() dups: set[str] = set() for name in step_names: if name in seen: dups.add(name) else: seen.add(name) if dups: errors.append(f"Duplicate step names: {dups}")Not required, but slightly easier to read and scales better if pipelines grow.
src/omnibase_core/utils/compute_executor.py (2)
112-239: Path resolver correctly implements v1.0 grammar; consider documenting$inputaliasThe resolver cleanly supports the documented v1.0 forms (
$.input[...]and$.steps.<name>[.output]), distinguishes validation vs operation faults, and blocks private attribute access, which is good for safety and observability. One small nit: you also accept the$inputalias, but it’s not mentioned under “Supported Path Formats”; either documenting this alias or dropping it for stricter grammar would avoid surprises for contract authors.
287-342: Validation-step pass-through behavior is clearly scoped for v1.0The function enforces
validation_configpresence and clearly documents/logs the current pass-through behavior with a TODO for v1.1 schema validation, which aligns with the staged rollout described in the docs. If you see log noise in high-throughput scenarios, you could later gate or downgrade the warning, but it’s fine for v1.0.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
docs/architecture/CONTRACT_DRIVEN_NODECOMPUTE_V1_0.md(1 hunks)docs/architecture/NODECOMPUTE_VERSIONING_ROADMAP.md(1 hunks)src/omnibase_core/enums/enum_compute_step_type.py(1 hunks)src/omnibase_core/errors/__init__.py(3 hunks)src/omnibase_core/errors/exception_compute_pipeline_error.py(1 hunks)src/omnibase_core/mixins/mixin_compute_execution.py(1 hunks)src/omnibase_core/models/compute/__init__.py(1 hunks)src/omnibase_core/models/compute/model_compute_step_metadata.py(1 hunks)src/omnibase_core/models/compute/model_compute_step_result.py(1 hunks)src/omnibase_core/models/contracts/subcontracts/model_compute_pipeline_step.py(1 hunks)src/omnibase_core/models/contracts/subcontracts/model_compute_subcontract.py(1 hunks)src/omnibase_core/models/model_compute_input.py(2 hunks)src/omnibase_core/models/model_compute_output.py(2 hunks)src/omnibase_core/utils/compute_executor.py(1 hunks)src/omnibase_core/utils/compute_transformations.py(1 hunks)tests/integration/test_compute_pipeline_integration.py(1 hunks)tests/unit/errors/test_compute_pipeline_error.py(1 hunks)tests/unit/models/compute/conftest.py(1 hunks)tests/unit/models/configuration/test_model_compute_cache_config.py(7 hunks)tests/unit/models/contracts/subcontracts/test_model_compute_subcontract.py(1 hunks)tests/unit/models/nodes/node_services/test_model_service_compute_health.py(4 hunks)tests/unit/models/nodes/node_services/test_model_service_compute_integration.py(2 hunks)tests/unit/models/nodes/node_services/test_model_service_compute_invocation.py(11 hunks)tests/unit/models/nodes/node_services/test_model_service_compute_lifecycle.py(4 hunks)tests/unit/utils/test_compute_pipeline.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/omnibase_core/enums/enum_compute_step_type.py
- src/omnibase_core/models/contracts/subcontracts/model_compute_pipeline_step.py
🧰 Additional context used
📓 Path-based instructions (5)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Always use Poetry for Python package management and task execution. Usepoetry runfor pytest, mypy, black, isort, and other tools. Never use pip directly or execute python commands without the poetry run prefix.
Enforce 100% strict mypy compliance. All functions must have complete type annotations. Usepoetry run mypy src/omnibase_core/to verify strict mode compliance with no errors.
Format code with black, sort imports with isort, and lint with ruff. Runpoetry run black src/ tests/,poetry run isort src/ tests/, andpoetry run ruff check --fix src/ tests/
Files:
tests/unit/models/nodes/node_services/test_model_service_compute_lifecycle.pysrc/omnibase_core/utils/compute_executor.pytests/unit/utils/test_compute_pipeline.pysrc/omnibase_core/models/model_compute_input.pytests/unit/models/nodes/node_services/test_model_service_compute_health.pysrc/omnibase_core/errors/__init__.pysrc/omnibase_core/models/model_compute_output.pysrc/omnibase_core/errors/exception_compute_pipeline_error.pytests/unit/errors/test_compute_pipeline_error.pysrc/omnibase_core/models/contracts/subcontracts/model_compute_subcontract.pysrc/omnibase_core/utils/compute_transformations.pytests/integration/test_compute_pipeline_integration.pytests/unit/models/compute/conftest.pytests/unit/models/contracts/subcontracts/test_model_compute_subcontract.pysrc/omnibase_core/mixins/mixin_compute_execution.pysrc/omnibase_core/models/compute/__init__.pysrc/omnibase_core/models/compute/model_compute_step_result.pytests/unit/models/nodes/node_services/test_model_service_compute_integration.pytests/unit/models/configuration/test_model_compute_cache_config.pytests/unit/models/nodes/node_services/test_model_service_compute_invocation.pysrc/omnibase_core/models/compute/model_compute_step_metadata.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Write tests using pytest with async support via pytest-asyncio. Maintain minimum 60% code coverage. Use pytest markers: @pytest.mark.unit, @pytest.mark.integration, @pytest.mark.slow
Add @pytest.mark.timeout() to prevent test hangs. Run tests withpoetry run pytest tests/ --timeout=60for 60-second timeout protection.
Files:
tests/unit/models/nodes/node_services/test_model_service_compute_lifecycle.pytests/unit/utils/test_compute_pipeline.pytests/unit/models/nodes/node_services/test_model_service_compute_health.pytests/unit/errors/test_compute_pipeline_error.pytests/integration/test_compute_pipeline_integration.pytests/unit/models/compute/conftest.pytests/unit/models/contracts/subcontracts/test_model_compute_subcontract.pytests/unit/models/nodes/node_services/test_model_service_compute_integration.pytests/unit/models/configuration/test_model_compute_cache_config.pytests/unit/models/nodes/node_services/test_model_service_compute_invocation.py
src/omnibase_core/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/omnibase_core/**/*.py: Use protocol names withcontainer.get_service()for dependency injection, never resolve services by concrete class names. Example:container.get_service('ProtocolEventBus')instead ofcontainer.get_service('EventBusService')
Always use ModelONEXContainer for dependency injection in node constructors and service resolution. Never confuse it with ModelContainer[T], which is a generic value wrapper, not a DI container.
Use structured error handling with ModelOnexError and EnumCoreErrorCode, never raise generic Exception. Always chain exceptions withfrom ewhen catching and re-raising.
Use the @standard_error_handling decorator on methods to eliminate 6+ lines of try/catch boilerplate. Example:@standard_error_handlingabove async methods.
Respect thread safety constraints: Do NOT share node instances (NodeCompute, NodeEffect), cache, or circuit breaker instances across threads without synchronization. Use thread-local instances or locks.
Use duck typing with protocols. Do not use isinstance checks with protocol types. Rely on the protocol interface directly.
Use EnumNodeKind for high-level architectural classification (EFFECT, COMPUTE, REDUCER, ORCHESTRATOR) when routing data through the ONEX pipeline. Use EnumNodeType for specific implementation discovery and capability matching.
Use ModelEventEnvelope for inter-service event-driven communication. Process events through the envelope pattern for consistent event handling.
Files:
src/omnibase_core/utils/compute_executor.pysrc/omnibase_core/models/model_compute_input.pysrc/omnibase_core/errors/__init__.pysrc/omnibase_core/models/model_compute_output.pysrc/omnibase_core/errors/exception_compute_pipeline_error.pysrc/omnibase_core/models/contracts/subcontracts/model_compute_subcontract.pysrc/omnibase_core/utils/compute_transformations.pysrc/omnibase_core/mixins/mixin_compute_execution.pysrc/omnibase_core/models/compute/__init__.pysrc/omnibase_core/models/compute/model_compute_step_result.pysrc/omnibase_core/models/compute/model_compute_step_metadata.py
src/omnibase_core/models/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use Pydantic 2.11+ models for all data validation and serialization. Ensure models are immutable after creation for thread safety.
Files:
src/omnibase_core/models/model_compute_input.pysrc/omnibase_core/models/model_compute_output.pysrc/omnibase_core/models/contracts/subcontracts/model_compute_subcontract.pysrc/omnibase_core/models/compute/__init__.pysrc/omnibase_core/models/compute/model_compute_step_result.pysrc/omnibase_core/models/compute/model_compute_step_metadata.py
src/omnibase_core/mixins/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Mixin classes should be composed into node classes for specific capabilities. Available mixins include: MixinDiscoveryResponder, MixinEventHandler, MixinEventListener, MixinNodeExecutor, MixinNodeLifecycle, MixinRequestResponseIntrospection, MixinWorkflowSupport.
Files:
src/omnibase_core/mixins/mixin_compute_execution.py
🧠 Learnings (7)
📓 Common learnings
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T23:34:11.788Z
Learning: Applies to src/omnibase_core/nodes/node_compute.py : NodeCompute is for data processing and transformation. Inherit from NodeCompute for COMPUTE nodes that perform calculations, validations, and data mapping.
📚 Learning: 2025-12-06T23:34:11.788Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T23:34:11.788Z
Learning: Applies to tests/**/*.py : Add pytest.mark.timeout() to prevent test hangs. Run tests with `poetry run pytest tests/ --timeout=60` for 60-second timeout protection.
Applied to files:
tests/unit/models/nodes/node_services/test_model_service_compute_lifecycle.pytests/unit/utils/test_compute_pipeline.pytests/unit/models/nodes/node_services/test_model_service_compute_health.pytests/unit/models/nodes/node_services/test_model_service_compute_integration.pytests/unit/models/nodes/node_services/test_model_service_compute_invocation.py
📚 Learning: 2025-12-06T23:34:11.788Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T23:34:11.788Z
Learning: Applies to docs/guides/node-building/**/*.md : Node building guide is the critical starting point for developers and AI agents. Follow the sequence: What is a Node → Node Types → COMPUTE Tutorial → EFFECT Tutorial → REDUCER Tutorial → ORCHESTRATOR Tutorial.
Applied to files:
docs/architecture/NODECOMPUTE_VERSIONING_ROADMAP.md
📚 Learning: 2025-12-06T23:34:11.788Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T23:34:11.788Z
Learning: Applies to src/omnibase_core/nodes/node_compute.py : NodeCompute is for data processing and transformation. Inherit from NodeCompute for COMPUTE nodes that perform calculations, validations, and data mapping.
Applied to files:
src/omnibase_core/utils/compute_executor.pysrc/omnibase_core/models/model_compute_input.pysrc/omnibase_core/models/model_compute_output.pysrc/omnibase_core/errors/exception_compute_pipeline_error.pysrc/omnibase_core/mixins/mixin_compute_execution.py
📚 Learning: 2025-12-06T23:34:11.788Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T23:34:11.788Z
Learning: Applies to tests/**/*.py : Write tests using pytest with async support via pytest-asyncio. Maintain minimum 60% code coverage. Use pytest markers: pytest.mark.unit, pytest.mark.integration, pytest.mark.slow
Applied to files:
tests/unit/utils/test_compute_pipeline.py
📚 Learning: 2025-12-06T23:34:11.788Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T23:34:11.788Z
Learning: Applies to src/omnibase_core/**/*.py : Use structured error handling with ModelOnexError and EnumCoreErrorCode, never raise generic Exception. Always chain exceptions with `from e` when catching and re-raising.
Applied to files:
src/omnibase_core/errors/__init__.pysrc/omnibase_core/errors/exception_compute_pipeline_error.pytests/unit/errors/test_compute_pipeline_error.py
📚 Learning: 2025-12-06T23:34:11.788Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T23:34:11.788Z
Learning: Applies to src/omnibase_core/mixins/**/*.py : Mixin classes should be composed into node classes for specific capabilities. Available mixins include: MixinDiscoveryResponder, MixinEventHandler, MixinEventListener, MixinNodeExecutor, MixinNodeLifecycle, MixinRequestResponseIntrospection, MixinWorkflowSupport.
Applied to files:
src/omnibase_core/mixins/mixin_compute_execution.py
🧬 Code graph analysis (13)
src/omnibase_core/utils/compute_executor.py (1)
src/omnibase_core/models/compute/model_compute_execution_context.py (1)
ModelComputeExecutionContext(44-95)
tests/unit/utils/test_compute_pipeline.py (20)
src/omnibase_core/enums/enum_case_mode.py (1)
EnumCaseMode(10-22)src/omnibase_core/enums/enum_compute_step_type.py (1)
EnumComputeStepType(50-91)src/omnibase_core/enums/enum_regex_flag.py (1)
EnumRegexFlag(10-22)src/omnibase_core/enums/enum_trim_mode.py (1)
EnumTrimMode(10-22)src/omnibase_core/enums/enum_unicode_form.py (1)
EnumUnicodeForm(10-24)src/omnibase_core/models/compute/model_compute_execution_context.py (1)
ModelComputeExecutionContext(44-95)src/omnibase_core/models/compute/model_compute_step_metadata.py (1)
ModelComputeStepMetadata(37-77)src/omnibase_core/models/compute/model_compute_step_result.py (1)
ModelComputeStepResult(64-122)src/omnibase_core/models/contracts/subcontracts/model_compute_pipeline_step.py (1)
ModelComputePipelineStep(81-200)src/omnibase_core/models/contracts/subcontracts/model_compute_subcontract.py (1)
ModelComputeSubcontract(73-147)src/omnibase_core/models/transformations/model_mapping_config.py (1)
ModelMappingConfig(13-37)src/omnibase_core/models/transformations/model_transform_case_config.py (1)
ModelTransformCaseConfig(15-27)src/omnibase_core/models/transformations/model_transform_json_path_config.py (1)
ModelTransformJsonPathConfig(13-35)src/omnibase_core/models/transformations/model_transform_regex_config.py (1)
ModelTransformRegexConfig(15-31)src/omnibase_core/models/transformations/model_transform_trim_config.py (1)
ModelTransformTrimConfig(15-27)src/omnibase_core/models/transformations/model_transform_unicode_config.py (1)
ModelTransformUnicodeConfig(15-27)src/omnibase_core/models/transformations/model_validation_step_config.py (1)
ModelValidationStepConfig(13-30)src/omnibase_core/utils/compute_executor.py (2)
execute_compute_pipeline(409-582)resolve_mapping_path(112-238)src/omnibase_core/utils/compute_transformations.py (7)
execute_transformation(413-474)transform_case(178-224)transform_identity(84-120)transform_json_path(319-398)transform_regex(123-175)transform_trim(227-274)transform_unicode(277-316)src/omnibase_core/protocols/event_bus.py (1)
operation_name(156-158)
src/omnibase_core/errors/__init__.py (1)
src/omnibase_core/errors/exception_compute_pipeline_error.py (1)
ComputePipelineError(56-129)
src/omnibase_core/errors/exception_compute_pipeline_error.py (1)
src/omnibase_core/enums/enum_core_error_code.py (1)
EnumCoreErrorCode(9-154)
tests/unit/errors/test_compute_pipeline_error.py (2)
src/omnibase_core/errors/exception_compute_pipeline_error.py (1)
ComputePipelineError(56-129)src/omnibase_core/enums/enum_core_error_code.py (1)
EnumCoreErrorCode(9-154)
src/omnibase_core/models/contracts/subcontracts/model_compute_subcontract.py (1)
src/omnibase_core/models/contracts/subcontracts/model_compute_pipeline_step.py (1)
ModelComputePipelineStep(81-200)
src/omnibase_core/utils/compute_transformations.py (10)
src/omnibase_core/enums/enum_case_mode.py (1)
EnumCaseMode(10-22)src/omnibase_core/enums/enum_core_error_code.py (1)
EnumCoreErrorCode(9-154)src/omnibase_core/enums/enum_regex_flag.py (1)
EnumRegexFlag(10-22)src/omnibase_core/enums/enum_transformation_type.py (1)
EnumTransformationType(11-34)src/omnibase_core/enums/enum_trim_mode.py (1)
EnumTrimMode(10-22)src/omnibase_core/models/transformations/model_transform_case_config.py (1)
ModelTransformCaseConfig(15-27)src/omnibase_core/models/transformations/model_transform_json_path_config.py (1)
ModelTransformJsonPathConfig(13-35)src/omnibase_core/models/transformations/model_transform_regex_config.py (1)
ModelTransformRegexConfig(15-31)src/omnibase_core/models/transformations/model_transform_trim_config.py (1)
ModelTransformTrimConfig(15-27)src/omnibase_core/models/transformations/model_transform_unicode_config.py (1)
ModelTransformUnicodeConfig(15-27)
tests/integration/test_compute_pipeline_integration.py (6)
src/omnibase_core/enums/enum_compute_step_type.py (1)
EnumComputeStepType(50-91)src/omnibase_core/models/compute/model_compute_execution_context.py (1)
ModelComputeExecutionContext(44-95)src/omnibase_core/models/contracts/subcontracts/model_compute_pipeline_step.py (1)
ModelComputePipelineStep(81-200)src/omnibase_core/models/contracts/subcontracts/model_compute_subcontract.py (1)
ModelComputeSubcontract(73-147)src/omnibase_core/models/transformations/model_mapping_config.py (1)
ModelMappingConfig(13-37)src/omnibase_core/utils/compute_executor.py (1)
execute_compute_pipeline(409-582)
tests/unit/models/contracts/subcontracts/test_model_compute_subcontract.py (4)
src/omnibase_core/enums/enum_compute_step_type.py (1)
EnumComputeStepType(50-91)src/omnibase_core/enums/enum_transformation_type.py (1)
EnumTransformationType(11-34)src/omnibase_core/models/contracts/subcontracts/model_compute_pipeline_step.py (1)
ModelComputePipelineStep(81-200)src/omnibase_core/models/contracts/subcontracts/model_compute_subcontract.py (1)
ModelComputeSubcontract(73-147)
src/omnibase_core/mixins/mixin_compute_execution.py (4)
src/omnibase_core/models/compute/model_compute_execution_context.py (1)
ModelComputeExecutionContext(44-95)src/omnibase_core/models/compute/model_compute_pipeline_result.py (1)
ModelComputePipelineResult(53-122)src/omnibase_core/models/contracts/subcontracts/model_compute_subcontract.py (1)
ModelComputeSubcontract(73-147)src/omnibase_core/utils/compute_executor.py (1)
execute_compute_pipeline(409-582)
src/omnibase_core/models/compute/__init__.py (4)
src/omnibase_core/models/compute/model_compute_execution_context.py (1)
ModelComputeExecutionContext(44-95)src/omnibase_core/models/compute/model_compute_pipeline_result.py (1)
ModelComputePipelineResult(53-122)src/omnibase_core/models/compute/model_compute_step_metadata.py (1)
ModelComputeStepMetadata(37-77)src/omnibase_core/models/compute/model_compute_step_result.py (1)
ModelComputeStepResult(64-122)
src/omnibase_core/models/compute/model_compute_step_result.py (1)
src/omnibase_core/models/compute/model_compute_step_metadata.py (1)
ModelComputeStepMetadata(37-77)
tests/unit/models/nodes/node_services/test_model_service_compute_integration.py (1)
src/omnibase_core/models/infrastructure/model_time_based.py (1)
timeout(288-310)
🪛 LanguageTool
docs/architecture/NODECOMPUTE_VERSIONING_ROADMAP.md
[style] ~55-~55: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...itions. Boring code is debuggable code. Boring APIs are learnable APIs. Stable: O...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
⏰ 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 (36)
tests/unit/models/nodes/node_services/test_model_service_compute_lifecycle.py (4)
94-95: LGTM! Pytest markers correctly applied.The
@pytest.mark.unitand@pytest.mark.timeout(60)markers are correctly added to categorize this as a unit test class and prevent test hangs. The 60-second timeout is appropriate given the complex async lifecycle operations, including service start/stop cycles and multiple sleep delays.Based on coding guidelines: "Write tests using pytest with async support via pytest-asyncio. Use pytest markers: @pytest.mark.unit, @pytest.mark.integration, @pytest.mark.slow. Add @pytest.mark.timeout() to prevent test hangs."
542-543: LGTM! Appropriate timeout for tool invocation tests.The markers are correctly applied. The 30-second timeout is well-suited for these simpler tool invocation tests, which have faster execution patterns compared to the lifecycle tests.
607-608: LGTM! Timeout is more than sufficient.The markers are correctly applied. The 30-second timeout is more than adequate for these predominantly synchronous health monitoring tests.
681-682: LGTM! Markers correctly applied.The markers properly categorize these shutdown handling tests and provide adequate timeout protection for the async shutdown operations.
tests/unit/models/configuration/test_model_compute_cache_config.py (1)
16-16: LGTM! Proper test categorization with unit markers.The addition of
@pytest.mark.unitto all test classes correctly categorizes these as unit tests, aligning with the project's coding guidelines. This enables selective test execution and improves test organization.Also applies to: 51-51, 132-132, 190-190, 257-257, 294-294, 347-347
docs/architecture/NODECOMPUTE_VERSIONING_ROADMAP.md (1)
1-993: Well-structured versioning roadmap.The document provides a comprehensive versioning strategy with clear:
- Philosophy ("boring first" principle)
- Feature progression across v1.0-v1.3
- Version guarantees and migration paths
- Success criteria for each version
The explicit deferral of features (caching, parallelism, conditionals) to later versions aligns well with the PR's v1.0 scope.
docs/architecture/CONTRACT_DRIVEN_NODECOMPUTE_V1_0.md (1)
1-930: Comprehensive v1.0 specification.The document provides a clear, implementable specification with:
- Well-defined core models with immutability guarantees
- Clear transformation config patterns using discriminated unions
- Detailed execution model with abort-on-first-failure semantics
- Comprehensive example contract demonstrating all step types
- Realistic implementation plan with acceptance criteria
The separation of VALIDATION as a step type (not transformation) is a good design decision that keeps semantic boundaries clean.
src/omnibase_core/models/model_compute_output.py (1)
1-51: Excellent documentation.The module and class docstrings are comprehensive with:
- Clear thread safety notes
- Well-documented type parameters
- Practical usage examples
- Cross-references to related modules
src/omnibase_core/utils/compute_transformations.py (4)
84-121: LGTM - Clean identity transformation implementation.The identity transformation correctly:
- Accepts any input type unchanged
- Requires no config (enforced by contract validation)
- Uses
del configto explicitly mark unused parameter- Documents the uniform registry handler pattern
123-176: LGTM - Robust regex transformation.Good defensive implementation with:
- Input type validation
- Explicit flag conversion (no implicit Python flag usage)
- Proper error wrapping with exception chaining (
from e)
178-317: LGTM - Clean string transformation implementations.All three transformations (
transform_case,transform_trim,transform_unicode) follow a consistent pattern:
- Input type validation
- Exhaustive enum matching with defensive else clause
- Clear error messages with appropriate error codes
The defensive else clauses for unknown enum values are good practice even though they should be unreachable with proper typing.
401-491: LGTM - Well-designed transformation registry and executor.The implementation correctly:
- Maps all 6 v1.0 transformation types to handlers
- Handles IDENTITY special case (no config required)
- Validates config presence for non-IDENTITY transformations
- Provides clear error messages with appropriate error codes
- Exports all public symbols in
__all__tests/unit/models/nodes/node_services/test_model_service_compute_invocation.py (2)
101-107: LGTM - Timeout markers added per coding guidelines.Adding
@pytest.mark.unitand@pytest.mark.timeout(30)to test classes follows the coding guidelines for preventing test hangs. The 30-second timeout is appropriate for service invocation tests that may involve async operations.Based on learnings: "Add pytest.mark.timeout() to prevent test hangs."
229-925: Consistent marker application across all test classes.All 11 test classes now have
@pytest.mark.unitand@pytest.mark.timeout(30)markers applied consistently. This ensures:
- Tests are properly categorized for selective execution
- Timeout protection prevents hangs during CI/CD
src/omnibase_core/errors/exception_compute_pipeline_error.py (1)
56-129: Well-structured exception class following project conventions.The implementation correctly:
- Inherits from
ModelOnexErrorper coding guidelines for structured error handling- Uses
EnumCoreErrorCode.PROCESSING_ERRORas the default error code- Provides complete type annotations for mypy strict compliance
- Injects
step_nameandoperationinto context while also storing as attributes for direct accessThe dual storage (in context dict and as attributes) is intentional and useful for both serialization and programmatic access.
tests/unit/models/nodes/node_services/test_model_service_compute_health.py (4)
131-133: Test markers properly applied per coding guidelines.The
@pytest.mark.unitand@pytest.mark.timeout(30)decorators are correctly applied. The 30-second timeout is appropriate for health status tests.
240-242: Appropriate timeout for health monitoring tests.
371-373: Good choice of longer timeout for graceful shutdown tests.The 60-second timeout is appropriate for
TestModelServiceComputeGracefulShutdownsince these tests involve async shutdown operations, signal handlers, and waiting for active invocations to complete.
655-657: Edge case tests properly marked.tests/unit/models/nodes/node_services/test_model_service_compute_integration.py (2)
73-75: Integration test markers appropriately applied.The 60-second timeout for
TestModelServiceComputeIntegrationis suitable given the comprehensive end-to-end tests including cache operations, health checks, and tool invocations.
601-603: MRO detail tests properly marked.The 30-second timeout is appropriate for these synchronous MRO verification tests.
tests/unit/models/contracts/subcontracts/test_model_compute_subcontract.py (1)
1-176: Comprehensive test coverage for ModelComputeSubcontract.The tests thoroughly cover:
- Minimal/default construction
- Pipeline with steps
- Timeout validation boundaries (1ms, 1hr max, rejection of 0/negative/exceeds-max)
- Immutability enforcement
- Extra field rejection
Good use of
pytest.raiseswithmatchpatterns for validation error assertions.src/omnibase_core/models/compute/model_compute_step_result.py (3)
48-61: Correct forward reference pattern for avoiding circular imports.The
TYPE_CHECKINGguard with runtime import andmodel_rebuild()is the proper pattern for handling forward references in Pydantic while avoiding circular import issues.
64-123: Well-designed immutable step result model.The model correctly:
- Uses
frozen=Truefor thread safety per coding guidelines- Uses
extra="forbid"to reject unknown fields- Documents success/failure semantics clearly
- Uses
str | Nonefor error_type (v1.0 design decision documented)- Provides comprehensive docstrings with usage examples
125-130: Runtime import for forward reference resolution.The
# noqa: E402suppresses the late import warning, which is intentional here. Themodel_rebuild()call finalizes the forward reference resolution.tests/unit/utils/test_compute_pipeline.py (1)
60-585: Well‑scoped unit coverage for transformations, mapping, and executorThe tests here exercise all transformation handlers (including flag/error paths), mapping path resolution, and core pipeline semantics (multi‑step, abort‑on‑failure, disabled steps, timing) in a focused, deterministic way. This gives strong protection around the new NodeCompute v1.0 surface without introducing brittle coupling to internal details.
src/omnibase_core/errors/__init__.py (1)
7-9: ComputePipelineError export aligns with existing lazy‑load patternTYPE_CHECKING import,
__all__entry, and the__getattr__branch forComputePipelineErrormirror how other error types are exposed from this package, giving a clean public API without adding eager imports or new cycles.Also applies to: 46-67, 110-116
src/omnibase_core/models/compute/model_compute_step_metadata.py (1)
37-77: Step metadata model matches executor semanticsFreezing the model and constraining
duration_msto be non‑negative fits howexecute_compute_pipelineconstructs metadata and how tests assert on it. Using a plainstr | Nonefortransformation_typeis consistent with storing the enum’s.valuewhile keeping the model simple.tests/integration/test_compute_pipeline_integration.py (1)
44-1200: Comprehensive integration coverage for NodeCompute v1.0This module gives very strong end‑to‑end coverage: step sequencing, abort‑on‑first‑failure, mapping over prior outputs, disabled steps, timing metadata, concurrency, and edge cases across all step types are all exercised against the public API. This should make regressions in the executor or transformation layer highly visible.
src/omnibase_core/models/model_compute_input.py (1)
61-102: Input model metadata cleanly matches described semanticsThe additional fields on
ModelComputeInput(operation_id, computation_type, cache/parallel flags, metadata, timestamp) are well‑documented and their defaults align with the examples. This should give NodeCompute implementations enough context for routing, caching, and basic observability without complicating the call‑site.Also applies to: 104-110
src/omnibase_core/models/compute/__init__.py (1)
1-78: Public compute models API is clear and minimalRe‑exporting just the four core compute models with a descriptive module docstring gives a tidy import surface (
from omnibase_core.models.compute import ...) that matches how the executor and tests use these types.src/omnibase_core/utils/compute_executor.py (4)
77-107: Error-type extraction helper is robust and aligned with result model
_get_error_typecleanly normalizesModelOnexError.error_codefor use in pipeline results, handlingNone, enum-like values, and raw strings without leaking implementation details. No changes needed.
241-285: Mapping-step execution is straightforward and leverages the resolver correctly
execute_mapping_stepenforces presence ofmapping_configand delegates all value extraction toresolve_mapping_path, which keeps mapping logic simple and consistent with path semantics. Implementation looks solid.
344-404: Step dispatcher cleanly separates concerns across step types
execute_pipeline_steprespectsstep.enabled, enforcestransformation_typefor transformation steps, and routes to the appropriate helper for TRANSFORMATION/MAPPING/VALIDATION, raising aModelOnexErrorfor unknown types. The separation keeps per-step logic focused and matches theEnumComputeStepTypecontract.
409-582: Pipeline executor correctly implements abort-on-first-failure and structured errorsThe main loop in
execute_compute_pipeline:
- Skips disabled steps.
- Times each executed step and records per-step metadata.
- Stores results in a dict keyed by
step_name, which enables subsequent path resolution.- Cleanly distinguishes
ModelOnexError(mapped via_get_error_type) from unexpected exceptions (logged and surfaced as"unexpected_error"), while always returning a structuredModelComputePipelineResult.This matches the v1.0 contract description and the coding guideline of using
ModelOnexErrorandEnumCoreErrorCodeas the primary error surface, with a safe fallback for unexpected failures. As per coding guidelines, this is a solid implementation of contract-driven, structured error handling.tests/unit/models/compute/conftest.py (1)
37-189: Fixture set cleanly covers typical compute contracts and contextsThe fixtures provide focused, reusable builders for execution contexts, transformation configs, pipeline steps, and several representative
ModelComputeSubcontractshapes (empty, single-step, multi-step, with timeout). This should keep unit and integration tests concise and consistent. No issues spotted.
…465] Documentation: - Add $input alias documentation for path resolution - Fix version inconsistencies (v1.1/v1.2/v1.3 alignment) - Change specific dates to relative milestones - Add example contract reference Model Validators: - Add unique step_name validator with helpful error messages - Add 6 new tests for step name uniqueness validation Code Quality & Observability: - Add logging for pipeline step execution with correlation_id - Add context to all error messages (13 error sites) - Add validation step warning with deduplication - Add TODO comments with target versions Code Consistency: - DRY: Create _validate_string_input() helper function - Align transform_identity signature with other handlers - Document private attribute security behavior - Optimize duplicate step detection from O(n²) to O(n) Test Infrastructure: - Add @pytest.mark.timeout(60) to integration tests - Tighten frozen model test assertions - Simplify regex test assertion - Add shared conftest.py for integration test fixtures
PR Review: Contract-Driven NodeCompute v1.0 [OMN-465]SummaryThis is a well-architected, production-ready implementation of contract-driven NodeCompute v1.0. The PR demonstrates exceptional attention to detail, strong adherence to ONEX design principles, and comprehensive documentation. The incremental versioning approach (v1.0 → v1.1 → v1.2) is exemplary. ✅ Strengths1. Architecture & Design
2. Code Quality
3. Testing
4. Documentation
5. Performance & Security
🔍 Issues & RecommendationsCritical IssuesNone identified - Code is production-ready. Minor Issues1. TODO Comments Need Tracking (Low Priority)Several TODOs reference v1.1/v1.2 features but lack ticket numbers:
Recommendation: Create Linear tickets for each TODO and reference them in comments (e.g., 2. Validation Step Warning Could Be Noisy (Low Priority)
Recommendation: Consider:
3. Path Resolution Duplication (Already Tracked in Code)
Recommendation: Defer to v1.1 as already noted in TODO (line 113). Potential Improvements1. Type Narrowing for Error Codes
def _get_error_type(error: ModelOnexError) -> str:
if error.error_code is None:
return "compute_error"
# Use value if enum, otherwise convert to string
return getattr(error.error_code, "value", str(error.error_code))This is cleaner but current implementation is also acceptable. 2. Contract Validation Could Validate Schema Refs
Recommendation: Add schema ref validation in v1.1 (when schema validation is fully implemented). 3. Missing Enum Export in
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
src/omnibase_core/utils/compute_executor.py (2)
54-56: Global mutable state contradicts "pure and stateless" claim.The global
_validation_warning_emittedflag introduces mutable module-level state, which contradicts the docstring claim that "All functions in this module are pure and stateless" (line 10). While the warning deduplication intent is reasonable, this creates a side effect that persists across calls.Consider using Python's
warnings.warnwith a custom warning class andwarnings.filterwarnings('once', ...)to achieve deduplication without module-level state, or document this exception explicitly.-# Track if validation warning has been emitted for session deduplication -_validation_warning_emitted = False +# Custom warning class for filtering +class ValidationPassthroughWarning(UserWarning): + """Warning emitted when validation steps use pass-through mode in v1.0.""" + passThen in
execute_validation_step:import warnings warnings.warn( "Validation steps are pass-through in v1.0. " "Schema validation will be implemented in v1.1.", ValidationPassthroughWarning, stacklevel=2, )And let the caller configure
warnings.filterwarnings('once', category=ValidationPassthroughWarning)if deduplication is desired.
184-186: Undocumented path alias may cause confusion.Line 184 accepts both
"$.input"and"$input"(without the dot), but only$.inputis documented in the docstring. This undocumented alias could lead to inconsistent usage patterns.Either document
$inputas an accepted alias or remove it for consistency:- if path == "$.input" or path == "$input": + if path == "$.input": return input_datatests/integration/conftest.py (1)
19-49: Fixtures are well-designed but appear unused in the test file.The fixtures correctly provide both single-use and factory patterns for execution contexts. However, reviewing
test_compute_pipeline_integration.py, each test class defines its own_create_context()method instead of using these fixtures.Consider refactoring the test classes to use these fixtures to reduce duplication:
# In test class: def test_example(self, compute_execution_context): result = execute_compute_pipeline(contract, data, compute_execution_context)Or for multiple contexts in one test:
def test_example(self, compute_execution_context_factory): ctx1 = compute_execution_context_factory() ctx2 = compute_execution_context_factory()tests/integration/test_compute_pipeline_integration.py (2)
60-65: Consider using the shared fixtures from conftest.py.The
_create_context()method is duplicated across 7 test classes (lines 60-65, 658-663, 874-879, 997-1002, 1235-1240, 1403-1408, 1544-1549). Thetests/integration/conftest.pyprovidescompute_execution_contextandcompute_execution_context_factoryfixtures specifically to eliminate this duplication.Example refactor:
class TestComputePipelineIntegration: - def _create_context(self) -> ModelComputeExecutionContext: - """Create a test execution context.""" - return ModelComputeExecutionContext( - operation_id=uuid4(), - correlation_id=uuid4(), - ) - def test_multi_step_pipeline_execution( + self, compute_execution_context: ModelComputeExecutionContext ) -> None: # ... result = execute_compute_pipeline( contract=contract, input_data=" hello world ", - context=self._create_context(), + context=compute_execution_context, )
1359-1360: Move import to module level for consistency.The inline import of
ModelValidationStepConfig(line 1359) is inconsistent with the other imports at the top of the file. While functional, module-level imports improve readability and catch import errors earlier.# At top of file with other imports +from omnibase_core.models.transformations.model_validation_step_config import ( + ModelValidationStepConfig, +) # In test method - from omnibase_core.models.transformations.model_validation_step_config import \ - ModelValidationStepConfig
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
docs/architecture/CONTRACT_DRIVEN_NODECOMPUTE_V1_0.md(1 hunks)docs/architecture/NODECOMPUTE_VERSIONING_ROADMAP.md(1 hunks)src/omnibase_core/mixins/mixin_compute_execution.py(1 hunks)src/omnibase_core/models/contracts/subcontracts/model_compute_subcontract.py(1 hunks)src/omnibase_core/utils/compute_executor.py(1 hunks)src/omnibase_core/utils/compute_transformations.py(1 hunks)tests/integration/conftest.py(1 hunks)tests/integration/test_compute_pipeline_integration.py(1 hunks)tests/unit/models/compute/test_compute_models.py(1 hunks)tests/unit/models/contracts/subcontracts/test_model_compute_subcontract.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- tests/unit/models/contracts/subcontracts/test_model_compute_subcontract.py
- tests/unit/models/compute/test_compute_models.py
- docs/architecture/CONTRACT_DRIVEN_NODECOMPUTE_V1_0.md
- src/omnibase_core/mixins/mixin_compute_execution.py
- src/omnibase_core/utils/compute_transformations.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. Usepoetry runfor pytest, mypy, black, isort, and other tools. Never use pip directly or execute python commands without the poetry run prefix.
Enforce 100% strict mypy compliance. All functions must have complete type annotations. Usepoetry run mypy src/omnibase_core/to verify strict mode compliance with no errors.
Format code with black, sort imports with isort, and lint with ruff. Runpoetry run black src/ tests/,poetry run isort src/ tests/, andpoetry run ruff check --fix src/ tests/
Files:
tests/integration/conftest.pysrc/omnibase_core/utils/compute_executor.pytests/integration/test_compute_pipeline_integration.pysrc/omnibase_core/models/contracts/subcontracts/model_compute_subcontract.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Write tests using pytest with async support via pytest-asyncio. Maintain minimum 60% code coverage. Use pytest markers: @pytest.mark.unit, @pytest.mark.integration, @pytest.mark.slow
Add @pytest.mark.timeout() to prevent test hangs. Run tests withpoetry run pytest tests/ --timeout=60for 60-second timeout protection.
Files:
tests/integration/conftest.pytests/integration/test_compute_pipeline_integration.py
src/omnibase_core/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/omnibase_core/**/*.py: Use protocol names withcontainer.get_service()for dependency injection, never resolve services by concrete class names. Example:container.get_service('ProtocolEventBus')instead ofcontainer.get_service('EventBusService')
Always use ModelONEXContainer for dependency injection in node constructors and service resolution. Never confuse it with ModelContainer[T], which is a generic value wrapper, not a DI container.
Use structured error handling with ModelOnexError and EnumCoreErrorCode, never raise generic Exception. Always chain exceptions withfrom ewhen catching and re-raising.
Use the @standard_error_handling decorator on methods to eliminate 6+ lines of try/catch boilerplate. Example:@standard_error_handlingabove async methods.
Respect thread safety constraints: Do NOT share node instances (NodeCompute, NodeEffect), cache, or circuit breaker instances across threads without synchronization. Use thread-local instances or locks.
Use duck typing with protocols. Do not use isinstance checks with protocol types. Rely on the protocol interface directly.
Use EnumNodeKind for high-level architectural classification (EFFECT, COMPUTE, REDUCER, ORCHESTRATOR) when routing data through the ONEX pipeline. Use EnumNodeType for specific implementation discovery and capability matching.
Use ModelEventEnvelope for inter-service event-driven communication. Process events through the envelope pattern for consistent event handling.
Files:
src/omnibase_core/utils/compute_executor.pysrc/omnibase_core/models/contracts/subcontracts/model_compute_subcontract.py
src/omnibase_core/models/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use Pydantic 2.11+ models for all data validation and serialization. Ensure models are immutable after creation for thread safety.
Files:
src/omnibase_core/models/contracts/subcontracts/model_compute_subcontract.py
🧠 Learnings (4)
📓 Common learnings
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T23:34:11.788Z
Learning: Applies to src/omnibase_core/nodes/node_compute.py : NodeCompute is for data processing and transformation. Inherit from NodeCompute for COMPUTE nodes that perform calculations, validations, and data mapping.
📚 Learning: 2025-12-06T23:34:11.788Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T23:34:11.788Z
Learning: Applies to docs/guides/node-building/**/*.md : Node building guide is the critical starting point for developers and AI agents. Follow the sequence: What is a Node → Node Types → COMPUTE Tutorial → EFFECT Tutorial → REDUCER Tutorial → ORCHESTRATOR Tutorial.
Applied to files:
docs/architecture/NODECOMPUTE_VERSIONING_ROADMAP.md
📚 Learning: 2025-12-06T23:34:11.788Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T23:34:11.788Z
Learning: Applies to src/omnibase_core/nodes/node_compute.py : NodeCompute is for data processing and transformation. Inherit from NodeCompute for COMPUTE nodes that perform calculations, validations, and data mapping.
Applied to files:
src/omnibase_core/utils/compute_executor.py
📚 Learning: 2025-12-06T23:34:11.788Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T23:34:11.788Z
Learning: Applies to src/omnibase_core/nodes/node_orchestrator.py : NodeOrchestrator is for workflow-driven coordination. Use ModelAction with Leases for multi-step workflows and parallel execution. This is the PRIMARY implementation as of v0.4.0 (replaces NodeOrchestratorDeclarative).
Applied to files:
src/omnibase_core/utils/compute_executor.py
🧬 Code graph analysis (3)
tests/integration/conftest.py (1)
src/omnibase_core/models/compute/model_compute_execution_context.py (1)
ModelComputeExecutionContext(44-95)
tests/integration/test_compute_pipeline_integration.py (7)
src/omnibase_core/enums/enum_compute_step_type.py (1)
EnumComputeStepType(50-91)src/omnibase_core/enums/enum_transformation_type.py (1)
EnumTransformationType(11-34)src/omnibase_core/models/compute/model_compute_execution_context.py (1)
ModelComputeExecutionContext(44-95)src/omnibase_core/models/contracts/subcontracts/model_compute_pipeline_step.py (1)
ModelComputePipelineStep(81-200)src/omnibase_core/models/contracts/subcontracts/model_compute_subcontract.py (1)
ModelComputeSubcontract(73-181)src/omnibase_core/models/transformations/model_mapping_config.py (1)
ModelMappingConfig(13-37)src/omnibase_core/utils/compute_executor.py (1)
execute_compute_pipeline(430-642)
src/omnibase_core/models/contracts/subcontracts/model_compute_subcontract.py (1)
src/omnibase_core/models/contracts/subcontracts/model_compute_pipeline_step.py (1)
ModelComputePipelineStep(81-200)
🪛 LanguageTool
docs/architecture/NODECOMPUTE_VERSIONING_ROADMAP.md
[style] ~55-~55: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...itions. Boring code is debuggable code. Boring APIs are learnable APIs. Stable: O...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
⏰ 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 (8)
docs/architecture/NODECOMPUTE_VERSIONING_ROADMAP.md (1)
1-993: Comprehensive and well-structured versioning roadmap.The document establishes a clear versioning philosophy with the "boring first" principle, appropriate for a platform primitive. The milestone-based timeline (lines 130-139) correctly avoids fixed calendar dates in favor of production-validated progression. The clear delineation between what's in scope for each version and the backward compatibility guarantees are well-articulated.
The three successive uses of "Boring" at line 55 appear intentional for rhetorical emphasis and fit the document's tone.
src/omnibase_core/utils/compute_executor.py (3)
298-362: LGTM: Validation step correctly implements v1.0 pass-through.The pass-through behavior with appropriate warnings is well-documented and aligned with the v1.0 specification. The debug logging provides good observability for troubleshooting.
365-424: LGTM: Clean step dispatcher with proper error handling.The dispatch logic covers all v1.0 step types with appropriate error handling for unknown types. The early return for disabled steps is efficient.
430-642: LGTM: Well-implemented pipeline executor with abort-on-first-failure.The implementation correctly follows the v1.0 specification:
- Sequential execution in definition order
- Comprehensive timing metrics with
time.perf_counter()- Failed steps are properly recorded before aborting
- Structured logging with context IDs for observability
The TODO for timeout enforcement is appropriately documented for v1.1.
tests/integration/test_compute_pipeline_integration.py (2)
1410-1464: LGTM: Excellent concurrency tests verifying thread safety.The concurrent execution tests properly verify that:
- Each execution creates its own context (line 1448)
- Multiple executions don't interfere with each other
- Results are correctly matched to inputs
This aligns with the coding guidelines requirement to respect thread safety constraints.
1536-1746: LGTM: Comprehensive edge case coverage.The edge case tests cover important boundary conditions including:
- All-disabled pipelines returning input unchanged
- Special characters (Unicode, regex metacharacters, newlines)
- Deeply nested structures
- Null/empty values
- Large collections
This provides confidence in the pipeline's robustness.
src/omnibase_core/models/contracts/subcontracts/model_compute_subcontract.py (2)
149-180: LGTM: Unique step name validation correctly implemented.The
@field_validatoron the pipeline field properly enforces unique step names, addressing the requirement from the past review comment. The error message helpfully includes the list of duplicate names for debugging.The use of
field_validatorovermodel_validatoris appropriate here since validation only depends on the pipeline field itself.
73-147: LGTM: Well-designed immutable model following Pydantic best practices.The model correctly implements:
- Immutability via
frozen=Truefor thread safety (per coding guidelines)extra="forbid"to prevent unknown fields- Appropriate field constraints on
pipeline_timeout_ms- Clear default values for optional fields
The docstring comprehensively documents attributes, thread safety, and versioning semantics.
Documentation: - Fix version inconsistencies (FILTER/MAP/REDUCE/SORT → v1.1, Expression Language → v1.1/v1.3) Model Validators: - Add ge=0 constraint to processing_time_ms in ModelComputePipelineResult - Add extra="forbid" to ConfigDict for consistency across compute models - Add frozen=True and ConfigDict to ModelComputeOutput Test Quality: - Add @pytest.mark.unit markers to test_compute_pipeline_error.py - Extract _create_context helper to shared fixture in conftest.py - Tighten test_regex_with_flags assertion with exact match verification - Use specific exception types (ValidationError, AttributeError) for frozen model tests - Move imports to module level in integration tests - Add 13 new tests for compute models CI Verification: All tests pass (3,836+ passed), mypy 0 errors
Code Review: Contract-Driven NodeCompute v1.0 [OMN-465]OverviewThis is an impressive and well-architected implementation of contract-driven compute pipelines. The PR introduces 9,283 additions across 52 files with excellent documentation, comprehensive testing (95 unit tests), and strong adherence to omnibase_core conventions. ✅ Strengths1. Exceptional Documentation (935+ lines)
2. Clean Architecture & Design Patterns✅ Pure Functions: All transformation logic in utility modules (thread-safe by design)
✅ Protocol-Driven: Follows omnibase_core's dependency injection patterns
✅ Error Handling: Uses
3. Strong Type Safety
4. Security Considerations✅ Private Attribute Protection (lines 204-208 in compute_executor.py, 440-449 in compute_transformations.py): elif part.startswith("_"):
raise ModelOnexError(
error_code=EnumCoreErrorCode.VALIDATION_ERROR,
message=f"Cannot access private attribute: '{part}'"
)This prevents path traversal attacks through object introspection while allowing dictionary keys starting with "_" (user data). 5. Testing Strategy
6. Performance & Observability
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
tests/unit/models/container/test_model_service.py (1)
6-6: ImportingValidationErroris appropriate and scopedThe new
ValidationErrorimport is correctly limited to this test module and matches its only usage intest_frozen_model. As a follow-up (not blocking), this file still lacks pytest markers and timeouts; consider adding@pytest.mark.unitand@pytest.mark.timeout(60)at the class or test level to align with the test guidelines.docs/architecture/CONTRACT_DRIVEN_NODECOMPUTE_V1_0.md (2)
192-199: Minor documentation inconsistency:duration_msconstraint not shown.The actual implementation in
model_compute_step_metadata.pyusesduration_ms: float = Field(ge=0)to enforce non-negative values, but this code snippet omits the constraint. Consider updating for accuracy:class ModelComputeStepMetadata(BaseModel): """Metadata for a single pipeline step execution.""" - duration_ms: float + duration_ms: float = Field(ge=0) transformation_type: str | None = None model_config = ConfigDict(frozen=True)
198-199: Documentation shows inconsistentConfigDictsettings across model snippets.
ModelComputeStepMetadata(line 198) showsConfigDict(frozen=True)but the actual implementation usesConfigDict(extra="forbid", frozen=True). Meanwhile,ModelComputeStepResult(line 217) correctly showsextra="forbid". Consider aligning for consistency:- model_config = ConfigDict(frozen=True) + model_config = ConfigDict(extra="forbid", frozen=True)tests/integration/test_compute_pipeline_integration.py (2)
172-175: Consider making error message assertion more flexible.The assertion checking for "string" or "str" in the error message works but could be brittle if error message format changes. Consider using a more general assertion or documenting that this checks for type-related error hints:
# Current - works but coupled to message format assert ( "string" in result.error_message.lower() or "str" in result.error_message.lower() ) # Alternative - just verify an error message exists (already tested above) # The specific message format is an implementation detailThis is a minor observation - the current approach is acceptable for integration tests where you want to verify meaningful error messages.
837-871: Consider removing unused fixture parameter.The
compute_execution_context_factoryfixture is declared but not used intest_context_ids_are_preservedsince the test creates its own context with specific IDs. This is appropriate for the test's purpose, but the unused parameter could be removed for clarity:def test_context_ids_are_preserved( - self, compute_execution_context_factory: ComputeContextFactory + self, ) -> None:Alternatively, keep it for consistency with other tests in the file - this is a minor style preference.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
docs/architecture/CONTRACT_DRIVEN_NODECOMPUTE_V1_0.md(1 hunks)src/omnibase_core/models/compute/model_compute_execution_context.py(1 hunks)src/omnibase_core/models/compute/model_compute_pipeline_result.py(1 hunks)src/omnibase_core/models/compute/model_compute_step_metadata.py(1 hunks)src/omnibase_core/models/model_compute_output.py(2 hunks)tests/integration/conftest.py(1 hunks)tests/integration/test_compute_pipeline_integration.py(1 hunks)tests/unit/errors/test_compute_pipeline_error.py(1 hunks)tests/unit/mixins/test_mixin_completion_data.py(2 hunks)tests/unit/models/compute/test_compute_models.py(1 hunks)tests/unit/models/container/test_model_service.py(2 hunks)tests/unit/utils/test_model_field_converter.py(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- tests/unit/models/compute/test_compute_models.py
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/unit/errors/test_compute_pipeline_error.py
- src/omnibase_core/models/model_compute_output.py
- src/omnibase_core/models/compute/model_compute_execution_context.py
- tests/integration/conftest.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. Usepoetry runfor pytest, mypy, black, isort, and other tools. Never use pip directly or execute python commands without the poetry run prefix.
Enforce 100% strict mypy compliance. All functions must have complete type annotations. Usepoetry run mypy src/omnibase_core/to verify strict mode compliance with no errors.
Format code with black, sort imports with isort, and lint with ruff. Runpoetry run black src/ tests/,poetry run isort src/ tests/, andpoetry run ruff check --fix src/ tests/
Files:
tests/unit/mixins/test_mixin_completion_data.pytests/unit/models/container/test_model_service.pytests/integration/test_compute_pipeline_integration.pytests/unit/utils/test_model_field_converter.pysrc/omnibase_core/models/compute/model_compute_step_metadata.pysrc/omnibase_core/models/compute/model_compute_pipeline_result.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Write tests using pytest with async support via pytest-asyncio. Maintain minimum 60% code coverage. Use pytest markers: @pytest.mark.unit, @pytest.mark.integration, @pytest.mark.slow
Add @pytest.mark.timeout() to prevent test hangs. Run tests withpoetry run pytest tests/ --timeout=60for 60-second timeout protection.
Files:
tests/unit/mixins/test_mixin_completion_data.pytests/unit/models/container/test_model_service.pytests/integration/test_compute_pipeline_integration.pytests/unit/utils/test_model_field_converter.py
src/omnibase_core/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/omnibase_core/**/*.py: Use protocol names withcontainer.get_service()for dependency injection, never resolve services by concrete class names. Example:container.get_service('ProtocolEventBus')instead ofcontainer.get_service('EventBusService')
Always use ModelONEXContainer for dependency injection in node constructors and service resolution. Never confuse it with ModelContainer[T], which is a generic value wrapper, not a DI container.
Use structured error handling with ModelOnexError and EnumCoreErrorCode, never raise generic Exception. Always chain exceptions withfrom ewhen catching and re-raising.
Use the @standard_error_handling decorator on methods to eliminate 6+ lines of try/catch boilerplate. Example:@standard_error_handlingabove async methods.
Respect thread safety constraints: Do NOT share node instances (NodeCompute, NodeEffect), cache, or circuit breaker instances across threads without synchronization. Use thread-local instances or locks.
Use duck typing with protocols. Do not use isinstance checks with protocol types. Rely on the protocol interface directly.
Use EnumNodeKind for high-level architectural classification (EFFECT, COMPUTE, REDUCER, ORCHESTRATOR) when routing data through the ONEX pipeline. Use EnumNodeType for specific implementation discovery and capability matching.
Use ModelEventEnvelope for inter-service event-driven communication. Process events through the envelope pattern for consistent event handling.
Files:
src/omnibase_core/models/compute/model_compute_step_metadata.pysrc/omnibase_core/models/compute/model_compute_pipeline_result.py
src/omnibase_core/models/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use Pydantic 2.11+ models for all data validation and serialization. Ensure models are immutable after creation for thread safety.
Files:
src/omnibase_core/models/compute/model_compute_step_metadata.pysrc/omnibase_core/models/compute/model_compute_pipeline_result.py
🧠 Learnings (1)
📚 Learning: 2025-12-06T23:34:11.788Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T23:34:11.788Z
Learning: Applies to src/omnibase_core/models/**/*.py : Use Pydantic 2.11+ models for all data validation and serialization. Ensure models are immutable after creation for thread safety.
Applied to files:
tests/unit/mixins/test_mixin_completion_data.pytests/unit/models/container/test_model_service.py
🧬 Code graph analysis (1)
src/omnibase_core/models/compute/model_compute_pipeline_result.py (1)
src/omnibase_core/models/compute/model_compute_step_result.py (1)
ModelComputeStepResult(64-122)
⏰ 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: claude-review
- GitHub Check: Smoke Tests
🔇 Additional comments (16)
tests/unit/utils/test_model_field_converter.py (1)
456-458: LGTM! Excellent test precision improvement.Changing from catching generic
Exceptionto the more specificAttributeErrormakes the test assertion more precise and accurate. The comment clearly explains the Python version compatibility consideration, which is helpful for maintainability.tests/unit/models/container/test_model_service.py (1)
59-60: Stricter frozen-model assertion matches Pydantic v2 behaviorUsing
pytest.raises(ValidationError, match="frozen")when mutating a frozenModelServicealigns with Pydantic v2’s behavior, where frozen models raiseValidationError(error typefrozen_instance) on attribute assignment.(deepwiki.com) The# type: ignore[misc]is also appropriate to satisfy strict mypy while exercising the mutation. This strengthens the immutability contract for service models and matches the Pydantic-based immutability learning for this repo. Based on learnings, this looks correct.tests/unit/mixins/test_mixin_completion_data.py (2)
5-7: ImportingValidationErroris appropriate for frozen Pydantic modelsUsing
ValidationErrorfrompydanticfor the immutability assertions is correct and keeps the dependency explicit in this test module. No changes needed here.
156-168: Test implementation is correct; error message matching is reliable and aligns with codebase patternsThe
test_mixin_completion_data_immutablecorrectly validates thatMixinCompletionDataraisesValidationErrorwhen attempting to assign to a frozen instance. Pydantic v2.12.5 (pinned in your project) raisesValidationErrorwith error typefrozen_instanceand message text containing "frozen" when assigning to a frozen model field, making thematch="frozen"assertion reliable.This approach is consistent with existing frozen model tests across your codebase (e.g.,
test_compute_models.py,test_model_service.py). No changes required.For future reference, you could optionally assert on the error type directly (
exc.value.errors()[0]["type"] == "frozen_instance") for slightly more precision, but this is not necessary given the message text is stable.src/omnibase_core/models/compute/model_compute_step_metadata.py (1)
1-77: LGTM! Well-structured immutable metadata model.The model correctly implements:
- Immutability via
frozen=Truefor thread safetyextra="forbid"for strict validation- Proper constraint on
duration_mswithge=0- Clear documentation with thread safety notes and examples
docs/architecture/CONTRACT_DRIVEN_NODECOMPUTE_V1_0.md (1)
1-935: Comprehensive v1.0 specification document - good overall structure.The document effectively covers:
- Clear design principles and scope boundaries
- Detailed model definitions with validation rules
- Well-defined execution semantics (abort-on-first-failure, path resolution)
- Explicit roadmap for deferred features (v1.1, v1.2)
- Actionable acceptance criteria
The previous review comments regarding
ModelValidationStepConfignaming and version alignment have been addressed.src/omnibase_core/models/compute/model_compute_pipeline_result.py (2)
53-122: LGTM! Well-documented pipeline result model.The model correctly implements:
- Comprehensive field documentation with success/failure semantics
- Immutability via
frozen=Truefor thread safetyextra="forbid"for strict validationprocessing_time_mswithge=0constraint- Proper forward reference typing for
step_results
125-130: Forward reference resolution pattern is correct and necessary.The runtime import with
model_rebuild()at lines 126-130 is the correct approach for Pydantic 2.x when usingfrom __future__ import annotationswithTYPE_CHECKING. Since the fieldstep_results: dict[str, "ModelComputeStepResult"]needs the actual class available at runtime for proper model schema resolution, the deferred import and explicit rebuild are necessary and idiomatic. No circular import issues exist; the dependency chain is clean (pipeline_result → step_result → step_metadata).tests/integration/test_compute_pipeline_integration.py (8)
1-56: LGTM! Well-organized imports and module docstring.The imports are properly organized with standard library, third-party (pytest), and local imports separated. The module docstring clearly describes the test coverage scope.
58-66: Good test class structure with appropriate markers.The class correctly uses:
@pytest.mark.integrationfor integration test categorization@pytest.mark.timeout(60)to prevent test hangs (per coding guidelines)- Clear class docstring explaining the fixture usage
67-118: LGTM! Solid multi-step pipeline test.Good coverage of sequential execution verification including step order and final output correctness.
680-827: LGTM! Comprehensive error scenario coverage.Tests properly verify:
- Invalid regex pattern handling
- Missing mapping path detection
- Nonexistent step reference failures
- Type mismatch propagation in middle of pipeline
All tests correctly verify that
error_stepanderror_messageare populated on failure.
1430-1437: Good: Extended timeout for concurrency tests.The 120-second timeout appropriately accounts for concurrent execution overhead. The test class docstring correctly explains this.
1439-1494: LGTM! Well-designed concurrency test.The test correctly:
- Creates fresh context for each execution (line 1477)
- Uses frozen/immutable contract (safe to share across threads)
- Collects results from all futures before assertions
- Verifies each execution produces correct output for its input
This aligns with the coding guidelines note about thread safety for immutable models.
1702-1742: LGTM! Good coverage of null and empty value handling.The test comprehensively covers:
Noneinput- Empty strings, lists, and dicts
- Nested structures with null/empty values
This ensures the identity transformation handles edge cases correctly.
1781-1782: LGTM! Standard pytest entry point.The
if __name__ == "__main__"block allows running tests directly viapython test_compute_pipeline_integration.py -v.
…N-465] Address all remaining PR #115 review issues for release-ready state: ## Critical & Major Fixes - Add pytest markers (@pytest.mark.unit) to 20+ test classes - Fix error labels: transformation_error → compute_error - Add ge=0 constraint to duration_ms and processing_duration_ms fields - Fix node_id=None serialization to avoid "None" string output - Align transform_identity signature with other handlers - Simplify IDENTITY transformation special case ## Test Improvements - Replace generic Exception with ValidationError/OnexError in tests - Create shared fixture for CLI command tests (conftest.py) - Update frozen model tests to use specific exception types ## Documentation - Document $input alias and path resolution semantics - Document global state limitation with TODO(v1.1) - Add EBNF grammar for path expressions ## New Files - src/omnibase_core/utils/compute_path_resolver.py - Shared path resolution utility - tests/unit/utils/test_compute_path_resolver.py - 63 comprehensive tests - tests/unit/models/core/conftest.py - Shared test fixtures ## Linear Tickets Created - OMN-497: Global state refactor - OMN-498: Error type enum - OMN-499: DRY path resolution - OMN-500: Schema validation - OMN-501: Timeout enforcement - OMN-502: Structured logging - OMN-503: TransformationError All tests pass, mypy --strict shows 0 errors.
Code Review: Contract-Driven NodeCompute v1.0I've completed a comprehensive review of this PR. Overall, this is exceptionally well-designed and implemented foundational work. The code demonstrates strong architectural principles, comprehensive testing, and excellent documentation. ✅ Strengths1. Excellent Architecture & Design Principles
2. Code Quality
3. Testing
4. Documentation
|
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 (1)
tests/unit/contracts/test_model_effect_retry_config.py (1)
175-233: Update exception types in validation tests for consistency with Pydantic behavior.All five fields (
max_attempts,base_delay_ms,max_delay_ms,circuit_breaker_threshold,circuit_breaker_timeout_s) use identical Pydantic Field validation withge=1constraint. Lines 183–210 incorrectly expect genericExceptionfor the first three fields, while lines 217–232 correctly expectValidationErrorfor the last two. Pydantic field constraints raiseValidationError, not genericException.Update lines 183, 186, 194, 197, 206, and 209 to expect
ValidationErrorinstead ofExceptionto match actual Pydantic behavior and maintain test consistency across all minimum-value validations.
🧹 Nitpick comments (13)
tests/unit/nodes/test_rollback_failure_handling.py (1)
27-357: Add timeout markers to prevent test hangs.All async test methods should include
@pytest.mark.timeout()markers to prevent potential hangs during CI execution, as specified in the coding guidelines.As per coding guidelines, add timeout markers to all async test methods. For example:
+ @pytest.mark.timeout(60) @pytest.mark.asyncio async def test_rollback_returns_success_when_all_operations_succeed(self):Apply this pattern to all 10 async test methods in both test classes.
tests/unit/models/security/test_model_permission.py (1)
1-27: Consider adding pytest markers and timeout to align with test guidelinesGiven this is a unit test module, you may want to add a module- or class-level
@pytest.mark.unitand@pytest.mark.timeout(...)(or ensure they’re applied via shared fixtures/config) to match the stated testing guidelines, even though these tests are fast in practice.src/omnibase_core/models/operations/model_event_metadata.py (1)
72-76: ModelEventMetadata remains mutable; consider alignment with immutable‑model guidelineUnrelated to this field change,
ModelEventMetadatais still mutable (validate_assignment=Trueandexecute()mutating viasetattr), which diverges from the guideline that models undersrc/omnibase_core/modelsshould be immutable after creation for thread safety. If this model is used across threads or shared widely, consider a future refactor toward immutability (e.g.,frozen=True/no in‑place mutation, usingmodel_copy(update=...)instead), or explicitly document this as an intentional exception so consumers know it is safe to mutate.Also applies to: 80-88
tests/unit/models/core/conftest.py (2)
19-23: default_version fixture is simple and correct; optionally add a return typeThe fixture is clear and does exactly what tests need. To better align with the “all functions annotated”/strict-typing guideline, you could give it an explicit return type:
-@pytest.fixture -def default_version(): +@pytest.fixture +def default_version() -> ModelSemVer: """Provide default SemVer version for tests.""" return ModelSemVer(major=1, minor=0, patch=0)This keeps usage unchanged while making the type obvious to readers and future mypy runs, should you extend checking to tests as well. As per coding guidelines, this helps maintain strict typing consistency.
25-55: create_test_command factory fixture looks good; consider typing its interfaceThe factory pattern and default values are solid and make tests concise. To make the fixture’s contract explicit and keep type discipline consistent with the rest of the project, you could:
- Annotate the fixture’s parameter and return type.
- Import
Callablefor the factory type.Example:
from collections.abc import Callable # near the other imports-@pytest.fixture -def create_test_command(default_version): +@pytest.fixture +def create_test_command( + default_version: ModelSemVer, +) -> Callable[[str, str, str], ModelCliCommandDefinition]: @@ - def _create( - command_name: str, node_name: str, category: str = "general" - ) -> ModelCliCommandDefinition: + def _create( + command_name: str, + node_name: str, + category: str = "general", + ) -> ModelCliCommandDefinition: return ModelCliCommandDefinition( version=default_version, command_name=command_name, target_node=ModelNodeReference.create_local(node_name), action=command_name, description=f"Test command {command_name}", event_type=ModelEventType( schema_version=default_version, event_name="NODE_START", namespace="onex", description="Test event", ), category=category, )This keeps the runtime behavior identical while making the fixture’s shape explicit for anyone reading or type-checking tests. As per coding guidelines, it prepares you for stricter mypy coverage over tests if you choose to enable it later.
tests/unit/nodes/test_retry_count_tracking.py (1)
35-36: Consider adding@pytest.mark.timeout()to async tests.Per coding guidelines, async tests should have timeout protection to prevent CI hangs. These tests involve file I/O and async operations.
+ @pytest.mark.timeout(30) @pytest.mark.asyncio async def test_retry_count_zero_on_success(self, service_effect):Apply similar timeouts to the other async test methods in this class.
src/omnibase_core/models/common/model_validation_metadata.py (2)
8-11: Consider addingfrozen=Truefor thread safety consistency.Per coding guidelines, Pydantic models should be immutable after creation for thread safety. Other models in this PR (e.g.,
ModelComputeStepMetadata,ModelComputePipelineResult) usefrozen=True. Consider aligning this model if immutability is desirable.- model_config = ConfigDict(extra="allow") + model_config = ConfigDict(extra="allow", frozen=True)
22-25: Consider addingge=0to other count fields for consistency.Since
duration_msnow hasge=0, similar non-negative constraints could apply tofiles_processed,rules_applied,violations_found, etc., as negative counts are semantically invalid.files_processed: int | None = Field( default=None, + ge=0, description="Number of files processed during validation", )src/omnibase_core/mixins/mixin_compute_execution.py (1)
187-192: Duplicate step name check is redundant with model validation.Per
ModelComputeSubcontract(see relevant snippets), afield_validatoralready raisesValueErrorif duplicate step names exist during model instantiation. Since thecontractparameter is typed asModelComputeSubcontract, it has already passed this validation.Consider removing this check or documenting that it's intentionally retained for a different error reporting format (list vs. exception).
tests/unit/models/compute/test_compute_models.py (1)
13-348: Good, focused coverage of model invariantsThe tests exercise minimal/full construction, immutability, extra-field rejection, and non-negative constraints across all compute result models and metadata. This gives solid confidence in the Pydantic config (extra="forbid", frozen=True, ge=0) and how these models will behave in pipelines. If you ever want to trim duplication, the negative-duration / negative-processing-time tests could be parameterized, but that’s optional.
src/omnibase_core/utils/compute_path_resolver.py (1)
317-329: Preserve original path in error context and align input-path error messageTwo small polish points:
- In
resolve_path(Line 317+),pathis mutated when stripping leading"$."/"$", and that normalized value is then passed into_traverse_path_segments. This means error messages andPathResolutionError.pathwon’t reflect the exact user-supplied expression (e.g."$.user.name"becomes"user.name"). Consider preserving anoriginal_pathand passing that into_traverse_path_segmentsfor clearer diagnostics:-def resolve_path( - path: str, +def resolve_path( + path: str, data: Any, *, check_private: bool = True, ) -> Any: @@ - # Handle root-level access - if not path or path == "$": - return data - - # Normalize path: remove leading "$." or "$" if present - if path.startswith("$."): - path = path[2:] - elif path.startswith("$"): - path = path[1:] - - # Split path and traverse - parts = path.split(".") - return _traverse_path_segments(data, parts, path, check_private=check_private) + original_path = path + if not path or path == "$": + return data + + if path.startswith("$."): + path = path[2:] + elif path.startswith("$"): + path = path[1:] + + parts = path.split(".") + return _traverse_path_segments( + data, + parts, + original_path, + check_private=check_private, + )
- In
resolve_input_path(Lines 371–381), invalid-path errors currently say:
Expected '$.input' or '$.input.<field>'but the implementation also supports the
$inputalias. Updating the message to mention$inputas well would make the error text accurately reflect the accepted forms.Also applies to: 371-381
src/omnibase_core/utils/compute_executor.py (1)
117-255: Reduce duplication by delegating tocompute_path_resolver.resolve_pipeline_path
resolve_mapping_pathre-implements path parsing and traversal that now lives inomnibase_core.utils.compute_path_resolver(with richer error context viaPathResolutionError). There’s already a TODO (OMN-499); given the new shared utility is in place, it’s worth collapsing this sooner rather than later to avoid drift.A minimal approach would be:
- Import
resolve_pipeline_pathandPathResolutionError.- Wrap
resolve_pipeline_pathand translatePathResolutionErrorintoModelOnexErroronce, preserving error codes and messages.For example (sketch only):
-from omnibase_core.enums.enum_core_error_code import EnumCoreErrorCode +from omnibase_core.enums.enum_core_error_code import EnumCoreErrorCode +from omnibase_core.utils.compute_path_resolver import ( + PathResolutionError, + resolve_pipeline_path, +) @@ def resolve_mapping_path( path: str, input_data: Any, step_results: dict[str, ModelComputeStepResult], ) -> Any: @@ - if not path.startswith("$"): - raise ModelOnexError( - error_code=EnumCoreErrorCode.VALIDATION_ERROR, - message=f"Invalid path: must start with '$', got '{path}'", - ) - ... - raise ModelOnexError( - error_code=EnumCoreErrorCode.VALIDATION_ERROR, - message=f"Invalid path prefix: '{path}'. Must be '$.input' or '$.steps.<name>'", - ) + try: + # Step results dict holds ModelComputeStepResult; resolver returns either + # the raw output or the object’s `.output` depending on configuration. + return resolve_pipeline_path(path, input_data, step_results) + except PathResolutionError as e: + # Preserve error_code and message, just normalize type to ModelOnexError + raise ModelOnexError( + error_code=e.error_code or EnumCoreErrorCode.OPERATION_FAILED, + message=e.message, + context=e.context, + ) from eThat keeps the compute executor’s public surface the same while centralizing path semantics and error-context behavior in one place.
src/omnibase_core/utils/compute_transformations.py (1)
361-371: Delegate JSON_PATH traversal to the shared resolver to avoid drift
transform_json_pathre-implements simple dot-notation traversal that is now centralized incompute_path_resolver.resolve_path. There’s already a TODO (OMN-499); wiring it up now would reduce duplication and keep JSON_PATH semantics in sync with other path consumers.A minimal change could be:
-from omnibase_core.models.transformations.model_transform_json_path_config import ( - ModelTransformJsonPathConfig, -) +from omnibase_core.models.transformations.model_transform_json_path_config import ( + ModelTransformJsonPathConfig, +) +from omnibase_core.utils.compute_path_resolver import resolve_path, PathResolutionError @@ -def transform_json_path( - data: Any, - config: ModelTransformJsonPathConfig, -) -> Any: - ... - path = config.path - # Handle root-level access, normalization, and traversal manually... - ... - return current +def transform_json_path( + data: Any, + config: ModelTransformJsonPathConfig, +) -> Any: + """ + Extract data using simple JSONPath-like path notation. + + Delegates traversal to `resolve_path` to share semantics with other + path-based operations (e.g., mapping steps). + """ + try: + return resolve_path(config.path, data) + except PathResolutionError as e: + # Re-wrap as ModelOnexError to keep transformation-layer error type stable + raise ModelOnexError( + error_code=e.error_code or EnumCoreErrorCode.OPERATION_FAILED, + message=e.message, + context=e.context, + ) from eThis keeps the transformation’s external behavior (raising
ModelOnexErrorwith appropriate codes) while ensuring path semantics and edge cases remain consistent with the shared resolver.Also applies to: 413-471
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
docs/architecture/CONTRACT_DRIVEN_NODECOMPUTE_V1_0.md(1 hunks)src/omnibase_core/infrastructure/node_base.py(1 hunks)src/omnibase_core/mixins/mixin_compute_execution.py(1 hunks)src/omnibase_core/models/common/model_validation_metadata.py(1 hunks)src/omnibase_core/models/operations/model_event_metadata.py(1 hunks)src/omnibase_core/utils/compute_executor.py(1 hunks)src/omnibase_core/utils/compute_path_resolver.py(1 hunks)src/omnibase_core/utils/compute_transformations.py(1 hunks)tests/unit/container/test_container_service_resolver.py(6 hunks)tests/unit/container/test_service_registry.py(2 hunks)tests/unit/contracts/test_model_effect_retry_config.py(2 hunks)tests/unit/core/test_model_base_factory.py(2 hunks)tests/unit/models/compute/test_compute_models.py(1 hunks)tests/unit/models/core/conftest.py(1 hunks)tests/unit/models/core/test_model_container.py(3 hunks)tests/unit/models/health/test_model_health_check.py(3 hunks)tests/unit/models/health/test_model_health_metric.py(2 hunks)tests/unit/models/health/test_model_health_status.py(5 hunks)tests/unit/models/security/test_model_permission.py(2 hunks)tests/unit/nodes/test_retry_count_tracking.py(1 hunks)tests/unit/nodes/test_rollback_failure_handling.py(2 hunks)tests/unit/test_init_exports.py(7 hunks)tests/unit/utils/test_compute_path_resolver.py(1 hunks)tests/unit/utils/test_compute_pipeline.py(1 hunks)tests/unit/utils/test_field_converter.py(1 hunks)tests/unit/validation/test_contracts.py(2 hunks)tests/unit/validation/test_contracts_extended.py(4 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Always use Poetry for Python package management and task execution. Usepoetry runfor pytest, mypy, black, isort, and other tools. Never use pip directly or execute python commands without the poetry run prefix.
Enforce 100% strict mypy compliance. All functions must have complete type annotations. Usepoetry run mypy src/omnibase_core/to verify strict mode compliance with no errors.
Format code with black, sort imports with isort, and lint with ruff. Runpoetry run black src/ tests/,poetry run isort src/ tests/, andpoetry run ruff check --fix src/ tests/
Files:
tests/unit/contracts/test_model_effect_retry_config.pysrc/omnibase_core/infrastructure/node_base.pytests/unit/models/health/test_model_health_check.pytests/unit/models/core/conftest.pytests/unit/utils/test_field_converter.pysrc/omnibase_core/utils/compute_path_resolver.pytests/unit/validation/test_contracts_extended.pytests/unit/nodes/test_rollback_failure_handling.pytests/unit/test_init_exports.pysrc/omnibase_core/models/operations/model_event_metadata.pytests/unit/container/test_container_service_resolver.pytests/unit/models/core/test_model_container.pytests/unit/container/test_service_registry.pytests/unit/models/health/test_model_health_metric.pytests/unit/validation/test_contracts.pytests/unit/nodes/test_retry_count_tracking.pysrc/omnibase_core/utils/compute_transformations.pytests/unit/core/test_model_base_factory.pysrc/omnibase_core/mixins/mixin_compute_execution.pytests/unit/models/security/test_model_permission.pysrc/omnibase_core/models/common/model_validation_metadata.pysrc/omnibase_core/utils/compute_executor.pytests/unit/models/health/test_model_health_status.pytests/unit/utils/test_compute_path_resolver.pytests/unit/utils/test_compute_pipeline.pytests/unit/models/compute/test_compute_models.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Write tests using pytest with async support via pytest-asyncio. Maintain minimum 60% code coverage. Use pytest markers: @pytest.mark.unit, @pytest.mark.integration, @pytest.mark.slow
Add @pytest.mark.timeout() to prevent test hangs. Run tests withpoetry run pytest tests/ --timeout=60for 60-second timeout protection.
Files:
tests/unit/contracts/test_model_effect_retry_config.pytests/unit/models/health/test_model_health_check.pytests/unit/models/core/conftest.pytests/unit/utils/test_field_converter.pytests/unit/validation/test_contracts_extended.pytests/unit/nodes/test_rollback_failure_handling.pytests/unit/test_init_exports.pytests/unit/container/test_container_service_resolver.pytests/unit/models/core/test_model_container.pytests/unit/container/test_service_registry.pytests/unit/models/health/test_model_health_metric.pytests/unit/validation/test_contracts.pytests/unit/nodes/test_retry_count_tracking.pytests/unit/core/test_model_base_factory.pytests/unit/models/security/test_model_permission.pytests/unit/models/health/test_model_health_status.pytests/unit/utils/test_compute_path_resolver.pytests/unit/utils/test_compute_pipeline.pytests/unit/models/compute/test_compute_models.py
src/omnibase_core/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/omnibase_core/**/*.py: Use protocol names withcontainer.get_service()for dependency injection, never resolve services by concrete class names. Example:container.get_service('ProtocolEventBus')instead ofcontainer.get_service('EventBusService')
Always use ModelONEXContainer for dependency injection in node constructors and service resolution. Never confuse it with ModelContainer[T], which is a generic value wrapper, not a DI container.
Use structured error handling with ModelOnexError and EnumCoreErrorCode, never raise generic Exception. Always chain exceptions withfrom ewhen catching and re-raising.
Use the @standard_error_handling decorator on methods to eliminate 6+ lines of try/catch boilerplate. Example:@standard_error_handlingabove async methods.
Respect thread safety constraints: Do NOT share node instances (NodeCompute, NodeEffect), cache, or circuit breaker instances across threads without synchronization. Use thread-local instances or locks.
Use duck typing with protocols. Do not use isinstance checks with protocol types. Rely on the protocol interface directly.
Use EnumNodeKind for high-level architectural classification (EFFECT, COMPUTE, REDUCER, ORCHESTRATOR) when routing data through the ONEX pipeline. Use EnumNodeType for specific implementation discovery and capability matching.
Use ModelEventEnvelope for inter-service event-driven communication. Process events through the envelope pattern for consistent event handling.
Files:
src/omnibase_core/infrastructure/node_base.pysrc/omnibase_core/utils/compute_path_resolver.pysrc/omnibase_core/models/operations/model_event_metadata.pysrc/omnibase_core/utils/compute_transformations.pysrc/omnibase_core/mixins/mixin_compute_execution.pysrc/omnibase_core/models/common/model_validation_metadata.pysrc/omnibase_core/utils/compute_executor.py
src/omnibase_core/models/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use Pydantic 2.11+ models for all data validation and serialization. Ensure models are immutable after creation for thread safety.
Files:
src/omnibase_core/models/operations/model_event_metadata.pysrc/omnibase_core/models/common/model_validation_metadata.py
src/omnibase_core/mixins/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Mixin classes should be composed into node classes for specific capabilities. Available mixins include: MixinDiscoveryResponder, MixinEventHandler, MixinEventListener, MixinNodeExecutor, MixinNodeLifecycle, MixinRequestResponseIntrospection, MixinWorkflowSupport.
Files:
src/omnibase_core/mixins/mixin_compute_execution.py
🧠 Learnings (9)
📚 Learning: 2025-12-06T23:34:11.788Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T23:34:11.788Z
Learning: Applies to tests/**/*.py : Write tests using pytest with async support via pytest-asyncio. Maintain minimum 60% code coverage. Use pytest markers: pytest.mark.unit, pytest.mark.integration, pytest.mark.slow
Applied to files:
tests/unit/utils/test_field_converter.pytests/unit/test_init_exports.pytests/unit/container/test_container_service_resolver.pytests/unit/container/test_service_registry.pytests/unit/utils/test_compute_pipeline.py
📚 Learning: 2025-12-06T23:34:11.788Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T23:34:11.788Z
Learning: Applies to src/omnibase_core/**/*.py : Use structured error handling with ModelOnexError and EnumCoreErrorCode, never raise generic Exception. Always chain exceptions with `from e` when catching and re-raising.
Applied to files:
tests/unit/validation/test_contracts_extended.pytests/unit/models/core/test_model_container.py
📚 Learning: 2025-12-06T23:34:11.788Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T23:34:11.788Z
Learning: Applies to src/omnibase_core/nodes/node_effect.py : NodeEffect is for external interactions (I/O). Inherit from NodeEffect for EFFECT nodes that handle API calls, database operations, file system, or message queues.
Applied to files:
tests/unit/nodes/test_rollback_failure_handling.py
📚 Learning: 2025-12-06T23:34:11.788Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T23:34:11.788Z
Learning: Applies to src/omnibase_core/**/*.py : Use protocol names with `container.get_service()` for dependency injection, never resolve services by concrete class names. Example: `container.get_service('ProtocolEventBus')` instead of `container.get_service('EventBusService')`
Applied to files:
tests/unit/container/test_container_service_resolver.py
📚 Learning: 2025-12-06T23:34:11.788Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T23:34:11.788Z
Learning: Applies to src/omnibase_core/**/*.py : Always use ModelONEXContainer for dependency injection in node constructors and service resolution. Never confuse it with ModelContainer[T], which is a generic value wrapper, not a DI container.
Applied to files:
tests/unit/models/core/test_model_container.py
📚 Learning: 2025-12-06T23:34:11.788Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T23:34:11.788Z
Learning: Applies to src/omnibase_core/models/**/*.py : Use Pydantic 2.11+ models for all data validation and serialization. Ensure models are immutable after creation for thread safety.
Applied to files:
tests/unit/core/test_model_base_factory.pysrc/omnibase_core/models/common/model_validation_metadata.pytests/unit/models/health/test_model_health_status.py
📚 Learning: 2025-12-06T23:34:11.788Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T23:34:11.788Z
Learning: Applies to src/omnibase_core/mixins/**/*.py : Mixin classes should be composed into node classes for specific capabilities. Available mixins include: MixinDiscoveryResponder, MixinEventHandler, MixinEventListener, MixinNodeExecutor, MixinNodeLifecycle, MixinRequestResponseIntrospection, MixinWorkflowSupport.
Applied to files:
src/omnibase_core/mixins/mixin_compute_execution.py
📚 Learning: 2025-12-06T23:34:11.788Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T23:34:11.788Z
Learning: Applies to src/omnibase_core/nodes/node_compute.py : NodeCompute is for data processing and transformation. Inherit from NodeCompute for COMPUTE nodes that perform calculations, validations, and data mapping.
Applied to files:
src/omnibase_core/mixins/mixin_compute_execution.pysrc/omnibase_core/utils/compute_executor.pydocs/architecture/CONTRACT_DRIVEN_NODECOMPUTE_V1_0.md
📚 Learning: 2025-12-06T23:34:11.788Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T23:34:11.788Z
Learning: Applies to tests/**/*.py : Add pytest.mark.timeout() to prevent test hangs. Run tests with `poetry run pytest tests/ --timeout=60` for 60-second timeout protection.
Applied to files:
tests/unit/utils/test_compute_pipeline.py
🧬 Code graph analysis (7)
src/omnibase_core/infrastructure/node_base.py (2)
src/omnibase_core/mixins/mixin_event_driven_node.py (1)
node_id(124-126)src/omnibase_core/mixins/mixin_node_setup.py (1)
node_id(67-68)
tests/unit/models/health/test_model_health_check.py (1)
src/omnibase_core/models/infrastructure/model_timeout.py (1)
timeout_seconds(231-233)
src/omnibase_core/utils/compute_path_resolver.py (1)
src/omnibase_core/enums/enum_core_error_code.py (1)
EnumCoreErrorCode(9-154)
src/omnibase_core/utils/compute_transformations.py (11)
src/omnibase_core/enums/enum_case_mode.py (1)
EnumCaseMode(10-22)src/omnibase_core/enums/enum_core_error_code.py (1)
EnumCoreErrorCode(9-154)src/omnibase_core/enums/enum_regex_flag.py (1)
EnumRegexFlag(10-22)src/omnibase_core/enums/enum_transformation_type.py (1)
EnumTransformationType(11-34)src/omnibase_core/enums/enum_trim_mode.py (1)
EnumTrimMode(10-22)src/omnibase_core/enums/enum_unicode_form.py (1)
EnumUnicodeForm(10-24)src/omnibase_core/models/transformations/model_transform_case_config.py (1)
ModelTransformCaseConfig(15-27)src/omnibase_core/models/transformations/model_transform_json_path_config.py (1)
ModelTransformJsonPathConfig(13-35)src/omnibase_core/models/transformations/model_transform_regex_config.py (1)
ModelTransformRegexConfig(15-31)src/omnibase_core/models/transformations/model_transform_trim_config.py (1)
ModelTransformTrimConfig(15-27)src/omnibase_core/models/transformations/model_transform_unicode_config.py (1)
ModelTransformUnicodeConfig(15-27)
src/omnibase_core/mixins/mixin_compute_execution.py (4)
src/omnibase_core/models/compute/model_compute_execution_context.py (1)
ModelComputeExecutionContext(44-95)src/omnibase_core/models/compute/model_compute_pipeline_result.py (1)
ModelComputePipelineResult(53-122)src/omnibase_core/models/contracts/subcontracts/model_compute_subcontract.py (1)
ModelComputeSubcontract(73-181)src/omnibase_core/utils/compute_executor.py (1)
execute_compute_pipeline(436-648)
src/omnibase_core/utils/compute_executor.py (6)
src/omnibase_core/enums/enum_compute_step_type.py (1)
EnumComputeStepType(50-91)src/omnibase_core/enums/enum_core_error_code.py (1)
EnumCoreErrorCode(9-154)src/omnibase_core/models/compute/model_compute_execution_context.py (1)
ModelComputeExecutionContext(44-95)src/omnibase_core/models/compute/model_compute_pipeline_result.py (1)
ModelComputePipelineResult(53-122)src/omnibase_core/models/contracts/subcontracts/model_compute_pipeline_step.py (1)
ModelComputePipelineStep(81-200)src/omnibase_core/models/contracts/subcontracts/model_compute_subcontract.py (1)
ModelComputeSubcontract(73-181)
tests/unit/utils/test_compute_path_resolver.py (1)
src/omnibase_core/utils/compute_path_resolver.py (5)
PathResolutionError(98-138)resolve_input_path(332-384)resolve_path(272-329)resolve_pipeline_path(487-551)resolve_step_path(387-484)
⏰ 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 (49)
tests/unit/models/core/test_model_container.py (3)
176-187: Aligning map failure expectations with OnexError is correctUsing
pytest.raises(OnexError)fortest_map_value_failurematches the standardized error model and keeps the existing message assertion intact. No further changes needed.
199-209: Validation failure test now correctly targets OnexErrorExpecting
OnexErrorfortest_validate_with_failureis consistent with the project’s error-handling approach and with other tests in this module. The assertions on message and container state remain appropriate.
210-225: Validator-exception path correctly asserts OnexError wrappingSwitching
test_validate_with_exception_in_validatortopytest.raises(OnexError)is aligned with the design where validator crashes are surfaced viaOnexError, while still checking that the wrapped message contains the originalValueErrortext. Looks good.tests/unit/container/test_service_registry.py (2)
37-38: Unit marker onTestServiceRegistryis appropriateTagging this test class with
@pytest.mark.unitcleanly classifies the suite without changing behavior and aligns with the stated pytest marker conventions.
As per coding guidelines, this follows the preferred test marking approach.
397-398: Unit marker onTestContainerIntegrationlooks goodApplying
@pytest.mark.unithere consistently tags these container–registry interaction tests as unit-level, matching the rest of the suite’s marker strategy.
Based on learnings, this is in line with how tests are expected to be marked.tests/unit/nodes/test_rollback_failure_handling.py (2)
23-23: LGTM! Correct unit marker addition.The
@pytest.mark.unitmarker correctly categorizes this test class and aligns with the project's testing conventions.
158-158: LGTM! Correct unit marker addition.The
@pytest.mark.unitmarker correctly categorizes this test class.tests/unit/models/security/test_model_permission.py (2)
25-27: ImportingValidationErroraligns tests with Pydantic semanticsUsing
from pydantic import ValidationErrorhere is appropriate and keeps the tests asserting against the concrete validation error type instead of a broad exception.
152-160: Narrowing the expected exception toValidationErroris a solid improvementSwitching this test to
with pytest.raises(ValidationError):correctly ties the invalid name pattern case to Pydantic’s model validation rather than a generic exception, improving test precision and documenting the intended contract.tests/unit/container/test_container_service_resolver.py (1)
26-27: Class-level@pytest.mark.unitusage is correct and consistentTagging all container resolver test classes with
@pytest.mark.unitcleanly categorizes these as unit tests and aligns with the stated pytest marker guidelines. No functional behavior changes are introduced, and the markers look complete and consistent across the file.Also applies to: 123-125, 204-207, 251-253, 288-291, 335-337
src/omnibase_core/infrastructure/node_base.py (1)
719-731: Initialization failure logging now robust against missingnode_idThe updated conditional around
node_idin_emit_initialization_failurecorrectly avoids a secondaryAttributeErrorwhen initialization fails beforeself.node_idis set, while still logging the ID when present and non-None. This is a safe robustness improvement with no behavioral regressions evident.src/omnibase_core/models/operations/model_event_metadata.py (1)
46-50: Non‑negativeprocessing_duration_mswith default 0 looks good; verify no callers relied on negative/None sentinelsThe new
Field(default=0, ge=0, description="Processing duration")enforces a sane, non‑negative duration and gives a deterministic default, which aligns with the broader pattern for timing fields in the compute models. Please just double‑check that no existing code paths or stored payloads relied on negative orNonesentinel values to signal “unknown/not started yet”; if they did, they may need a small normalization step to map those to0or another explicit flag (e.g.,processed).tests/unit/contracts/test_model_effect_retry_config.py (1)
397-408: LGTM! Assignment validation correctly expects ValidationError.The updated test correctly expects
ValidationErrorwhen assigning an invalid value tomax_attempts, which is consistent with Pydantic'svalidate_assignment=Truebehavior and matches the pattern used in enum assignment validation tests (lines 167-172).tests/unit/nodes/test_retry_count_tracking.py (1)
31-32: LGTM! Unit marker added as per coding guidelines.The
@pytest.mark.unitmarker is correctly applied to the test class, aligning with the project's test categorization standards.tests/unit/utils/test_field_converter.py (1)
9-10: LGTM! Unit marker correctly applied.The
@pytest.mark.unitmarker is appropriately added to categorize these tests.tests/unit/validation/test_contracts_extended.py (5)
22-22: LGTM! Appropriate import for precise exception handling.Adding the
ValidationErrorimport enables more specific exception assertions, which is better practice than catching broad exception types.
100-101: LGTM! Precise exception type for Pydantic validation failures.Using
ValidationErrorinstead of a generic exception is correct for testing Pydantic model validation when required fields are missing.
129-130: LGTM! Appropriate multi-exception handling for empty YAML edge case.Empty YAML strings yield
Nonefromyaml.safe_load, which can cause eitherValidationError(Pydantic) orTypeErrordepending on how the model handlesNoneinput.
136-138: LGTM! Comprehensive exception handling for whitespace-with-tabs edge case.The comment correctly explains that tab characters in YAML can trigger scanner errors. The triple exception tuple appropriately covers all possible failure modes.
148-149: LGTM! Correct exception type for null value validation.
ValidationErroris the appropriate exception when Pydantic rejects null values for required fields.src/omnibase_core/models/common/model_validation_metadata.py (1)
17-21: LGTM! Non-negativity constraint correctly applied.Adding
ge=0toduration_msensures validation rejects negative durations, which aligns with the non-negative duration semantics used in other compute models (e.g.,ModelComputeStepMetadata).src/omnibase_core/mixins/mixin_compute_execution.py (5)
1-36: LGTM! Comprehensive module documentation.Excellent documentation including purpose, thread safety notes, usage examples, and cross-references. This follows best practices for mixin documentation.
91-148: LGTM! Async wrapper with appropriate documentation.The async wrapper correctly documents that the underlying execution is synchronous (lines 104-107). For compute pipelines that are primarily data transformations, this is acceptable. If pipelines become CPU-intensive, consider using
asyncio.to_thread()to avoid blocking the event loop.
194-207: LGTM! Correct forward-reference validation logic.The path parsing correctly handles:
$.steps.<name>.outputformat with proper prefix removal- Skips non-step paths like
$.input(line 199 condition)- Tracks executed steps incrementally to catch forward references
This is valuable static analysis that catches configuration errors before runtime.
88-89: Type hint documents mixing class contract.The
node_id: UUIDdeclaration correctly documents the expected attribute from the mixing class while allowing the method (line 142) to handle cases where it might be absent viahasattr.
212-214: LGTM! Proper__all__export.The mixin is correctly exported for public API access.
docs/architecture/CONTRACT_DRIVEN_NODECOMPUTE_V1_0.md (1)
1-1001: LGTM! Comprehensive and well-structured v1.0 specification.This specification document is thorough, well-organized, and provides clear guidance for implementation. The design decisions are sound:
- Sequential-only pipelines with abort-on-first-failure semantics provide a solid foundation
- Six built-in transformations cover common use cases without over-engineering
- Immutability guarantees (
frozen=True) are properly documented and enforced- String-based error types in v1.0 with a clear upgrade path to enums in v1.1+
- Example contract demonstrates all features correctly
The document successfully balances v1.0 simplicity with clear extensibility for v1.1/v1.2 features.
tests/unit/models/health/test_model_health_metric.py (2)
4-4: LGTM! Import addition improves test specificity.Adding the explicit
ValidationErrorimport from Pydantic enables more precise exception testing.
59-62: LGTM! More specific exception handling.Updating from generic
ExceptiontoValidationErrormakes the test more precise and aligns with Pydantic's validation model. This change correctly validates that invalid trend values trigger Pydantic's validation mechanism.tests/unit/models/health/test_model_health_status.py (2)
7-7: LGTM! Import addition enables precise exception testing.Adding the explicit
ValidationErrorimport from Pydantic supports more specific exception assertions throughout the test suite.
74-108: LGTM! Improved test precision with ValidationError.These updates correctly expect
ValidationErrorfor various validation failures:
- Invalid status patterns (line 74)
- Out-of-range health scores (lines 85, 87)
- Invalid check durations (line 97)
- Invalid uptime values (line 107)
This aligns with Pydantic's validation model and makes the tests more specific and maintainable.
tests/unit/test_init_exports.py (1)
18-280: LGTM! Proper test categorization with pytest markers.All test classes now correctly include the
@pytest.mark.unitdecorator, which enables proper test filtering and categorization. This aligns with the coding guidelines requirement for pytest markers on all test classes.As per coding guidelines: "Use pytest markers: @pytest.mark.unit, @pytest.mark.integration, @pytest.mark.slow"
tests/unit/validation/test_contracts.py (2)
7-7: LGTM! Import supports refined exception handling.Adding
ValidationErrorimport enables more precise exception testing in YAML loading scenarios.
39-48: LGTM! More precise exception expectations for YAML edge cases.These updates correctly refine exception handling:
- Line 39: Empty YAML may raise either
ValidationError(Pydantic validation) orTypeError(Python type errors), properly covering both cases- Line 47: Invalid YAML syntax correctly expects
yaml.YAMLErrorfor parsing failuresThis improves test accuracy and reduces false positives.
tests/unit/core/test_model_base_factory.py (2)
9-9: LGTM! Import addition for precise validation testing.Adding
ValidationErrorto the existing Pydantic imports enables more specific exception assertions in factory validation tests.
232-233: LGTM! Precise exception handling for factory validation.Updating from a placeholder generic
ExceptiontoValidationErrorcorrectly validates that Pydantic's validation mechanism is triggered when invalid data (string instead of float for price) is passed to the factory'screatemethod.tests/unit/models/health/test_model_health_check.py (2)
4-4: LGTM! Import enables precise validation testing.Adding
ValidationErrorto the Pydantic imports supports more specific exception assertions for health check validation tests.
164-203: LGTM! Precise validation error expectations.These updates correctly expect
ValidationErrorfor validation failures:
- Lines 164-175: Invalid HTTP status codes (99 and 600 outside valid 100-599 range)
- Lines 192-203: Invalid timeouts (0 and 301 outside valid 1-300 range)
This aligns with Pydantic's field validation and makes the tests more specific and maintainable.
tests/unit/utils/test_compute_pipeline.py (10)
1-58: LGTM! Comprehensive imports and proper file header.The imports correctly cover all necessary enums, models, and utilities for testing the v1.0 NodeCompute pipeline. The docstring clearly identifies this as testing the Contract-Driven NodeCompute v1.0 implementation.
60-80: LGTM! Complete identity transformation tests.The identity transformation tests correctly verify:
- Returns input unchanged for various types (string, int, dict, None)
- Preserves lists and nested structures
This aligns with the v1.0 spec requirement that IDENTITY has no config and passes data through unchanged.
82-133: LGTM! Comprehensive regex transformation tests.Tests correctly cover:
- Simple replacement patterns
- All three regex flags (IGNORECASE, MULTILINE, DOTALL)
- Error handling for non-string input
- Error handling for invalid regex patterns
This thoroughly validates the REGEX transformation type from the v1.0 spec.
135-164: LGTM! Complete case transformation tests.Tests correctly validate all three case modes (UPPER, LOWER, TITLE) and error handling for non-string input and empty strings. Aligns with the CASE_CONVERSION transformation in the v1.0 spec.
166-195: LGTM! Thorough trim transformation tests.Tests correctly cover all three trim modes (BOTH, LEFT, RIGHT), error handling for non-string input, and edge case of no whitespace. Validates the TRIM transformation type from the v1.0 spec.
197-221: LGTM! Unicode normalization tests.Tests correctly validate NFC/NFD normalization forms and error handling for non-string input. The composed/decomposed character tests properly exercise the NORMALIZE_UNICODE transformation.
223-257: LGTM! JSONPath extraction tests.Tests comprehensively cover:
- Simple field extraction
- Nested field access
- Error handling for missing fields
- Root access with
$- Deeply nested paths
This validates the JSON_PATH transformation type from the v1.0 spec.
259-297: LGTM! Transformation dispatcher tests.Tests correctly validate the
execute_transformationdispatcher:
- IDENTITY works without config
- Non-IDENTITY transformations require config
- Dispatching to each transformation type works correctly
This ensures the transformation registry pattern works as specified.
299-375: LGTM! Comprehensive path resolution tests.Tests thoroughly validate
resolve_mapping_pathfor:
- Full input access (
$.input)- Input field access (
$.input.<field>)- Nested input fields
- Step output access (both explicit
.outputand shorthand forms)- Error cases (invalid prefix, missing fields, missing steps, invalid subpaths)
This aligns with the v1.0 mapping path grammar specified in the contract document.
377-586: LGTM! Comprehensive pipeline execution tests with proper timeout protection.The pipeline executor tests thoroughly validate:
- Simple and multi-step pipelines
- Abort-on-first-failure semantics (line 442-470)
- Identity transformation in pipelines
- Disabled step handling
- Empty pipeline behavior
- Timing/metadata tracking
- Regex transformation in pipelines
The
@pytest.mark.timeout(30)decorator (line 378) correctly prevents test hangs during pipeline execution, as required by coding guidelines.As per coding guidelines: "Add @pytest.mark.timeout() to prevent test hangs."
src/omnibase_core/utils/compute_executor.py (1)
436-648: Executor semantics align with v1.0 spec
execute_compute_pipelinecorrectly implements sequential execution with abort-on-first-failure, records per-step duration and metadata, distinguishesModelOnexErrorfrom unexpected exceptions, and returns an immutableModelComputePipelineResultwith detailed error context. The handling of disabled steps andsteps_executedordering also matches the contract comments.
…MN-465] - docs: add $input alias documentation, fix code block languages, align versions - tests: add @pytest.mark.timeout() to all compute test classes - tests: fix test_path_with_consecutive_dots to use actual consecutive dots - tests: use specific ValidationError instead of generic Exception - src: align transform_identity signature with other handlers - src: add ge=0 constraint to retry_count, document model mutability - src: add thread safety documentation to NodeBase - src: improve error message context with original path
Pull Request Review: Contract-Driven NodeCompute v1.0 [OMN-465]OverviewThis is a well-architected implementation of contract-driven compute pipelines with excellent documentation, comprehensive testing, and strong adherence to the project's coding standards. The PR introduces 10,728 additions across 73 files with a minimal, stable foundation approach that can be safely shipped and extended incrementally. ✅ Strengths1. Architecture & Design
2. Code Quality
3. Security✅ No dangerous patterns found:
4. Documentation
|
| Category | Status | Notes |
|---|---|---|
| Code Injection | ✅ Safe | No eval/exec/compile |
| Path Traversal | Private attrs blocked, but needs allowlist | |
| ReDoS | 🔴 Vulnerable | User regex patterns need timeout |
| Input Validation | Pydantic validation good, but no size limits | |
| DoS via Timeout | 🔴 Vulnerable | Timeout not enforced |
📝 Final Recommendations
Must Fix Before Merge (P0):
- Implement ReDoS protection (timeout or pattern validation)
- Implement pipeline timeout enforcement
- Add input size validation
- Fix or document global mutable state as technical debt
Should Fix Before v1.0 GA (P1):
- Add missing edge case tests (concurrent execution, large inputs, ReDoS)
- Improve error messages with examples
- Add security testing to CI pipeline
Nice to Have (P2):
- Path resolution optimization
- More user-friendly validation error messages
- Performance benchmarks
✨ Conclusion
This is excellent work with a solid architectural foundation. The code quality, documentation, and test coverage are exemplary. The v1.0 scope is appropriately minimal and the roadmap is clear.
However, the security issues (ReDoS and missing timeout enforcement) are blocking for production use. Once these are addressed, this PR will be ready to merge.
Estimated Effort to Fix Critical Issues: 4-8 hours
- ReDoS protection: 2-3 hours (testing + implementation)
- Timeout enforcement: 2-3 hours (implementation + tests)
- Input validation: 1-2 hours (straightforward)
Recommendation: Request Changes - Address security concerns before merge
The architecture is sound and the implementation is high-quality. Address the security concerns and this will be production-ready.
Reviewed by: Claude Code (Automated PR Review)
Review Date: 2025-12-07
Files Reviewed: 73 changed files (+10,728 / -50)
Tests Found: 367 test occurrences across 11 test files
Code Review: Contract-Driven NodeCompute v1.0 [OMN-465]OverviewThis is an exceptionally well-crafted PR that implements the v1.0 contract-driven NodeCompute specification. The implementation demonstrates strong engineering discipline with excellent documentation, comprehensive testing, and adherence to ONEX architecture principles. Summary: ✅ APPROVE with minor recommendations for future iterations. 🎯 Strengths1. Architecture & Design Excellence
2. Code Quality
3. Testing
4. Documentation
🔍 Code Quality AnalysisExcellent Patterns Found1. Thread-Safe Global State with Clear Documentation# compute_executor.py:54-59
# TODO(OMN-497): Refactor to context-based warning tracking instead of global state.
# This global mutable state is a known exception to the "pure and stateless" principle.
# It affects only warning output (not computation results) and pipeline execution
# remains deterministic.
_validation_warning_emitted = FalseAnalysis: While global state is generally problematic, this is well-documented as:
2. Security-First Path Resolution# compute_executor.py:209-214
# Block private attribute access for security
elif part.startswith("_"):
raise ModelOnexError(
error_code=EnumCoreErrorCode.VALIDATION_ERROR,
message=f"Path '{path}' invalid: cannot access private attribute '{part}'",
)Analysis: Excellent security practice preventing internal state exposure. 3. Explicit Unused Parameter Handling# compute_transformations.py:167-169
def transform_identity(data: Any, config: ModelTransformationConfig | None) -> Any:
# config parameter is intentionally unused - exists for registry uniformity
del config # Explicitly mark as unused to satisfy linters
return dataAnalysis: Shows attention to detail and consideration for tooling. 4. Comprehensive Error Context# compute_transformations.py:217-226
except re.error as e:
raise ModelOnexError(
error_code=EnumCoreErrorCode.OPERATION_FAILED,
message=f"Invalid regex pattern: {e}",
context={
"transform_type": "REGEX",
"pattern": config.pattern,
"replacement": config.replacement,
"regex_error": str(e),
},
) from eAnalysis: Excellent structured error handling with full context preservation. 🐛 Potential Issues1. Pipeline Timeout Not Enforced (Known Issue)File: # TODO(OMN-501): Enforce pipeline_timeout_ms - currently declared in contract but not enforced.Impact: Low (documented as v1.0 limitation) 2. Validation Step Pass-Through (Known Issue)File: 3. Duplicate Path Resolution LogicFiles:
Impact: Low (isolated duplication, no bugs) 4. Missing Type Guard for Step ResultsFile: result = step_results[step_name]
if len(parts) == 1:
return result.output # No check if result.success == FalseImpact: Low (abort-on-first-failure prevents failed steps from being referenced) 🔒 Security Analysis✅ Secure
|
Comprehensive PR Review: Contract-Driven NodeCompute v1.0Overall Assessment: EXCELLENT - Ship It! ✅This is exceptionally well-crafted with strong ONEX principles adherence, comprehensive testing, and production-ready quality. Summary: 10,724 additions / 50 deletions across 73 files ✅ StrengthsArchitecture & Design (Outstanding)
Code Quality (Excellent)
Testing (Comprehensive)
Documentation (Production-Ready)
ONEX Compliance (Perfect)
🔍 Review FindingsCritical Issues: NONE ✅ Minor Observations (Not Blocking)
🛡️ Security: EXCELLENT ✅
⚡ Performance: GOOD ✅
🧪 Test Coverage: EXCELLENT ✅
📋 Best Practices: PERFECT ✅CLAUDE.md Adherence: Python Best Practices: 🚀 RecommendationsBefore Merge: OPTIONAL
Post-Merge: TRACKED ✅
📊 Metrics
✅ APPROVED - Ship It!Production-ready code that: Confidence: Very High Exceptional work on documentation, thread safety, security, testing, and forward-thinking design. This is a model PR for omnibase_core! 🎉 Reviewed by: Claude Code (Sonnet 4.5) |
- Remove unnecessary string quotes from type annotations (UP037) - Remove unused noqa: E402 directive (RUF100) - Minor test cleanup
PR Review: Contract-Driven NodeCompute v1.0 [OMN-465]This is a comprehensive, well-executed implementation of the v1.0 contract-driven NodeCompute specification. The PR demonstrates exceptional attention to detail, thorough testing, and adherence to the repository's strict quality standards. ✅ Overall Assessment: APPROVEDStrengths🎯 Architecture & Design
📚 Documentation Excellence
🧪 Test Coverage
🔧 Code Quality
📝 Detailed Findings1. Code Quality & Best Practices ✅Excellent adherence to CLAUDE.md conventions:
Minor observation (already addressed):
2. Potential Bugs ✅ None FoundThe code demonstrates:
3. Performance Considerations ✅Good performance characteristics:
Deferred optimizations (documented):
4. Security Concerns ✅ Well-AddressedSecurity features:
Documentation:
5. Test Coverage ✅ ExcellentCoverage breakdown:
Test quality:
🎓 RecommendationsRequired Actions: None ✨All critical, major, and minor issues have been addressed in the 12 commit series. Optional Enhancements (Future PRs):
📊 CI/CD StatusExpected CI Behavior:
Verification Needed:
🏆 ConclusionThis PR represents exemplary engineering:
The incremental commit history (12 commits addressing review feedback) demonstrates:
Recommendation: MERGE after CI passes ✅ 📋 Checklist
Review conducted by: Claude Code Agent Great work on this implementation! 🎉 |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (11)
src/omnibase_core/infrastructure/node_base.py (2)
85-91: Docstring thread-safety guidance aligns with architecture constraintsThe new THREAD SAFETY AND STATE section correctly reflects the per-instance mutable state and explicitly warns against sharing node instances across threads, matching the broader threading guidance for ONEX nodes. Referencing the dedicated THREADING guide is also helpful for discoverability; just ensure
docs/guides/THREADING.mdexists or update the path if it moves.Based on learnings, node instances should not be shared across threads without synchronization.
733-735: Safer initialization-failure logging for unsetnode_idConditionally resolving
node_idhere avoids both AttributeError (whennode_idisn’t set yet) and misleading"None"values in logs. Falling back to"unknown"is a sensible default for early initialization failures.You could extract this pattern into a small helper (e.g.,
_safe_node_id_str()) if you find yourself needing the same fallback elsewhere.tests/unit/utils/test_compute_pipeline.py (1)
63-597: Strong, contract-aligned coverage for transformations and pipeline executionThe test suite thoroughly exercises the v1.0 behaviors (sequential pipelines, abort-on-first-failure, disabled steps, timing, regex/JSON_PATH, and path resolution), and it adheres to the pytest marker/timeout guidelines. If you want to tighten end-to-end coverage further, you could optionally add one or two pipeline tests that include VALIDATION and MAPPING steps (in addition to the existing
resolve_mapping_pathunit tests), but this is not blocking.src/omnibase_core/utils/compute_path_resolver.py (1)
98-385: Path resolver design matches the documented v1.0 path grammar
PathResolutionErrorplusresolve_path/resolve_input_path/resolve_step_path/resolve_pipeline_pathgive a clean, pure, and well-typed surface for both JSON_PATH and pipeline paths, with sensible use ofEnumCoreErrorCodeand private-attribute guarding. Implementation matches the grammar and alias rules described in the doc (e.g.,$input⇔$.input,$.steps.<name>⇔$.steps.<name>.output), and should be safe under concurrency. The only tiny polish you might consider is updating theresolve_input_patherror message to also mention the$input/$input.<field>forms since they’re accepted aliases, but behavior is already correct.tests/unit/models/transformations/test_transformation_configs.py (1)
31-223: Config model tests comprehensively cover defaults and validationThis module does a good job exercising minimal/full configs,
config_typediscriminators, immutability, extra-field rejection, and validation error paths for all the transformation, mapping, and validation config models. It’s tightly aligned with the model definitions insrc/omnibase_core/models/transformations/and fits the pytest marker/timeout guidelines. The only tiny style nit is the localimport pytestintest_empty_mappings_raises_error, which could be removed since pytest is already imported at the top, but that’s purely optional.src/omnibase_core/mixins/mixin_compute_execution.py (1)
73-87: Update docstring example to use ModelOnexError for contract validationThe example in the class docstring at lines 73-87 shows:
errors = self.validate_compute_contract(self.contract.compute_operations) if errors: raise ValueError(f"Invalid contract: {errors}")Project guidelines require structured error handling via
ModelOnexErrorandEnumCoreErrorCode. Update the example to useModelOnexError(error_code=EnumCoreErrorCode.VALIDATION_ERROR, message=...)instead ofValueErrorto reflect the framework's error-handling standards and avoid misleading new code that contradicts these guidelines.docs/architecture/NODECOMPUTE_VERSIONING_ROADMAP.md (1)
106-112: Add languages to fenced code blocks (markdownlint MD040) and optionally tweak repeated sentence.Several fenced blocks lack a language spec:
- Lines 106–112 (version number block)
- Lines 132–137 (milestone list)
- Lines 728–734 (schema-freezing pseudo-steps)
- Lines 930–934 (backward-compatibility promise)
To satisfy markdownlint and improve rendering, consider:
-``` +```text 1.x.y ... -``` +```textSimilarly, use
text,bash, or a more specific language for the other blocks.Optionally, you could rephrase one of the consecutive “Boring …” sentences around Line 55 to avoid the repeated-start wording flagged by LanguageTool, but that’s purely stylistic.
Also applies to: 132-137, 728-734, 930-934
tests/integration/test_compute_pipeline_integration.py (2)
579-626: Relax or isolate hardprocessing_time_msthresholds to avoid flaky tests.
test_large_data_performance_baseline,test_many_steps_pipeline, andtest_large_list_inputassert strict upper bounds onprocessing_time_ms(500–1000ms). On slower or contended CI runners, even simple transformations can exceed these bounds and make otherwise-correct code look failing.Consider either:
- Loosening the thresholds substantially, or
- Moving these checks behind a separate marker (e.g., a perf suite) or converting them to logging-only expectations rather than hard assertions.
Also applies to: 629-676, 1744-1777
1273-1342: Add an integration scenario exercising theJSON_PATHtransformation.The v1.0 contract promises six built-in transformations, including
JSON_PATH, but this “all_transformation_types” test currently covers only NORMALIZE_UNICODE, REGEX, TRIM, CASE_CONVERSION, and IDENTITY.To align integration coverage with the v1.0 surface, consider either:
- Adding a
JSON_PATHstep into this pipeline, or- Introducing a dedicated
test_json_path_transformation_in_pipelinethat exercises JSON_PATH against nested input and verifies both success and error paths.src/omnibase_core/models/contracts/subcontracts/model_compute_pipeline_step.py (1)
154-202: Optionally tighten validation to reject stray config on mismatchedstep_type.Currently, the validator enforces required configs for each
step_typebut doesn’t forbid, for example:
step_type=MAPPINGwith a non-Nonetransformation_type/transformation_config, orstep_type=TRANSFORMATIONwith a non-Nonemapping_config.These extra fields are ignored by the executor but could indicate a misconfigured contract.
Consider extending
validate_step_configto assert that non-relevant config fields areNonefor each step type to catch such mistakes early.src/omnibase_core/utils/compute_executor.py (1)
54-60: Be aware of minor race on_validation_warning_emittedunder concurrency.The one-time validation warning is guarded by a module-level
_validation_warning_emittedflag that’s mutated without synchronization. Under concurrent validation steps, multiple threads could observeFalseand emit the warning more than once.It doesn’t affect determinism or results (only warning volume), but if strict “once per process” behavior is desired, consider guarding it with a simple lock or moving warning tracking into a context object when you address OMN-497/OMN-500.
Also applies to: 354-364
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
docs/architecture/CONTRACT_DRIVEN_NODECOMPUTE_V1_0.md(1 hunks)docs/architecture/NODECOMPUTE_VERSIONING_ROADMAP.md(1 hunks)src/omnibase_core/enums/__init__.py(5 hunks)src/omnibase_core/infrastructure/node_base.py(2 hunks)src/omnibase_core/mixins/mixin_compute_execution.py(1 hunks)src/omnibase_core/models/compute/model_compute_pipeline_result.py(1 hunks)src/omnibase_core/models/compute/model_compute_step_result.py(1 hunks)src/omnibase_core/models/contracts/subcontracts/__init__.py(2 hunks)src/omnibase_core/models/contracts/subcontracts/model_compute_pipeline_step.py(1 hunks)src/omnibase_core/models/operations/model_event_metadata.py(2 hunks)src/omnibase_core/utils/compute_executor.py(1 hunks)src/omnibase_core/utils/compute_path_resolver.py(1 hunks)src/omnibase_core/utils/compute_transformations.py(1 hunks)tests/integration/conftest.py(1 hunks)tests/integration/test_compute_pipeline_integration.py(1 hunks)tests/unit/enums/test_compute_enums.py(1 hunks)tests/unit/models/compute/test_compute_models.py(1 hunks)tests/unit/models/transformations/test_transformation_configs.py(1 hunks)tests/unit/utils/test_compute_path_resolver.py(1 hunks)tests/unit/utils/test_compute_pipeline.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/omnibase_core/models/operations/model_event_metadata.py
- src/omnibase_core/models/contracts/subcontracts/init.py
- tests/unit/models/compute/test_compute_models.py
- src/omnibase_core/models/compute/model_compute_step_result.py
🧰 Additional context used
📓 Path-based instructions (5)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Always use Poetry for Python package management and task execution. Usepoetry runfor pytest, mypy, black, isort, and other tools. Never use pip directly or execute python commands without the poetry run prefix.
Enforce 100% strict mypy compliance. All functions must have complete type annotations. Usepoetry run mypy src/omnibase_core/to verify strict mode compliance with no errors.
Format code with black, sort imports with isort, and lint with ruff. Runpoetry run black src/ tests/,poetry run isort src/ tests/, andpoetry run ruff check --fix src/ tests/
Files:
src/omnibase_core/enums/__init__.pysrc/omnibase_core/models/contracts/subcontracts/model_compute_pipeline_step.pysrc/omnibase_core/infrastructure/node_base.pytests/unit/enums/test_compute_enums.pysrc/omnibase_core/utils/compute_transformations.pysrc/omnibase_core/utils/compute_path_resolver.pysrc/omnibase_core/models/compute/model_compute_pipeline_result.pysrc/omnibase_core/mixins/mixin_compute_execution.pytests/unit/utils/test_compute_pipeline.pytests/integration/test_compute_pipeline_integration.pysrc/omnibase_core/utils/compute_executor.pytests/integration/conftest.pytests/unit/models/transformations/test_transformation_configs.pytests/unit/utils/test_compute_path_resolver.py
src/omnibase_core/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/omnibase_core/**/*.py: Use protocol names withcontainer.get_service()for dependency injection, never resolve services by concrete class names. Example:container.get_service('ProtocolEventBus')instead ofcontainer.get_service('EventBusService')
Always use ModelONEXContainer for dependency injection in node constructors and service resolution. Never confuse it with ModelContainer[T], which is a generic value wrapper, not a DI container.
Use structured error handling with ModelOnexError and EnumCoreErrorCode, never raise generic Exception. Always chain exceptions withfrom ewhen catching and re-raising.
Use the @standard_error_handling decorator on methods to eliminate 6+ lines of try/catch boilerplate. Example:@standard_error_handlingabove async methods.
Respect thread safety constraints: Do NOT share node instances (NodeCompute, NodeEffect), cache, or circuit breaker instances across threads without synchronization. Use thread-local instances or locks.
Use duck typing with protocols. Do not use isinstance checks with protocol types. Rely on the protocol interface directly.
Use EnumNodeKind for high-level architectural classification (EFFECT, COMPUTE, REDUCER, ORCHESTRATOR) when routing data through the ONEX pipeline. Use EnumNodeType for specific implementation discovery and capability matching.
Use ModelEventEnvelope for inter-service event-driven communication. Process events through the envelope pattern for consistent event handling.
Files:
src/omnibase_core/enums/__init__.pysrc/omnibase_core/models/contracts/subcontracts/model_compute_pipeline_step.pysrc/omnibase_core/infrastructure/node_base.pysrc/omnibase_core/utils/compute_transformations.pysrc/omnibase_core/utils/compute_path_resolver.pysrc/omnibase_core/models/compute/model_compute_pipeline_result.pysrc/omnibase_core/mixins/mixin_compute_execution.pysrc/omnibase_core/utils/compute_executor.py
src/omnibase_core/models/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use Pydantic 2.11+ models for all data validation and serialization. Ensure models are immutable after creation for thread safety.
Files:
src/omnibase_core/models/contracts/subcontracts/model_compute_pipeline_step.pysrc/omnibase_core/models/compute/model_compute_pipeline_result.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Write tests using pytest with async support via pytest-asyncio. Maintain minimum 60% code coverage. Use pytest markers: @pytest.mark.unit, @pytest.mark.integration, @pytest.mark.slow
Add @pytest.mark.timeout() to prevent test hangs. Run tests withpoetry run pytest tests/ --timeout=60for 60-second timeout protection.
Files:
tests/unit/enums/test_compute_enums.pytests/unit/utils/test_compute_pipeline.pytests/integration/test_compute_pipeline_integration.pytests/integration/conftest.pytests/unit/models/transformations/test_transformation_configs.pytests/unit/utils/test_compute_path_resolver.py
src/omnibase_core/mixins/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Mixin classes should be composed into node classes for specific capabilities. Available mixins include: MixinDiscoveryResponder, MixinEventHandler, MixinEventListener, MixinNodeExecutor, MixinNodeLifecycle, MixinRequestResponseIntrospection, MixinWorkflowSupport.
Files:
src/omnibase_core/mixins/mixin_compute_execution.py
🧠 Learnings (6)
📚 Learning: 2025-12-06T23:34:11.788Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T23:34:11.788Z
Learning: Applies to src/omnibase_core/**/*.py : Use EnumNodeKind for high-level architectural classification (EFFECT, COMPUTE, REDUCER, ORCHESTRATOR) when routing data through the ONEX pipeline. Use EnumNodeType for specific implementation discovery and capability matching.
Applied to files:
src/omnibase_core/enums/__init__.py
📚 Learning: 2025-12-06T23:34:11.788Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T23:34:11.788Z
Learning: Applies to src/omnibase_core/nodes/node_compute.py : NodeCompute is for data processing and transformation. Inherit from NodeCompute for COMPUTE nodes that perform calculations, validations, and data mapping.
Applied to files:
src/omnibase_core/enums/__init__.pysrc/omnibase_core/mixins/mixin_compute_execution.pysrc/omnibase_core/utils/compute_executor.py
📚 Learning: 2025-12-06T23:34:11.788Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T23:34:11.788Z
Learning: Applies to src/omnibase_core/**/*.py : Respect thread safety constraints: Do NOT share node instances (NodeCompute, NodeEffect), cache, or circuit breaker instances across threads without synchronization. Use thread-local instances or locks.
Applied to files:
src/omnibase_core/infrastructure/node_base.py
📚 Learning: 2025-12-06T23:34:11.788Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T23:34:11.788Z
Learning: Applies to tests/**/*.py : Write tests using pytest with async support via pytest-asyncio. Maintain minimum 60% code coverage. Use pytest markers: pytest.mark.unit, pytest.mark.integration, pytest.mark.slow
Applied to files:
tests/unit/enums/test_compute_enums.pytests/unit/utils/test_compute_pipeline.py
📚 Learning: 2025-12-06T23:34:11.788Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T23:34:11.788Z
Learning: Applies to tests/**/*.py : Add pytest.mark.timeout() to prevent test hangs. Run tests with `poetry run pytest tests/ --timeout=60` for 60-second timeout protection.
Applied to files:
tests/unit/enums/test_compute_enums.pytests/unit/utils/test_compute_pipeline.py
📚 Learning: 2025-12-06T23:34:11.788Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T23:34:11.788Z
Learning: Applies to src/omnibase_core/mixins/**/*.py : Mixin classes should be composed into node classes for specific capabilities. Available mixins include: MixinDiscoveryResponder, MixinEventHandler, MixinEventListener, MixinNodeExecutor, MixinNodeLifecycle, MixinRequestResponseIntrospection, MixinWorkflowSupport.
Applied to files:
src/omnibase_core/mixins/mixin_compute_execution.py
🧬 Code graph analysis (11)
src/omnibase_core/enums/__init__.py (6)
src/omnibase_core/enums/enum_case_mode.py (1)
EnumCaseMode(10-22)src/omnibase_core/enums/enum_compute_step_type.py (1)
EnumComputeStepType(50-91)src/omnibase_core/enums/enum_regex_flag.py (1)
EnumRegexFlag(10-22)src/omnibase_core/enums/enum_transformation_type.py (1)
EnumTransformationType(11-34)src/omnibase_core/enums/enum_trim_mode.py (1)
EnumTrimMode(10-22)src/omnibase_core/enums/enum_unicode_form.py (1)
EnumUnicodeForm(10-24)
src/omnibase_core/models/contracts/subcontracts/model_compute_pipeline_step.py (4)
src/omnibase_core/enums/enum_compute_step_type.py (1)
EnumComputeStepType(50-91)src/omnibase_core/enums/enum_transformation_type.py (1)
EnumTransformationType(11-34)src/omnibase_core/models/transformations/model_mapping_config.py (1)
ModelMappingConfig(13-37)src/omnibase_core/models/transformations/model_validation_step_config.py (1)
ModelValidationStepConfig(13-30)
src/omnibase_core/infrastructure/node_base.py (2)
src/omnibase_core/mixins/mixin_event_driven_node.py (1)
node_id(124-126)src/omnibase_core/mixins/mixin_node_setup.py (1)
node_id(67-68)
tests/unit/enums/test_compute_enums.py (6)
src/omnibase_core/enums/enum_case_mode.py (1)
EnumCaseMode(10-22)src/omnibase_core/enums/enum_compute_step_type.py (1)
EnumComputeStepType(50-91)src/omnibase_core/enums/enum_regex_flag.py (1)
EnumRegexFlag(10-22)src/omnibase_core/enums/enum_transformation_type.py (1)
EnumTransformationType(11-34)src/omnibase_core/enums/enum_trim_mode.py (1)
EnumTrimMode(10-22)src/omnibase_core/enums/enum_unicode_form.py (1)
EnumUnicodeForm(10-24)
src/omnibase_core/utils/compute_transformations.py (10)
src/omnibase_core/enums/enum_case_mode.py (1)
EnumCaseMode(10-22)src/omnibase_core/enums/enum_regex_flag.py (1)
EnumRegexFlag(10-22)src/omnibase_core/enums/enum_transformation_type.py (1)
EnumTransformationType(11-34)src/omnibase_core/enums/enum_trim_mode.py (1)
EnumTrimMode(10-22)src/omnibase_core/enums/enum_unicode_form.py (1)
EnumUnicodeForm(10-24)src/omnibase_core/models/transformations/model_transform_case_config.py (1)
ModelTransformCaseConfig(15-27)src/omnibase_core/models/transformations/model_transform_json_path_config.py (1)
ModelTransformJsonPathConfig(13-35)src/omnibase_core/models/transformations/model_transform_regex_config.py (1)
ModelTransformRegexConfig(15-31)src/omnibase_core/models/transformations/model_transform_trim_config.py (1)
ModelTransformTrimConfig(15-27)src/omnibase_core/models/transformations/model_transform_unicode_config.py (1)
ModelTransformUnicodeConfig(15-27)
src/omnibase_core/utils/compute_path_resolver.py (1)
src/omnibase_core/enums/enum_core_error_code.py (1)
EnumCoreErrorCode(9-154)
src/omnibase_core/models/compute/model_compute_pipeline_result.py (1)
src/omnibase_core/models/compute/model_compute_step_result.py (1)
ModelComputeStepResult(64-122)
tests/integration/test_compute_pipeline_integration.py (9)
src/omnibase_core/enums/enum_case_mode.py (1)
EnumCaseMode(10-22)src/omnibase_core/enums/enum_regex_flag.py (1)
EnumRegexFlag(10-22)src/omnibase_core/enums/enum_transformation_type.py (1)
EnumTransformationType(11-34)src/omnibase_core/enums/enum_trim_mode.py (1)
EnumTrimMode(10-22)src/omnibase_core/enums/enum_unicode_form.py (1)
EnumUnicodeForm(10-24)src/omnibase_core/models/compute/model_compute_execution_context.py (1)
ModelComputeExecutionContext(44-95)src/omnibase_core/models/contracts/subcontracts/model_compute_pipeline_step.py (1)
ModelComputePipelineStep(81-202)src/omnibase_core/models/transformations/model_mapping_config.py (1)
ModelMappingConfig(13-37)src/omnibase_core/utils/compute_executor.py (1)
execute_compute_pipeline(442-660)
tests/integration/conftest.py (1)
src/omnibase_core/models/compute/model_compute_execution_context.py (1)
ModelComputeExecutionContext(44-95)
tests/unit/models/transformations/test_transformation_configs.py (11)
src/omnibase_core/enums/enum_case_mode.py (1)
EnumCaseMode(10-22)src/omnibase_core/enums/enum_regex_flag.py (1)
EnumRegexFlag(10-22)src/omnibase_core/enums/enum_trim_mode.py (1)
EnumTrimMode(10-22)src/omnibase_core/enums/enum_unicode_form.py (1)
EnumUnicodeForm(10-24)src/omnibase_core/models/transformations/model_mapping_config.py (1)
ModelMappingConfig(13-37)src/omnibase_core/models/transformations/model_transform_case_config.py (1)
ModelTransformCaseConfig(15-27)src/omnibase_core/models/transformations/model_transform_json_path_config.py (1)
ModelTransformJsonPathConfig(13-35)src/omnibase_core/models/transformations/model_transform_regex_config.py (1)
ModelTransformRegexConfig(15-31)src/omnibase_core/models/transformations/model_transform_trim_config.py (1)
ModelTransformTrimConfig(15-27)src/omnibase_core/models/transformations/model_transform_unicode_config.py (1)
ModelTransformUnicodeConfig(15-27)src/omnibase_core/models/transformations/model_validation_step_config.py (1)
ModelValidationStepConfig(13-30)
tests/unit/utils/test_compute_path_resolver.py (2)
src/omnibase_core/enums/enum_core_error_code.py (1)
EnumCoreErrorCode(9-154)src/omnibase_core/utils/compute_path_resolver.py (5)
PathResolutionError(98-138)resolve_input_path(332-384)resolve_path(272-329)resolve_pipeline_path(487-551)resolve_step_path(387-484)
🪛 LanguageTool
docs/architecture/NODECOMPUTE_VERSIONING_ROADMAP.md
[style] ~55-~55: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...itions. Boring code is debuggable code. Boring APIs are learnable APIs. Stable: O...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🪛 markdownlint-cli2 (0.18.1)
docs/architecture/NODECOMPUTE_VERSIONING_ROADMAP.md
106-106: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
132-132: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
728-728: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
930-930: 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: claude-review
- GitHub Check: Smoke Tests
🔇 Additional comments (5)
tests/integration/conftest.py (1)
17-60: Integration context factory is well-typed and aligned with ExecutionContextThe
ComputeContextFactoryalias andcompute_execution_context_factoryfixture cleanly encapsulate creation of freshModelComputeExecutionContextinstances with unique IDs, and the typing will play nicely with mypy --strict. No changes needed.tests/unit/enums/test_compute_enums.py (1)
13-140: Enum tests are precise and aligned with the v1.0 specThese tests nail the expected string values, member counts, and string-serialization guarantees for the compute enums, and they follow the pytest marker/timeout guidelines. No changes needed.
src/omnibase_core/enums/__init__.py (1)
23-37: Public exposure of NodeCompute enums viaenums.__init__looks goodImporting and exporting the six new compute enums here (case mode, step type, regex flag, transformation type, trim mode, unicode form) fits the existing domain-grouped structure and makes the v1.0 NodeCompute surface available via the central enums package. This is a clean, backward-compatible addition.
Also applies to: 158-159, 194-196, 203-208, 339-345
tests/unit/utils/test_compute_path_resolver.py (1)
31-549: Unit coverage for path resolvers looks solid and aligned with the shared utility.The tests exercise root and nested paths, private-attribute blocking, invalid prefixes, error-code semantics, and edge cases like consecutive dots and very long paths. They closely match the documented behavior of
resolve_path,resolve_input_path,resolve_step_path,resolve_pipeline_path, andPathResolutionError.No issues from this module on my side.
src/omnibase_core/models/compute/model_compute_pipeline_result.py (1)
53-122: Pipeline result model is consistent and well-aligned with executor semantics.The fields, immutability (
frozen=True), and failure semantics (outputNone, error_* populated, step_results up to failed step) line up withexecute_compute_pipelineand the integration tests. Forward-ref wiring toModelComputeStepResultviamodel_rebuild()is also correct.No changes requested here.
…MN-465] Documentation: - Update path-resolution example to match actual implementation - Add language specifiers to markdown code blocks (MD040) - Fix YAML example to use lowercase enum values Code quality: - Replace truthiness check with explicit `is not None` for Pydantic models - Fix node_id=None serialization to show `<unset>` instead of literal "None" - Add ge=0 constraints to duration_ms and count fields across models - Fix docstring examples to match actual function behavior Tests: - Improve test_path_with_consecutive_dots to test edge case properly
PR Review: Contract-Driven NodeCompute v1.0 [OMN-465]This is an exceptionally well-designed and implemented feature. The contract-driven compute pipeline system is a solid foundation that follows ONEX architecture principles closely. Below is my detailed review. 🎯 Overall AssessmentVERDICT: APPROVE with minor suggestions This PR demonstrates excellent software engineering:
✅ Strengths1. Architecture & DesignExcellent adherence to ONEX principles:
Smart scoping decisions:
2. Code QualityType Safety:
Error Handling:
Documentation:
3. TestingImpressive test coverage:
Good test practices:
4. Path Expression DesignSmart minimalism for v1.0:
🔍 Issues & Concerns
|
Code Review: Contract-Driven NodeCompute v1.0 [OMN-465]This is an excellent implementation of a foundational feature. The code quality, documentation, and test coverage are exemplary. ✅ Strengths1. Outstanding Documentation ⭐
2. Strong Architecture Principles
3. Excellent Type Safety
4. Comprehensive Test Coverage (95 tests)
🔍 Issues Requiring Attention1. Global Mutable State (Thread Safety)
|
…ion [OMN-465] Replace ModelOnexError with ValueError in EnumNodeType.get_node_kind() to comply with the architectural rule that enums must not import models (prevents circular dependencies).
…g fixes [OMN-465] - Add language specifiers to fenced code blocks (MD040 compliance) - docs/INDEX.md: text for directory structure - docs/testing/TESTMON_USAGE.md: bash for commands - docs/patterns/EVENT_DRIVEN_ARCHITECTURE.md: python/text - docs/quality/TERMINOLOGY_AUDIT_REPORT.md: python - docs/testing/TEST_COUNT_CLARIFICATION_SUMMARY.md: bash - docs/quality/DOCUMENTATION_VALIDATION_REPORT.md: bash - Fix JSON_PATH documentation in compute_transformations.py - Clarify that path must start with "$" per ModelTransformJsonPathConfig validator - Update examples to show correct usage - Enhance test_path_with_consecutive_dots test - Add test cases for leading and mixed consecutive dots - Improve coverage of path resolution edge cases
- Apply ruff formatting to compute_transformations.py (parenthesized imports) - Fix test_unknown_node_type_raises_error to expect ValueError instead of ModelOnexError (aligning with commit 8814947 which changed to ValueError to avoid enum->model architectural violation)
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 (3)
src/omnibase_core/enums/enum_node_type.py (1)
204-225: Docstring inconsistency: mentionsModelOnexErrorbut code raisesValueError.The docstring at line 205 states "Raises: ModelOnexError" but the implementation now correctly raises
ValueError(per the comment at lines 220-221 explaining the architectural constraint). Update the docstring to match the implementation.Returns: The architectural kind (COMPUTE, EFFECT, REDUCER, ORCHESTRATOR, or RUNTIME_HOST) Raises: - ModelOnexError: If the node type has no kind mapping (e.g., UNKNOWN) + ValueError: If the node type has no kind mapping (e.g., UNKNOWN) Example:src/omnibase_core/mixins/mixin_introspection_publisher.py (1)
338-346:_detect_health_endpointshould return a generic endpoint when_node_idis unsetWith the current implementation:
if hasattr(self, "health_check"): node_id = getattr(self, "_node_id", None) or "<unset>" return f"/health/{node_id}"the method returns
/health/<unset>when the node lacks an ID. While"<unset>"is a recognized sentinel throughout the codebase, including it in a discovery endpoint path is inconsistent with how related discovery metadata is structured (e.g., test fixtures andmodel_hub_registration_event.pyuse generic/health).Prefer returning a generic endpoint when the ID is not set:
if hasattr(self, "health_check"): node_id_raw = getattr(self, "_node_id", None) return ( f"/health/{node_id_raw}" if node_id_raw else "/health" )This aligns health endpoint discovery with the pattern used elsewhere in the codebase and ensures the advertised endpoint is actionable without requiring consumers to handle the
"<unset>"sentinel in URLs.src/omnibase_core/mixins/mixin_event_handler.py (1)
112-128: Wrap UUID conversions fornode_idin all logging contexts to prevent hard failuresThe logging setup across
_setup_event_handlers,_handle_introspection_request,_handle_node_discovery_request, andcleanup_event_handlersconvertsnode_idto UUID without guarding against invalid strings:node_id = getattr(self, "_node_id", None) or "<unset>" node_id=( UUID(node_id) if isinstance(node_id, str) and node_id != "<unset>" else None )Since
_node_idcan be a string (returned by_load_node_id()), callingUUID(node_id)on a non-UUID string raisesValueError, turning safe logging operations into crashes. This affects lines 112–128, 209–221, 229–245, 286–299, 307–323, 390–406, and 408–424.Use a try/except guard or the established
uuid_from_string()helper (available inutil_uuid_utilities):node_id_raw = getattr(self, "_node_id", None) node_id = node_id_raw if node_id_raw is not None else "<unset>" log_node_id: UUID | None = None if isinstance(node_id_raw, UUID): log_node_id = node_id_raw elif isinstance(node_id, str) and node_id != "<unset>": try: log_node_id = UUID(node_id) except ValueError: log_node_id = NoneThen use
log_node_idin theModelLogContext. Apply this pattern to all affected logging sites.
🧹 Nitpick comments (10)
src/omnibase_core/models/discovery/model_node_health_event.py (1)
68-72: LGTM! Consider adding constraints to other metric fields for consistency.The non-negativity constraint on
health_check_duration_msis correct and improves data validation. However, several other metric fields would benefit from similar constraints:
cpu_usage_percent,memory_usage_percent,error_rate: Should havege=0, le=100per their descriptionsuptime_seconds,request_count: Should havege=0as counts/durations cannot be negativeApply this diff to add consistent constraints:
cpu_usage_percent: float | None = Field( default=None, + ge=0, + le=100, description="CPU usage percentage (0-100)", ) memory_usage_percent: float | None = Field( default=None, + ge=0, + le=100, description="Memory usage percentage (0-100)", ) response_time_ms: float | None = Field( default=None, + ge=0, description="Average response time in milliseconds", ) # Operational metrics uptime_seconds: int | None = Field( - default=None, description="Node uptime in seconds" + default=None, + ge=0, + description="Node uptime in seconds" ) error_rate: float | None = Field( default=None, + ge=0, + le=100, description="Error rate percentage (0-100)", ) request_count: int | None = Field( - default=None, description="Total requests processed" + default=None, + ge=0, + description="Total requests processed" )docs/testing/TESTMON_USAGE.md (1)
49-52: Minor improvement: align coverage syntax with Quick Start example.Line 51's coverage command is valid but minimal. For clarity and consistency, consider matching the more complete format from line 20.
Apply this diff to improve clarity:
-poetry run pytest --testmon --cov --cov-fail-under=60 +poetry run pytest --testmon --cov=src/omnibase_core --cov-report=term-missing --cov-fail-under=60This makes the scope and reporting explicit, consistent with the Quick Start example.
src/omnibase_core/models/cli/model_trace_data.py (1)
34-34: LGTM! Field-level validation is a best practice.The
ge=0constraint appropriately enforces non-negativity at model construction and assignment time, aligning with Pydantic validation best practices for duration fields.Optional: Consider the redundancy with the runtime check.
Lines 94-99 perform the same non-negativity check in
validate_instance(). Withvalidate_assignment=Trueand thege=0field constraint, the runtime check is now redundant—Pydantic will reject negative values beforevalidate_instance()is ever called. You may:
- Remove lines 94-99 (preferred if you trust Pydantic validation), or
- Document why both checks exist (defense-in-depth, external data scenarios, etc.)
src/omnibase_core/models/operations/model_execution_metadata.py (1)
81-86: Non-negative constraints for error/warning counts; consider deduping validationThe
ge=0constraints onerror_countandwarning_countcorrectly enforce non-negative counts at the model level and are consistent with how counts are derived elsewhere (e.g., vialen(...)inModelValidationResult). The additional< 0checks invalidate_instanceare now redundant but harmless.If you want to simplify, you could safely drop the explicit negative-count check in
validate_instanceand rely on the Pydantic constraints, or keep both if you prefer the explicitModelOnexErrorsemantics.Also applies to: 192-196
tests/unit/utils/test_compute_path_resolver.py (1)
517-522: Rename test to match actual behavior or add actual unicode key tests.The test
test_unicode_keysactually tests ASCII keys with underscores (user_name,full_name), not keys containing unicode characters. Consider either:
- Renaming to
test_keys_with_underscores, or- Adding actual unicode key tests (e.g.,
{"用户": "test", "名前": "User"})def test_unicode_keys(self) -> None: - """Test paths with unicode keys.""" - data = {"user_name": "test", "full_name": "Test User"} - assert resolve_path("user_name", data) == "test" - assert resolve_path("full_name", data) == "Test User" + """Test paths with actual unicode keys.""" + data = {"用户": "test", "名前": "Test User", "émoji_🎉": "party"} + assert resolve_path("用户", data) == "test" + assert resolve_path("名前", data) == "Test User" + assert resolve_path("émoji_🎉", data) == "party"src/omnibase_core/utils/compute_transformations.py (3)
42-51: Consider moving imports above type aliases.Type aliases
JsonValue,JsonType, andTransformationHandlerare defined before imports (lines 44-50), which is non-standard. While this works since these aliases don't depend on the imports below,isortand PEP 8 convention place all imports at the top.Additionally,
JsonValueandJsonTypedon't appear to be used within this module—they're only exported in__all__. If they exist solely for downstream consumers, consider documenting this or moving them to a shared types module.
201-210: Consider using a mapping dict for flag conversion.The if-elif chain for flag conversion works but a mapping would be more maintainable:
+_REGEX_FLAG_MAP: dict[EnumRegexFlag, re.RegexFlag] = { + EnumRegexFlag.IGNORECASE: re.IGNORECASE, + EnumRegexFlag.MULTILINE: re.MULTILINE, + EnumRegexFlag.DOTALL: re.DOTALL, +} + def transform_regex(data: str, config: ModelTransformRegexConfig) -> str: ... _validate_string_input(data, "REGEX") - # Convert EnumRegexFlag to Python re flags - flags = 0 - for flag in config.flags: - if flag == EnumRegexFlag.IGNORECASE: - flags |= re.IGNORECASE - elif flag == EnumRegexFlag.MULTILINE: - flags |= re.MULTILINE - elif flag == EnumRegexFlag.DOTALL: - flags |= re.DOTALL + flags = 0 + for flag in config.flags: + flags |= _REGEX_FLAG_MAP.get(flag, 0)
549-561: Simplify transformation_type value access.Since
transformation_typeis typed asEnumTransformationType, it always has a.valueattribute. Thehasattrcheck is unnecessary:raise ModelOnexError( error_code=EnumCoreErrorCode.VALIDATION_ERROR, message=f"Configuration required for {transformation_type} transformation", context={ - "transform_type": ( - transformation_type.value - if hasattr(transformation_type, "value") - else str(transformation_type) - ), + "transform_type": transformation_type.value, "input_type": type(data).__name__, }, )src/omnibase_core/mixins/mixin_introspection_publisher.py (2)
143-156: Fallback introspection logging withnode_idsentinel is fine; consider centralizing the sentinelThe updated fallback now logs both
node_id_raw(in the context when it’s aUUID) and a human-readablenode_idstring with the "" sentinel. That’s consistent with the broader sentinel approach and keeps logging safe.Given the same
"<unset>"literal appears in multiple mixins, you might consider a shared constant (e.g.,NODE_ID_UNSET_SENTINEL) in a small utilities module to avoid divergence across files, but this is purely a maintainability nit.
353-369: Clarifysource_node_idsemantics and remove unusedsource_node_id_strin_publish_with_retryIn
_publish_with_retryyou now do:node_id_raw = getattr(self, "_node_id", None) ... if isinstance(node_id_raw, UUID): source_node_id_str = str(node_id_raw) elif isinstance(node_id_raw, str): source_node_id_str = node_id_raw else: source_node_id_str = "<unset>" # Ensure source_node_id is UUID if isinstance(node_id_raw, UUID): source_uuid = node_id_raw else: source_uuid = uuid4() envelope = ModelEventEnvelope.create_broadcast( payload=event, source_node_id=source_uuid, correlation_id=event.correlation_id, )Observations:
source_node_id_stris currently unused, so it’s just dead code and may trigger lint warnings.- When
_node_idis a string (even a stable one),source_node_idbecomes a freshuuid4()on every call, so events from the same logical node cannot be correlated bysource_node_idunless_node_idis already aUUID.If the intention is that only true UUID-valued
_node_ids participate in correlation, the behavior is fine; in that case, you can dropsource_node_id_strentirely. If instead you want a stablesource_node_ideven for string IDs, consider deriving a UUID from the string (e.g., via the same helper used in your discovery models) rather thanuuid4().For example, minimal cleanup if you’re happy with the current semantics:
- source_node_id_str: str - if isinstance(node_id_raw, UUID): - source_node_id_str = str(node_id_raw) - elif isinstance(node_id_raw, str): - source_node_id_str = node_id_raw - else: - source_node_id_str = "<unset>" - # Ensure source_node_id is UUID - if isinstance(node_id_raw, UUID): - source_uuid = node_id_raw - else: - source_uuid = uuid4() + if isinstance(node_id_raw, UUID): + source_uuid = node_id_raw + else: + source_uuid = uuid4()Or, if you want deterministic IDs from strings, replace the
uuid4()branch with your existing string→UUID helper.Also applies to: 381-387
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
docs/INDEX.md(1 hunks)docs/architecture/CONTRACT_DRIVEN_NODECOMPUTE_V1_0.md(1 hunks)docs/architecture/NODECOMPUTE_VERSIONING_ROADMAP.md(1 hunks)docs/patterns/EVENT_DRIVEN_ARCHITECTURE.md(11 hunks)docs/quality/DOCUMENTATION_VALIDATION_REPORT.md(2 hunks)docs/quality/TERMINOLOGY_AUDIT_REPORT.md(2 hunks)docs/testing/TESTMON_USAGE.md(4 hunks)docs/testing/TEST_COUNT_CLARIFICATION_SUMMARY.md(2 hunks)src/omnibase_core/enums/enum_node_type.py(1 hunks)src/omnibase_core/mixins/mixin_compute_execution.py(1 hunks)src/omnibase_core/mixins/mixin_event_handler.py(7 hunks)src/omnibase_core/mixins/mixin_introspection_publisher.py(9 hunks)src/omnibase_core/models/cli/model_result_summary.py(1 hunks)src/omnibase_core/models/cli/model_trace_data.py(1 hunks)src/omnibase_core/models/contracts/subcontracts/model_compute_pipeline_step.py(1 hunks)src/omnibase_core/models/discovery/model_node_health_event.py(1 hunks)src/omnibase_core/models/events/model_intent_execution_result.py(1 hunks)src/omnibase_core/models/operations/model_execution_metadata.py(2 hunks)src/omnibase_core/utils/compute_transformations.py(1 hunks)tests/unit/utils/test_compute_path_resolver.py(1 hunks)
✅ Files skipped from review due to trivial changes (4)
- docs/INDEX.md
- docs/quality/DOCUMENTATION_VALIDATION_REPORT.md
- docs/testing/TEST_COUNT_CLARIFICATION_SUMMARY.md
- docs/architecture/CONTRACT_DRIVEN_NODECOMPUTE_V1_0.md
🚧 Files skipped from review as they are similar to previous changes (1)
- src/omnibase_core/mixins/mixin_compute_execution.py
🧰 Additional context used
📓 Path-based instructions (5)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Always use Poetry for Python package management and task execution. Usepoetry runfor pytest, mypy, black, isort, and other tools. Never use pip directly or execute python commands without the poetry run prefix.
Enforce 100% strict mypy compliance. All functions must have complete type annotations. Usepoetry run mypy src/omnibase_core/to verify strict mode compliance with no errors.
Format code with black, sort imports with isort, and lint with ruff. Runpoetry run black src/ tests/,poetry run isort src/ tests/, andpoetry run ruff check --fix src/ tests/
Files:
src/omnibase_core/models/cli/model_trace_data.pysrc/omnibase_core/models/operations/model_execution_metadata.pysrc/omnibase_core/models/contracts/subcontracts/model_compute_pipeline_step.pysrc/omnibase_core/enums/enum_node_type.pysrc/omnibase_core/mixins/mixin_introspection_publisher.pysrc/omnibase_core/mixins/mixin_event_handler.pysrc/omnibase_core/models/discovery/model_node_health_event.pysrc/omnibase_core/models/cli/model_result_summary.pysrc/omnibase_core/models/events/model_intent_execution_result.pysrc/omnibase_core/utils/compute_transformations.pytests/unit/utils/test_compute_path_resolver.py
src/omnibase_core/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/omnibase_core/**/*.py: Use protocol names withcontainer.get_service()for dependency injection, never resolve services by concrete class names. Example:container.get_service('ProtocolEventBus')instead ofcontainer.get_service('EventBusService')
Always use ModelONEXContainer for dependency injection in node constructors and service resolution. Never confuse it with ModelContainer[T], which is a generic value wrapper, not a DI container.
Use structured error handling with ModelOnexError and EnumCoreErrorCode, never raise generic Exception. Always chain exceptions withfrom ewhen catching and re-raising.
Use the @standard_error_handling decorator on methods to eliminate 6+ lines of try/catch boilerplate. Example:@standard_error_handlingabove async methods.
Respect thread safety constraints: Do NOT share node instances (NodeCompute, NodeEffect), cache, or circuit breaker instances across threads without synchronization. Use thread-local instances or locks.
Use duck typing with protocols. Do not use isinstance checks with protocol types. Rely on the protocol interface directly.
Use EnumNodeKind for high-level architectural classification (EFFECT, COMPUTE, REDUCER, ORCHESTRATOR) when routing data through the ONEX pipeline. Use EnumNodeType for specific implementation discovery and capability matching.
Use ModelEventEnvelope for inter-service event-driven communication. Process events through the envelope pattern for consistent event handling.
Files:
src/omnibase_core/models/cli/model_trace_data.pysrc/omnibase_core/models/operations/model_execution_metadata.pysrc/omnibase_core/models/contracts/subcontracts/model_compute_pipeline_step.pysrc/omnibase_core/enums/enum_node_type.pysrc/omnibase_core/mixins/mixin_introspection_publisher.pysrc/omnibase_core/mixins/mixin_event_handler.pysrc/omnibase_core/models/discovery/model_node_health_event.pysrc/omnibase_core/models/cli/model_result_summary.pysrc/omnibase_core/models/events/model_intent_execution_result.pysrc/omnibase_core/utils/compute_transformations.py
src/omnibase_core/models/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use Pydantic 2.11+ models for all data validation and serialization. Ensure models are immutable after creation for thread safety.
Files:
src/omnibase_core/models/cli/model_trace_data.pysrc/omnibase_core/models/operations/model_execution_metadata.pysrc/omnibase_core/models/contracts/subcontracts/model_compute_pipeline_step.pysrc/omnibase_core/models/discovery/model_node_health_event.pysrc/omnibase_core/models/cli/model_result_summary.pysrc/omnibase_core/models/events/model_intent_execution_result.py
src/omnibase_core/mixins/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Mixin classes should be composed into node classes for specific capabilities. Available mixins include: MixinDiscoveryResponder, MixinEventHandler, MixinEventListener, MixinNodeExecutor, MixinNodeLifecycle, MixinRequestResponseIntrospection, MixinWorkflowSupport.
Files:
src/omnibase_core/mixins/mixin_introspection_publisher.pysrc/omnibase_core/mixins/mixin_event_handler.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Write tests using pytest with async support via pytest-asyncio. Maintain minimum 60% code coverage. Use pytest markers: @pytest.mark.unit, @pytest.mark.integration, @pytest.mark.slow
Add @pytest.mark.timeout() to prevent test hangs. Run tests withpoetry run pytest tests/ --timeout=60for 60-second timeout protection.
Files:
tests/unit/utils/test_compute_path_resolver.py
🧠 Learnings (21)
📓 Common learnings
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T23:34:11.788Z
Learning: Applies to src/omnibase_core/nodes/node_compute.py : NodeCompute is for data processing and transformation. Inherit from NodeCompute for COMPUTE nodes that perform calculations, validations, and data mapping.
📚 Learning: 2025-12-06T23:34:11.788Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T23:34:11.788Z
Learning: Applies to src/omnibase_core/**/*.py : Always use ModelONEXContainer for dependency injection in node constructors and service resolution. Never confuse it with ModelContainer[T], which is a generic value wrapper, not a DI container.
Applied to files:
docs/quality/TERMINOLOGY_AUDIT_REPORT.md
📚 Learning: 2025-12-06T23:34:11.788Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T23:34:11.788Z
Learning: Applies to src/omnibase_core/nodes/**/*.py : Node classes must always call `super().__init__(container)` in their constructor with a ModelONEXContainer parameter. This eliminates 80+ lines of boilerplate initialization code.
Applied to files:
docs/quality/TERMINOLOGY_AUDIT_REPORT.md
📚 Learning: 2025-12-06T23:34:11.788Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T23:34:11.788Z
Learning: Applies to src/omnibase_core/**/*.py : Use protocol names with `container.get_service()` for dependency injection, never resolve services by concrete class names. Example: `container.get_service('ProtocolEventBus')` instead of `container.get_service('EventBusService')`
Applied to files:
docs/quality/TERMINOLOGY_AUDIT_REPORT.md
📚 Learning: 2025-12-06T23:34:11.788Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T23:34:11.788Z
Learning: Protocol-Driven Dependency Injection should use container-based service resolution with protocol interfaces, not concrete class registry dependencies. This ensures loose coupling and testability.
Applied to files:
docs/quality/TERMINOLOGY_AUDIT_REPORT.md
📚 Learning: 2025-12-06T23:34:11.788Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T23:34:11.788Z
Learning: Applies to src/omnibase_core/**/*.py : Use EnumNodeKind for high-level architectural classification (EFFECT, COMPUTE, REDUCER, ORCHESTRATOR) when routing data through the ONEX pipeline. Use EnumNodeType for specific implementation discovery and capability matching.
Applied to files:
src/omnibase_core/enums/enum_node_type.pydocs/patterns/EVENT_DRIVEN_ARCHITECTURE.md
📚 Learning: 2025-12-06T23:34:11.788Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T23:34:11.788Z
Learning: Applies to src/omnibase_core/**/*.py : Use structured error handling with ModelOnexError and EnumCoreErrorCode, never raise generic Exception. Always chain exceptions with `from e` when catching and re-raising.
Applied to files:
src/omnibase_core/enums/enum_node_type.pydocs/patterns/EVENT_DRIVEN_ARCHITECTURE.md
📚 Learning: 2025-12-06T23:34:11.788Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T23:34:11.788Z
Learning: Applies to src/omnibase_core/mixins/**/*.py : Mixin classes should be composed into node classes for specific capabilities. Available mixins include: MixinDiscoveryResponder, MixinEventHandler, MixinEventListener, MixinNodeExecutor, MixinNodeLifecycle, MixinRequestResponseIntrospection, MixinWorkflowSupport.
Applied to files:
src/omnibase_core/mixins/mixin_introspection_publisher.pysrc/omnibase_core/mixins/mixin_event_handler.py
📚 Learning: 2025-12-06T23:34:11.788Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T23:34:11.788Z
Learning: Applies to src/omnibase_core/**/*.py : Use ModelEventEnvelope for inter-service event-driven communication. Process events through the envelope pattern for consistent event handling.
Applied to files:
src/omnibase_core/mixins/mixin_introspection_publisher.pydocs/patterns/EVENT_DRIVEN_ARCHITECTURE.md
📚 Learning: 2025-12-06T23:34:11.788Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T23:34:11.788Z
Learning: Applies to src/omnibase_core/models/**/*.py : Use Pydantic 2.11+ models for all data validation and serialization. Ensure models are immutable after creation for thread safety.
Applied to files:
docs/patterns/EVENT_DRIVEN_ARCHITECTURE.md
📚 Learning: 2025-12-06T23:34:11.788Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T23:34:11.788Z
Learning: Applies to src/omnibase_core/nodes/node_reducer.py : NodeReducer is for FSM-driven state aggregation and management. Use ModelIntent for state transitions. This is the PRIMARY implementation as of v0.4.0 (replaces NodeReducerDeclarative).
Applied to files:
docs/patterns/EVENT_DRIVEN_ARCHITECTURE.md
📚 Learning: 2025-12-06T23:34:11.788Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T23:34:11.788Z
Learning: Respect the ONEX Four-Node Architecture: data flows unidirectionally from EFFECT → COMPUTE → REDUCER → ORCHESTRATOR with no backwards dependencies. This is an architectural principle, not tied to specific files.
Applied to files:
docs/patterns/EVENT_DRIVEN_ARCHITECTURE.md
📚 Learning: 2025-12-06T23:34:11.788Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T23:34:11.788Z
Learning: Applies to src/omnibase_core/nodes/node_compute.py : NodeCompute is for data processing and transformation. Inherit from NodeCompute for COMPUTE nodes that perform calculations, validations, and data mapping.
Applied to files:
docs/patterns/EVENT_DRIVEN_ARCHITECTURE.mdsrc/omnibase_core/utils/compute_transformations.py
📚 Learning: 2025-12-06T23:34:11.788Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T23:34:11.788Z
Learning: Applies to src/omnibase_core/nodes/node_effect.py : NodeEffect is for external interactions (I/O). Inherit from NodeEffect for EFFECT nodes that handle API calls, database operations, file system, or message queues.
Applied to files:
docs/patterns/EVENT_DRIVEN_ARCHITECTURE.md
📚 Learning: 2025-12-06T23:34:11.788Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T23:34:11.788Z
Learning: Always verify setup with: `poetry run pytest tests/ -x`, `poetry run mypy src/omnibase_core/`, and `pre-commit run --all-files` before development cycles.
Applied to files:
docs/testing/TESTMON_USAGE.md
📚 Learning: 2025-12-06T23:34:11.788Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T23:34:11.788Z
Learning: Applies to **/*.py : Always use Poetry for Python package management and task execution. Use `poetry run` for pytest, mypy, black, isort, and other tools. Never use pip directly or execute python commands without the poetry run prefix.
Applied to files:
docs/testing/TESTMON_USAGE.md
📚 Learning: 2025-12-06T23:34:11.788Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T23:34:11.788Z
Learning: Applies to **/*.py : Format code with black, sort imports with isort, and lint with ruff. Run `poetry run black src/ tests/`, `poetry run isort src/ tests/`, and `poetry run ruff check --fix src/ tests/`
Applied to files:
docs/testing/TESTMON_USAGE.md
📚 Learning: 2025-12-06T23:34:11.788Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T23:34:11.788Z
Learning: Applies to tests/**/*.py : Add pytest.mark.timeout() to prevent test hangs. Run tests with `poetry run pytest tests/ --timeout=60` for 60-second timeout protection.
Applied to files:
docs/testing/TESTMON_USAGE.md
📚 Learning: 2025-12-06T23:34:11.788Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T23:34:11.788Z
Learning: Applies to tests/**/*.py : Write tests using pytest with async support via pytest-asyncio. Maintain minimum 60% code coverage. Use pytest markers: pytest.mark.unit, pytest.mark.integration, pytest.mark.slow
Applied to files:
docs/testing/TESTMON_USAGE.md
📚 Learning: 2025-12-06T23:34:11.788Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T23:34:11.788Z
Learning: Applies to .pre-commit-config.yaml : Pre-commit hooks must include: mypy with strict mode, black formatting, isort import sorting, and ruff linting. Run `pre-commit install` during setup and `pre-commit run --all-files` before commits.
Applied to files:
docs/testing/TESTMON_USAGE.md
📚 Learning: 2025-12-06T23:34:11.788Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T23:34:11.788Z
Learning: Applies to **/*.py : Enforce 100% strict mypy compliance. All functions must have complete type annotations. Use `poetry run mypy src/omnibase_core/` to verify strict mode compliance with no errors.
Applied to files:
docs/testing/TESTMON_USAGE.md
🧬 Code graph analysis (6)
src/omnibase_core/models/operations/model_execution_metadata.py (1)
src/omnibase_core/models/common/model_validation_result.py (2)
error_count(94-96)warning_count(99-101)
src/omnibase_core/models/contracts/subcontracts/model_compute_pipeline_step.py (4)
src/omnibase_core/enums/enum_compute_step_type.py (1)
EnumComputeStepType(50-91)src/omnibase_core/enums/enum_transformation_type.py (1)
EnumTransformationType(11-34)src/omnibase_core/models/transformations/model_mapping_config.py (1)
ModelMappingConfig(13-37)src/omnibase_core/models/transformations/model_validation_step_config.py (1)
ModelValidationStepConfig(13-30)
src/omnibase_core/mixins/mixin_introspection_publisher.py (1)
src/omnibase_core/models/discovery/model_nodeintrospectionevent.py (2)
ModelNodeIntrospectionEvent(13-176)create_from_node_info(123-176)
src/omnibase_core/models/cli/model_result_summary.py (2)
src/omnibase_core/protocols/event_bus.py (1)
retry_count(176-178)src/omnibase_core/models/common/model_validation_result.py (2)
error_count(94-96)warning_count(99-101)
src/omnibase_core/utils/compute_transformations.py (11)
src/omnibase_core/enums/enum_case_mode.py (1)
EnumCaseMode(10-22)src/omnibase_core/enums/enum_core_error_code.py (1)
EnumCoreErrorCode(9-154)src/omnibase_core/enums/enum_regex_flag.py (1)
EnumRegexFlag(10-22)src/omnibase_core/enums/enum_transformation_type.py (1)
EnumTransformationType(11-34)src/omnibase_core/enums/enum_trim_mode.py (1)
EnumTrimMode(10-22)src/omnibase_core/enums/enum_unicode_form.py (1)
EnumUnicodeForm(10-24)src/omnibase_core/models/transformations/model_transform_case_config.py (1)
ModelTransformCaseConfig(15-27)src/omnibase_core/models/transformations/model_transform_json_path_config.py (1)
ModelTransformJsonPathConfig(13-35)src/omnibase_core/models/transformations/model_transform_regex_config.py (1)
ModelTransformRegexConfig(15-31)src/omnibase_core/models/transformations/model_transform_trim_config.py (1)
ModelTransformTrimConfig(15-27)src/omnibase_core/models/transformations/model_transform_unicode_config.py (1)
ModelTransformUnicodeConfig(15-27)
tests/unit/utils/test_compute_path_resolver.py (1)
src/omnibase_core/utils/compute_path_resolver.py (5)
PathResolutionError(98-138)resolve_input_path(332-384)resolve_path(272-329)resolve_pipeline_path(487-551)resolve_step_path(387-484)
🪛 LanguageTool
docs/architecture/NODECOMPUTE_VERSIONING_ROADMAP.md
[style] ~55-~55: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...itions. Boring code is debuggable code. Boring APIs are learnable APIs. Stable: O...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
⏰ 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: Smoke Tests
🔇 Additional comments (35)
src/omnibase_core/models/events/model_intent_execution_result.py (1)
51-55: Non-negative constraint onexecution_duration_mslooks correctAdding
ge=0keepsNonevalid while preventing negative durations, which matches the semantics of an execution time in milliseconds and aligns with the other timing fields introduced in this PR. Only payloads that previously sent invalid negative values will now fail validation, which is an appropriate tightening of the contract.docs/testing/TESTMON_USAGE.md (4)
9-21: LGTM! Quick Start section provides clear, accurate examples with proper poetry run prefix and coverage integration.
37-43: LGTM! Local Development examples are clear and correct—reset procedure with --testmon-noselect is the right approach.
56-59: LGTM! Full Suite command is correct and clear.
86-106: LGTM! Troubleshooting section provides clear guidance with correct poetry run examples and proper database reset procedures. The Correct/Wrong comparison at lines 94-98 effectively reinforces the poetry requirement from coding guidelines.src/omnibase_core/models/cli/model_result_summary.py (1)
36-42: LGTM! Non-negativity constraints improve data integrity.The
ge=0constraints on duration and count fields are semantically correct and align with the strict validation patterns introduced across the NodeCompute v1.0 compute pipeline models. Withvalidate_assignment=True, these constraints are enforced both at creation and on subsequent assignments, preventing invalid states.src/omnibase_core/models/operations/model_execution_metadata.py (1)
48-52: Non-negative constraint onduration_msis consistent and safeAdding
ge=0toduration_msaligns with the existing logical validation invalidate_instanceand the broader pattern of non-negative duration fields across the models. No behavioral concerns from this change.docs/patterns/EVENT_DRIVEN_ARCHITECTURE.md (3)
15-26: LGTM! Proper code fence language specification and ModelEventEnvelope usage.The example correctly demonstrates the event envelope pattern with
ModelEventEnvelopefor inter-service communication. Based on learnings, this aligns with the project's event-driven architecture guidelines.
93-110: LGTM! Correct protocol-based service resolution.The example at line 106 correctly uses
container.get_service("ProtocolEventBus")for dependency injection, following the coding guideline to resolve services by protocol names rather than concrete class names.
187-197: LGTM! Good demonstration of structured payload best practice.The example effectively contrasts unstructured payloads with Pydantic model-based structured payloads, reinforcing the guideline to use Pydantic models for data validation.
docs/architecture/NODECOMPUTE_VERSIONING_ROADMAP.md (3)
130-139: LGTM! Timeline appropriately updated to milestone-based scheduling.The timeline now correctly uses milestone-based sequencing rather than specific calendar dates, with the clarifying note that implementation depends on production feedback. This addresses the previous review concern about outdated Q1 2025 references.
766-779: Good clarification on registry lockdown.The documentation clearly states that the transformation registry is internal-only and user-defined transformations are not supported. This is an important design decision that prevents determinism, replay, and security issues.
505-514: This is prospective documentation—ModelComputeCaching is not yet implemented.The documented Pydantic v2 pattern with
model_config = ConfigDict(extra="forbid", frozen=True)is correct and follows best practices for immutability. No discrepancies exist between documentation and implementation because this is a planned feature in the roadmap, not yet implemented in the codebase.src/omnibase_core/models/contracts/subcontracts/model_compute_pipeline_step.py (4)
33-50: LGTM! YAML example now uses correct lowercase enum values.The example correctly shows
transformation_type: "case_conversion"matching theEnumTransformationTypestring values. This addresses the previous review concern about uppercase enum values failing validation.
70-78: LGTM! Clean imports with proper type organization.Imports are well-organized, bringing in the necessary enums and model types. The use of a discriminated union type (
ModelTransformationConfig) from the types module is appropriate for handling multiple transformation config types.
170-171: LGTM! Proper Pydantic v2 configuration.The model correctly uses
ConfigDict(extra="forbid", frozen=True)ensuring immutability and strict field validation, which aligns with the coding guidelines for thread-safe models.
173-202: LGTM! Comprehensive step configuration validation.The
@model_validator(mode="after")correctly enforces:
- TRANSFORMATION steps require
transformation_type- Non-IDENTITY transformations require
transformation_config- IDENTITY transformations must NOT have config (important design decision)
- MAPPING steps require
mapping_config- VALIDATION steps require
validation_configThis validation ensures contracts are well-formed at construction time rather than failing at execution.
src/omnibase_core/enums/enum_node_type.py (1)
220-225: LGTM! Correct architectural fix to avoid enum→model import cycle.The change from
ModelOnexErrortoValueErrorproperly respects the architectural constraint that enums should not import models. The comment clearly documents the rationale, and the error message includes helpful context (the unmapped type and available types).docs/quality/TERMINOLOGY_AUDIT_REPORT.md (2)
171-179: LGTM! Code fence language specification added.Adding the
pythonlanguage identifier to the code block enables proper syntax highlighting, improving documentation readability.
205-209: LGTM! Consistent code fence formatting.The code example correctly demonstrates resolving services by protocol interface (
"ProtocolEventBus"), which aligns with the coding guidelines.tests/unit/utils/test_compute_path_resolver.py (7)
1-29: LGTM!Well-structured module docstring clearly documents test categories. Imports are minimal and appropriate for the test scope.
31-55: LGTM!Test fixtures are well-designed with
MockStepResultfor step output testing,MockObjectwith private attributes for security testing, andNestedObjectfor nested traversal scenarios.
62-172: LGTM!Comprehensive test coverage for
resolve_pathincluding root access, nested traversal, mixed dict/object access, error handling, and private attribute security. Tests properly use@pytest.mark.unitand@pytest.mark.timeout(30)per coding guidelines.
179-240: LGTM!Tests cover both
$.inputand$inputalias forms, nested access, error handling, and private attribute security on input objects while correctly allowing underscore dict keys.
247-332: LGTM!Excellent coverage of step path resolution including shorthand/explicit form equivalence, multiple steps, various output types, and comprehensive error scenarios (missing step, invalid subpath, empty step name).
402-461: LGTM with a note on context access pattern.The tests for
PathResolutionErrorcorrectly verify error creation and context fields. The nested access pattern (error.context.get("additional_context", {}).get("context", {})) appears to match howModelOnexErrorwraps custom context. The subclass relationship test at lines 455-460 ensures proper error hierarchy.
536-567: LGTM!Good stress test for deeply nested paths (50 levels) and thorough coverage of step name variations including underscores and numbers.
src/omnibase_core/utils/compute_transformations.py (7)
73-112: LGTM!Clean DRY helper with proper error handling using
ModelOnexErrorandEnumCoreErrorCode.VALIDATION_ERRORper coding guidelines. Good error context for debugging.
122-167: LGTM!Identity transformation is clean with good documentation explaining the
configparameter's purpose for registry uniformity. Usingdel configto satisfy linters is acceptable.
226-273: LGTM!Case transformation is straightforward with good defensive error handling for unknown modes. The else branch guards against future enum additions.
275-323: LGTM!Trim transformation follows the same clean pattern as case transformation with proper validation and defensive error handling.
325-361: LGTM!Unicode normalization is clean and correctly uses
config.form.valueto get the normalization form string forunicodedata.normalize. Docstring example now correctly shows'café'.
363-475: LGTM!The implementation correctly differentiates between dict keys (allowing underscore keys) and object attributes (blocking private attributes starting with
_). The TODO acknowledges potential deduplication withcompute_path_resolver.py.
568-582: LGTM!Clean and complete
__all__export list covering type aliases, individual transformation functions, the dispatcher, and the registry.src/omnibase_core/mixins/mixin_event_handler.py (1)
343-345:target_node_idmatching normalization looks reasonableNormalizing
_node_idthrough a singlestr(node_id)beforefnmatch.fnmatchkeeps matching behavior consistent whether_node_idis a UUID or a string, while the "" sentinel ensures nodes without an ID won’t accidentally match typical concretetarget_node_idpatterns.
| node_id_raw = getattr(self, "_node_id", None) | ||
| node_id = node_id_raw if node_id_raw is not None else "<unset>" | ||
| try: |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat src/omnibase_core/mixins/mixin_introspection_publisher.pyRepository: OmniNode-ai/omnibase_core
Length of output: 16700
🏁 Script executed:
rg "class ModelNodeIntrospectionEvent" src/ --type py -A 30Repository: OmniNode-ai/omnibase_core
Length of output: 3348
🏁 Script executed:
rg "def create_from_node_info" src/ --type py -A 15Repository: OmniNode-ai/omnibase_core
Length of output: 1623
🏁 Script executed:
rg "uuid_from_string|def.*uuid.*from" src/ --type py -A 5Repository: OmniNode-ai/omnibase_core
Length of output: 50381
Avoid UUID(node_id) on arbitrary strings when publishing introspection events
The code at lines 65-69 attempts to convert node_id to a UUID using UUID(node_id), which will raise ValueError if node_id is a non-empty non-UUID-format string. However, ModelNodeIntrospectionEvent.create_from_node_info() accepts node_id: UUID | str and has a field validator that safely converts strings to UUIDs using uuid_from_string() from the utilities module.
Instead of manually constructing event_node_id with fallible UUID() conversion, pass node_id_raw directly to create_from_node_info() and let the model's validator handle the conversion:
node_id_raw = getattr(self, "_node_id", None)
- node_id = node_id_raw if node_id_raw is not None else "<unset>"
+ node_id = node_id_raw if node_id_raw is not None else uuid4()
try:
introspection_data = self._gather_introspection_data()
correlation_id = uuid4()
if not hasattr(self, "get_node_type"):
raise ModelOnexError(
message=f"Node {self.__class__.__name__} must implement get_node_type() "
"returning one of: effect, compute, reducer, orchestrator",
error_code=EnumCoreErrorCode.VALIDATION_ERROR,
)
node_type = self.get_node_type()
- # For creating the event, we need a valid UUID - generate one if unset
- event_node_id = (
- UUID(node_id)
- if isinstance(node_id, str) and node_id != "<unset>"
- else (node_id_raw if isinstance(node_id_raw, UUID) else uuid4())
- )
introspection_event = ModelNodeIntrospectionEvent.create_from_node_info(
- node_id=event_node_id,
+ node_id=node_id,
node_name=introspection_data.node_name,Also remove the "<unset>" sentinel from the health endpoint URL construction at line 338—use a UUID fallback instead:
def _detect_health_endpoint(self) -> str | None:
"""Detect if this node has a health endpoint."""
try:
if hasattr(self, "health_check"):
- node_id = getattr(self, "_node_id", None) or "<unset>"
+ node_id = getattr(self, "_node_id", None) or uuid4()
return f"/health/{node_id}"
except Exception:
pass
return NoneIn _publish_with_retry() at lines 381–387, the variable source_node_id_str is computed but never used; derive source_uuid directly instead:
source_node_id_str: str
if isinstance(node_id_raw, UUID):
source_node_id_str = str(node_id_raw)
elif isinstance(node_id_raw, str):
source_node_id_str = node_id_raw
else:
source_node_id_str = "<unset>"
# Ensure source_node_id is UUID
if isinstance(node_id_raw, UUID):
source_uuid = node_id_raw
else:
source_uuid = uuid4()Remove the unused source_node_id_str variable and simplify the UUID derivation.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| node_id_raw = getattr(self, "_node_id", None) | |
| node_id = node_id_raw if node_id_raw is not None else "<unset>" | |
| try: | |
| node_id_raw = getattr(self, "_node_id", None) | |
| node_id = node_id_raw if node_id_raw is not None else uuid4() | |
| try: |
🤖 Prompt for AI Agents
In src/omnibase_core/mixins/mixin_introspection_publisher.py around lines 50–52
and the later referenced offsets, replace the manual, fallible UUID(node_id)
conversion by passing node_id_raw (the original attribute value) directly into
ModelNodeIntrospectionEvent.create_from_node_info so the model validator uses
uuid_from_string safely; remove the "<unset>" sentinel when building the health
endpoint URL at the health URL construction site (line ~338) and instead use a
proper UUID fallback (e.g., derive or cast to a UUID) so the URL always contains
a valid UUID; and in _publish_with_retry (lines ~381–387) remove the unused
source_node_id_str variable and compute source_uuid directly from the original
source node id value to simplify logic and avoid unused variables.
…MN-465] Fix test_unknown_type_intentionally_unmapped to expect ValueError instead of ModelOnexError, matching the architectural change in enum_node_type.py that avoids enum->model import violations.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
tests/unit/enums/test_enum_node_kind.py (1)
475-475: Add pytest markers per coding guidelines.The test function is missing the
@pytest.mark.unitmarker. Per the coding guidelines, all test functions should use pytest markers to categorize tests.As per coding guidelines, add the appropriate markers:
+ @pytest.mark.unit + @pytest.mark.timeout(5) def test_unknown_node_type_raises_error(self) -> None:Note: This appears to be a pattern throughout the file—consider adding markers to all test methods in a follow-up.
src/omnibase_core/utils/compute_transformations.py (2)
126-171: Consider enforcing “IDENTITY has no config” invariant insideexecute_transformation.Right now the “no config for IDENTITY” rule is enforced at the
ModelComputePipelineStepcontract level and documented intransform_identity, butexecute_transformationwill silently accept a non-Noneconfig forEnumTransformationType.IDENTITYand ignore it.To make the invariant self-contained at the dispatcher boundary (and closer to the spec that IDENTITY has no config), consider tightening the config checks like this:
def execute_transformation( data: Any, transformation_type: EnumTransformationType, config: ModelTransformationConfig | None, ) -> Any: @@ - # Validate config requirements: - # - IDENTITY: config must be None (no configuration needed) - # - All others: config is required - # Contract-level validation (ModelComputePipelineStep) ensures IDENTITY steps - # never have transformation_config set, so config will be None here. - if transformation_type != EnumTransformationType.IDENTITY and config is None: - raise ModelOnexError( - error_code=EnumCoreErrorCode.VALIDATION_ERROR, - message=f"Configuration required for {transformation_type} transformation", - context={ - "transform_type": ( - transformation_type.value - if hasattr(transformation_type, "value") - else str(transformation_type) - ), - "input_type": type(data).__name__, - }, - ) + # Validate config requirements: + # - IDENTITY: config must be None (no configuration allowed) + # - All others: config is required + if transformation_type == EnumTransformationType.IDENTITY: + if config is not None: + raise ModelOnexError( + error_code=EnumCoreErrorCode.VALIDATION_ERROR, + message="IDENTITY transformation must not have configuration", + context={ + "transform_type": transformation_type.value, + "input_type": type(data).__name__, + }, + ) + elif config is None: + raise ModelOnexError( + error_code=EnumCoreErrorCode.VALIDATION_ERROR, + message=f"Configuration required for {transformation_type} transformation", + context={ + "transform_type": transformation_type.value, + "input_type": type(data).__name__, + }, + )This keeps pipeline behavior unchanged (since steps are already validated) but hardens the public API against misuse and makes the “IDENTITY has no config” rule true no matter how
execute_transformationis invoked.Also applies to: 493-569
373-425: Minor: simplify root-path handling intransform_json_pathnow thatpathis always non-empty.Given
ModelTransformJsonPathConfig.validate_pathalready enforces thatpathis non-empty and starts with"$", thenot pathpart of the root check is effectively unreachable unless someone bypasses Pydantic validation.You could simplify this block to reflect the contract more directly:
- path = config.path - - # Handle root-level access - if not path or path == "$": - return data + path = config.path + + # Handle root-level access (validated to be a non-empty '$'-prefixed path) + if path == "$": + return dataNot essential, but it tightens the implementation to the documented model constraints and makes the early-return condition easier to read.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/omnibase_core/utils/compute_transformations.py(1 hunks)tests/unit/enums/test_enum_node_kind.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Always use Poetry for Python package management and task execution. Usepoetry runfor pytest, mypy, black, isort, and other tools. Never use pip directly or execute python commands without the poetry run prefix.
Enforce 100% strict mypy compliance. All functions must have complete type annotations. Usepoetry run mypy src/omnibase_core/to verify strict mode compliance with no errors.
Format code with black, sort imports with isort, and lint with ruff. Runpoetry run black src/ tests/,poetry run isort src/ tests/, andpoetry run ruff check --fix src/ tests/
Files:
tests/unit/enums/test_enum_node_kind.pysrc/omnibase_core/utils/compute_transformations.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Write tests using pytest with async support via pytest-asyncio. Maintain minimum 60% code coverage. Use pytest markers: @pytest.mark.unit, @pytest.mark.integration, @pytest.mark.slow
Add @pytest.mark.timeout() to prevent test hangs. Run tests withpoetry run pytest tests/ --timeout=60for 60-second timeout protection.
Files:
tests/unit/enums/test_enum_node_kind.py
src/omnibase_core/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/omnibase_core/**/*.py: Use protocol names withcontainer.get_service()for dependency injection, never resolve services by concrete class names. Example:container.get_service('ProtocolEventBus')instead ofcontainer.get_service('EventBusService')
Always use ModelONEXContainer for dependency injection in node constructors and service resolution. Never confuse it with ModelContainer[T], which is a generic value wrapper, not a DI container.
Use structured error handling with ModelOnexError and EnumCoreErrorCode, never raise generic Exception. Always chain exceptions withfrom ewhen catching and re-raising.
Use the @standard_error_handling decorator on methods to eliminate 6+ lines of try/catch boilerplate. Example:@standard_error_handlingabove async methods.
Respect thread safety constraints: Do NOT share node instances (NodeCompute, NodeEffect), cache, or circuit breaker instances across threads without synchronization. Use thread-local instances or locks.
Use duck typing with protocols. Do not use isinstance checks with protocol types. Rely on the protocol interface directly.
Use EnumNodeKind for high-level architectural classification (EFFECT, COMPUTE, REDUCER, ORCHESTRATOR) when routing data through the ONEX pipeline. Use EnumNodeType for specific implementation discovery and capability matching.
Use ModelEventEnvelope for inter-service event-driven communication. Process events through the envelope pattern for consistent event handling.
Files:
src/omnibase_core/utils/compute_transformations.py
🧠 Learnings (3)
📓 Common learnings
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T23:34:11.788Z
Learning: Applies to src/omnibase_core/nodes/node_compute.py : NodeCompute is for data processing and transformation. Inherit from NodeCompute for COMPUTE nodes that perform calculations, validations, and data mapping.
📚 Learning: 2025-12-06T23:34:11.788Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T23:34:11.788Z
Learning: Applies to src/omnibase_core/**/*.py : Use EnumNodeKind for high-level architectural classification (EFFECT, COMPUTE, REDUCER, ORCHESTRATOR) when routing data through the ONEX pipeline. Use EnumNodeType for specific implementation discovery and capability matching.
Applied to files:
tests/unit/enums/test_enum_node_kind.py
📚 Learning: 2025-12-06T23:34:11.788Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T23:34:11.788Z
Learning: Applies to src/omnibase_core/**/*.py : Use structured error handling with ModelOnexError and EnumCoreErrorCode, never raise generic Exception. Always chain exceptions with `from e` when catching and re-raising.
Applied to files:
tests/unit/enums/test_enum_node_kind.py
🧬 Code graph analysis (2)
tests/unit/enums/test_enum_node_kind.py (1)
src/omnibase_core/enums/enum_node_type.py (2)
EnumNodeType(96-321)get_node_kind(194-227)
src/omnibase_core/utils/compute_transformations.py (11)
src/omnibase_core/enums/enum_case_mode.py (1)
EnumCaseMode(10-22)src/omnibase_core/enums/enum_core_error_code.py (1)
EnumCoreErrorCode(9-154)src/omnibase_core/enums/enum_regex_flag.py (1)
EnumRegexFlag(10-22)src/omnibase_core/enums/enum_transformation_type.py (1)
EnumTransformationType(11-34)src/omnibase_core/enums/enum_trim_mode.py (1)
EnumTrimMode(10-22)src/omnibase_core/enums/enum_unicode_form.py (1)
EnumUnicodeForm(10-24)src/omnibase_core/models/transformations/model_transform_case_config.py (1)
ModelTransformCaseConfig(15-27)src/omnibase_core/models/transformations/model_transform_json_path_config.py (1)
ModelTransformJsonPathConfig(13-35)src/omnibase_core/models/transformations/model_transform_regex_config.py (1)
ModelTransformRegexConfig(15-31)src/omnibase_core/models/transformations/model_transform_trim_config.py (1)
ModelTransformTrimConfig(15-27)src/omnibase_core/models/transformations/model_transform_unicode_config.py (1)
ModelTransformUnicodeConfig(15-27)
⏰ 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: Smoke Tests
🔇 Additional comments (3)
tests/unit/enums/test_enum_node_kind.py (1)
476-495: LGTM! Test correctly validates ValueError for UNKNOWN node type.The test changes correctly implement the architectural decision to use
ValueErrorinstead ofModelOnexErrorto avoid circular enum→model imports. The docstring clearly explains the design rationale, and the assertion appropriately validates that the error message contains "UNKNOWN".src/omnibase_core/utils/compute_transformations.py (2)
1-50: Module-level design, docs, and type aliases are clear and consistent.The top-level docstring, thread-safety notes, and
JsonValue/JsonType/TransformationHandleraliases nicely document the intended usage and constraints for this module. The examples align with the actual API surface and NodeCompute v1.0 spec as described in the PR.
77-116: Good reuse of_validate_string_inputand consistentModelOnexErrorusage.Centralizing string-type validation in
_validate_string_inputand reusing it across REGEX/CASE/TRIM/UNICODE keeps error semantics consistent and matches the guideline to raiseModelOnexErrorwithEnumCoreErrorCode.VALIDATION_ERRORfor type issues. No changes needed here.Also applies to: 173-365
1 participant
Summary
Implement the v1.0 contract-driven NodeCompute specification with sequential pipelines, abort-on-first-failure semantics, and 6 built-in transformation types.
This is a minimal, stable foundation that can be shipped safely and extended incrementally.
v1.0 Scope
$.input/$.steps.<name>.outputpathsFiles Added
Enums (6)
EnumComputeStepType- VALIDATION, TRANSFORMATION, MAPPINGEnumTransformationType- 6 transformation typesEnumCaseMode,EnumRegexFlag,EnumTrimMode,EnumUnicodeFormModels (16)
ModelComputeExecutionContext- Typed execution contextModelComputeStepMetadata/Result- Step-level execution trackingModelComputePipelineResult- Pipeline result with abort semanticsModelComputePipelineStep- Pipeline step with model validatorModelComputeSubcontract- v1.0 contract definitionModelTransformationConfig)Execution (3)
compute_transformations.py- 6 pure transformation functionscompute_executor.py- Pipeline executor with path resolutionMixinComputeExecution- Integration mixin for NodeComputeDocumentation (3)
CONTRACT_DRIVEN_NODECOMPUTE_V1_0.md- v1.0 specificationNODECOMPUTE_VERSIONING_ROADMAP.md- Version roadmapNODECOMPUTE_FULL_DESIGN_V1X_TARGET.md- Full vision documentKey Design Decisions
frozen=Truefor future parallel supportTest plan
Linear Issue
OMN-465
Summary by CodeRabbit
New Features
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.