Repository navigation
refactor(contracts): enforce contract_version field, remove deprecated version fallback [OMN-1436] - #425
Merged
Conversation
…d version fallback [OMN-1436] Remove backward compatibility for deprecated `version` field in node contracts: - Delete duck-typed fallback in contract_hash_registry.py - Add model_validator to ModelYamlContract rejecting `version` field - Add early rejection in util_contract_loader.py with clear error - Update compute_contract_fingerprint.py to reject deprecated field - Update regenerate_fingerprints.py to skip files with deprecated field - Update test fixtures to use contract_version Breaking change: Node contracts using `version:` will now fail with clear error message directing users to use `contract_version:` instead (OMN-1431). Handler contracts (ModelHandlerContract) intentionally keep `version` field.
Contributor
📝 WalkthroughWalkthroughEnforces deprecation of top-level Changes
Sequence Diagram(s)sequenceDiagram
participant Script as scripts/regenerate_fingerprints.py
participant Loader as util_contract_loader._parse_contract_content
participant Validator as compute_contract_fingerprint.validate_strict_contract
participant Registry as contract_hash_registry.compute_contract_fingerprint
Script->>Loader: load YAML content
alt top-level "version" present without "contract_version"
Loader-->>Script: raise ModelOnexError (CONTRACT_VALIDATION_ERROR)
else parsed dict returned
Loader-->>Script: parsed dict
Script->>Validator: validate_strict_contract(parsed dict)
alt strict contract (validator -> True)
Validator-->>Script: True
Script->>Registry: compute_contract_fingerprint(strict path / ModelContractCompute)
Registry-->>Script: fingerprint
else flexible contract (validator -> False)
Validator-->>Script: False
Script->>Registry: compute_contract_fingerprint(flexible path / ModelYamlContract)
Registry-->>Script: fingerprint
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
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_hash_registry.py (1)
46-65: Add explicitConfigDictmodel_config for ModelTestContract and include thefrom __future__ import annotationsimport.The Pydantic model lacks the required
model_configattribute. Per coding guidelines, all Python files with Pydantic models must have explicitmodel_configwithConfigDict(not empty). Also ensurefrom __future__ import annotationsis placed as the second import after the module docstring.🛠️ Proposed change
+from __future__ import annotations + import json from datetime import datetime import pytest -from pydantic import BaseModel, Field, field_validator +from pydantic import BaseModel, ConfigDict, Field, field_validator from omnibase_core.contracts import ( @@ -46,6 +48,11 @@ from omnibase_core.models.errors.model_onex_error import ModelOnexError class ModelTestContract(BaseModel): """Simple test contract model for fingerprinting tests. + model_config = ConfigDict( + extra="ignore", + validate_assignment=True, + ) + Note: contract_version field accepts both string ("1.0.0") and ModelContractVersion for test convenience, but is stored as ModelContractVersion. """
🤖 Fix all issues with AI agents
In `@scripts/regenerate_fingerprints.py`:
- Around line 616-634: The current check in regenerate_fingerprints.py only
skips files where "version" exists and "contract_version" does not, but loaders
reject any presence of "version"; update the condition in the block that returns
RegenerateResult (the one using has_version and has_contract_version) to skip
whenever "version" is present (i.e., use if has_version:) so that any contract
containing the deprecated "version" field returns the same skipped
RegenerateResult with the existing skip_reason, file_path,
old_fingerprint/new_fingerprint/changing flags and message.
In `@src/omnibase_core/models/contracts/model_yaml_contract.py`:
- Around line 66-75: Replace the ValueError in reject_deprecated_version_field
with a ModelOnexError so ONEX error codes and context are preserved: change the
raise in model method reject_deprecated_version_field to raise
ModelOnexError(...) using an appropriate EnumCoreErrorCode (as used elsewhere,
e.g., in scripts/compute_contract_fingerprint.py), include the same descriptive
message and any relevant context/field name in the ModelOnexError constructor so
error code and structured info are retained.
…tor [OMN-1436]
Split is_strict_contract() into two functions per code review feedback:
- validate_strict_contract(): canonical enforcement function that raises
ModelOnexError with detailed context for invalid contracts
- is_strict_contract(): pure predicate wrapper that never raises
Changes:
- Standardize error codes to CONTRACT_VALIDATION_ERROR consistently
- Use context={} dict pattern for structured error details
- Add 17 new tests including critical "both versions present" footgun test
- Update test fixtures to use contract_version in lint_contract and
regenerate_fingerprints tests
Contributor
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 (2)
tests/unit/scripts/test_regenerate_fingerprints.py (1)
69-129: Update YAML fixtures to usecontract_version(deprecatedversionnow rejected).With the new enforcement, these fixtures will be skipped or error out, reducing coverage and potentially failing tests. Please migrate them to
contract_versionconsistently.🛠️ Example update (apply similarly to other fixtures)
-version: +contract_version: major: 1 minor: 0 patch: 0tests/unit/scripts/test_lint_contract.py (1)
68-86: Add explicit Pydanticmodel_configtoModelSimpleTestContract.The model lacks
model_config, which violates project guidelines and can silently accept deprecated or unexpected fields. AddConfigDictto enforce strict validation:Proposed fix
-from pydantic import BaseModel, Field, field_validator +from pydantic import BaseModel, ConfigDict, Field, field_validator class ModelSimpleTestContract(BaseModel): + model_config = ConfigDict(extra="forbid", validate_assignment=True)
🤖 Fix all issues with AI agents
In `@tests/unit/scripts/test_compute_contract_fingerprint.py`:
- Around line 140-157: The assertions in the tests are looking up a nested
"context" key that doesn't exist; update the tests that inspect error.context
(in the test functions around validate_strict_contract usage, e.g.,
test_error_includes_migration_guidance and the earlier test checking
has_contract_version) to replace error.context.get("additional_context",
{}).get("context", {}) with error.context.get("additional_context", {}), so the
tests read the direct context dict produced by validate_strict_contract.
Comment on lines
+140
to
+157
| # Context is nested under additional_context.context | ||
| assert error.context is not None | ||
| inner_context = error.context.get("additional_context", {}).get("context", {}) | ||
| assert inner_context.get("has_contract_version") is True | ||
|
|
||
| def test_error_includes_migration_guidance( | ||
| self, deprecated_version_contract_data: dict[str, object] | ||
| ) -> None: | ||
| """Test that error message includes helpful migration guidance.""" | ||
| with pytest.raises(ModelOnexError) as exc_info: | ||
| validate_strict_contract(deprecated_version_contract_data) | ||
|
|
||
| error = exc_info.value | ||
| # Check that context includes actionable guidance | ||
| # Context is nested under additional_context.context | ||
| assert error.context is not None | ||
| inner_context = error.context.get("additional_context", {}).get("context", {}) | ||
| assert "suggestion" in inner_context |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find ModelOnexError implementation
find . -name "*.py" -type f | xargs grep -l "class ModelOnexError" | head -5Repository: OmniNode-ai/omnibase_core
Length of output: 182
🏁 Script executed:
#!/bin/bash
# Locate validate_strict_contract
find . -name "*.py" -type f | xargs grep -l "def validate_strict_contract" | head -5Repository: OmniNode-ai/omnibase_core
Length of output: 110
🏁 Script executed:
#!/bin/bash
# Get the test file to understand context better
cat -n tests/unit/scripts/test_compute_contract_fingerprint.py | sed -n '130,170p'Repository: OmniNode-ai/omnibase_core
Length of output: 2217
🏁 Script executed:
#!/bin/bash
# Inspect ModelOnexError implementation
cat -n src/omnibase_core/models/errors/model_onex_error.py | head -150Repository: OmniNode-ai/omnibase_core
Length of output: 7136
🏁 Script executed:
#!/bin/bash
# Look at model_onex_error_details to understand context structure
cat -n src/omnibase_core/models/core/model_onex_error_details.pyRepository: OmniNode-ai/omnibase_core
Length of output: 3686
🏁 Script executed:
#!/bin/bash
# Inspect validate_strict_contract implementation
cat -n scripts/compute_contract_fingerprint.py | head -200Repository: OmniNode-ai/omnibase_core
Length of output: 9755
🏁 Script executed:
#!/bin/bash
# Check the full ModelOnexError class, especially the context property
cat -n src/omnibase_core/models/errors/model_onex_error.py | sed -n '140,250p'Repository: OmniNode-ai/omnibase_core
Length of output: 4555
🏁 Script executed:
#!/bin/bash
# Look for the context property definition
rg -n "def context|@property" src/omnibase_core/models/errors/model_onex_error.py -A 3Repository: OmniNode-ai/omnibase_core
Length of output: 1118
🏁 Script executed:
#!/bin/bash
# Check _ModelOnexErrorData to understand the full context structure
cat -n src/omnibase_core/models/common/model_onex_error_data.pyRepository: OmniNode-ai/omnibase_core
Length of output: 3188
🏁 Script executed:
#!/bin/bash
# Let me trace through what gets stored in _simple_context more clearly
# by looking at the complete __init__ method
cat -n src/omnibase_core/models/errors/model_onex_error.py | sed -n '61,144p'Repository: OmniNode-ai/omnibase_core
Length of output: 4113
🏁 Script executed:
#!/bin/bash
# Verify the exact structure of TypedDictBasicErrorContext
rg -n "TypedDictBasicErrorContext" src/omnibase_core/types/type_core.py -A 10Repository: OmniNode-ai/omnibase_core
Length of output: 233
🏁 Script executed:
#!/bin/bash
# Find and inspect TypedDictBasicErrorContext definition
cat -n src/omnibase_core/types/typed_dict_basic_error_context.pyRepository: OmniNode-ai/omnibase_core
Length of output: 1761
Update context assertions to remove extra nesting level.
In validate_strict_contract(), all context values (e.g., deprecated_field, suggestion, has_contract_version) are passed directly in the context dict and become error.context["additional_context"]. The test assertions incorrectly access .get("context", {}) on top of that, looking for a non-existent nested context key. Change both lines 142 and 156 from:
inner_context = error.context.get("additional_context", {}).get("context", {})to:
inner_context = error.context.get("additional_context", {})🤖 Prompt for AI Agents
In `@tests/unit/scripts/test_compute_contract_fingerprint.py` around lines 140 -
157, The assertions in the tests are looking up a nested "context" key that
doesn't exist; update the tests that inspect error.context (in the test
functions around validate_strict_contract usage, e.g.,
test_error_includes_migration_guidance and the earlier test checking
has_contract_version) to replace error.context.get("additional_context",
{}).get("context", {}) with error.context.get("additional_context", {}), so the
tests read the direct context dict produced by validate_strict_contract.
…1436] Address code review feedback for PR #425: - Fix regenerate_fingerprints.py skip logic to reject ANY presence of deprecated 'version' field (not just when contract_version is absent), matching validate_strict_contract() behavior - Replace fragile sys.path manipulation with importlib.util for cleaner script imports in test_compute_contract_fingerprint.py - Update skip_reason message with clearer migration guidance
… all backwards compat [OMN-1436] - validate_strict_contract() now raises ModelOnexError when strict fields present but contract_version is missing (no more silent acceptance) - Fix docstring examples to use "Raises ModelOnexError" syntax - Update tests to verify new stricter validation behavior - Add tests for predicate wrapper handling and error context validation
…rprint.py [OMN-1436] Address code review feedback: - Convert invalid doctest examples to reStructuredText code blocks - Examples using ">>> func()" with "Raises ModelOnexError" as output were invalid doctest syntax - Enhance error propagation comment at validate_strict_contract() call site to clarify that ModelOnexError propagates intentionally
…: ModelSemVer [OMN-1436]
Complete handler contract migration for full consistency with node contracts:
- Change ModelHandlerContract field from `version: str` to `contract_version: ModelSemVer`
- Add model_validator to reject deprecated `version` field with clear migration guidance
- Update protocol_handler_contract.py: `handler_version` -> `contract_version`
- Update JSON schema to use structured version object
- Update all validators, merge engine, and CLI references
- Update validation scripts for structured version format
- Update HANDLER_CONTRACT_GUIDE.md with migration examples
- Update CHANGELOG.md with breaking change documentation
- Standardize error messages to use "Rename" language consistently
- Fix pre-existing missing node_type in model-validate demo contract
BREAKING CHANGE: Handler contracts now use `contract_version` (ModelSemVer)
instead of `version` (str). YAML format changes from:
version: "1.0.0"
To:
contract_version:
major: 1
minor: 0
patch: 0
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
scripts/validation/handler_contract_validator.py (1)
307-339: Reject deprecatedversioninputs explicitly.
extra="allow"means payloads with bothcontract_versionand legacyversionstill validate, which undermines the deprecation policy. Add a before-model validator to fail fast whenversionis present.🔧 Proposed fix
class MinimalHandlerContract(BaseModel): @@ model_config = ConfigDict( extra="allow", # Allow additional fields for flexible contract formats validate_default=True, str_strip_whitespace=True, validate_assignment=True, ) + + `@model_validator`(mode="before") + `@classmethod` + def reject_deprecated_version(cls, data: Any) -> Any: + if isinstance(data, dict) and "version" in data: + raise ValueError( + "Deprecated 'version' field is not allowed. " + "Use 'contract_version' instead." + ) + return datasrc/omnibase_core/schemas/handler_contract.schema.json (1)
428-428: Schema description/examples still referenceversion.The top-level description and examples mention
versionand string semantics; update them tocontract_versionto avoid misleading schema docs.src/omnibase_core/protocols/handler/contracts/protocol_handler_contract.py (1)
299-305: Update YAML examples to the structuredcontract_versionformat.
If handler contracts now require the structured object, the YAML samples should reflect that to avoid misleading consumers.Suggested doc tweak
- contract_version: "1.0.0" + contract_version: + major: 1 + minor: 0 + patch: 0 @@ - contract_version: "1.0.0" + contract_version: + major: 1 + minor: 0 + patch: 0Also applies to: 345-351
🤖 Fix all issues with AI agents
In `@scripts/validation/validate-contracts.py`:
- Around line 146-159: When validating the contract version in the fallback path
(the block that checks "contract_version" on the content dict and appends to
errors), add an explicit check for the legacy "version" field and append a
deprecation-specific message (e.g., "Deprecated field 'version' found; please
use 'contract_version' with {major, minor, patch}") instead of only the generic
"Missing required field: contract_version"; keep existing type/structure checks
for "contract_version" (cv variable) unchanged and only trigger the deprecation
message when "version" is present on the content dict so callers get consistent
deprecated-field messaging.
In `@src/omnibase_core/models/contracts/model_handler_contract.py`:
- Around line 275-295: Replace the ValueError in the classmethod
reject_deprecated_version_field with a ModelOnexError that includes an ONEX
domain error code (e.g., "E_ONEX_DOMAIN_DEPRECATED_FIELD") and the same
descriptive message; ensure ModelOnexError is imported where used and update any
tests that assert ValueError to expect ModelOnexError and the corresponding
error code instead.
In `@src/omnibase_core/schemas/handler_contract.schema.json`:
- Around line 444-479: The schema incorrectly defines
contract_version.prerelease and contract_version.build as "string | null" but
must match ModelSemVer which uses arrays (tuples) not plain strings; update
contract_version.prerelease to accept "array | null" with items allowing string
or integer (to represent tuple[str | int, ...]) and update
contract_version.build to accept "array | null" with items as string (tuple[str,
...]), ensure arrays allow one-or-more items (minItems: 1), preserve default:
null and existing descriptions, and adjust anyOf blocks for these properties
rather than leaving them as string types.
🧹 Nitpick comments (4)
tests/unit/validation/phases/test_expanded_validator.py (1)
20-36: Addfrom __future__ import annotationsafter the module docstring.This file uses modern typing; the guideline requires the future import.
♻️ Suggested update
"""Tests for ExpandedContractValidator (Phase 3 Contract Validation). @@ Related: - OMN-1128: Contract Validation Pipeline """ +from __future__ import annotations + import pytest from pydantic import ValidationErrorAs per coding guidelines, please align imports accordingly.
tests/unit/models/contracts/test_model_handler_contract.py (1)
15-30: Addfrom __future__ import annotationsafter the module docstring.Required by the Python file guidelines.
♻️ Suggested update
""" Unit tests for ModelHandlerContract. @@ """ +from __future__ import annotations + import pytest from pydantic import ValidationErrorAs per coding guidelines, please align imports accordingly.
tests/unit/resolution/test_execution_resolver.py (1)
19-38: Addfrom __future__ import annotationsafter the module docstring.Required by the Python file guidelines.
♻️ Suggested update
""" Comprehensive tests for ExecutionResolver. @@ """ +from __future__ import annotations + from typing import AnyAs per coding guidelines, please align imports accordingly.
src/omnibase_core/models/contracts/model_handler_contract.py (1)
42-55: Addfrom __future__ import annotationsafter the module docstring.Required by the Python file guidelines.
♻️ Suggested update
""" Handler Contract Model for ONEX Framework. @@ """ +from __future__ import annotations + from typing import AnyAs per coding guidelines, please align imports accordingly.
Comment on lines
+275
to
+295
| @model_validator(mode="before") | ||
| @classmethod | ||
| def reject_deprecated_version_field(cls, data: Any) -> Any: | ||
| """ | ||
| Reject deprecated 'version' field - use 'contract_version' instead. | ||
|
|
||
| Args: | ||
| data: Raw input data. | ||
|
|
||
| Returns: | ||
| Validated data. | ||
|
|
||
| Raises: | ||
| ValueError: If deprecated 'version' field is present. | ||
| """ | ||
| if isinstance(data, dict) and "version" in data: | ||
| raise ValueError( | ||
| "Handler contracts must use 'contract_version', not 'version'. " | ||
| "The 'version' field was renamed per ONEX specification (OMN-1436)." | ||
| ) | ||
| return data |
Contributor
There was a problem hiding this comment.
Use ModelOnexError with error code for deprecated version.
This is an ONEX domain validation error and should carry a serializable error code. Update tests expecting ValueError accordingly.
🐛 Suggested fix
if isinstance(data, dict) and "version" in data:
- raise ValueError(
- "Handler contracts must use 'contract_version', not 'version'. "
- "The 'version' field was renamed per ONEX specification (OMN-1436)."
- )
+ raise ModelOnexError(
+ message=(
+ "Handler contracts must use 'contract_version', not 'version'. "
+ "The 'version' field was renamed per ONEX specification (OMN-1436)."
+ ),
+ error_code=EnumCoreErrorCode.VALIDATION_ERROR,
+ )As per coding guidelines, ONEX domain errors should use ModelOnexError with codes.
🤖 Prompt for AI Agents
In `@src/omnibase_core/models/contracts/model_handler_contract.py` around lines
275 - 295, Replace the ValueError in the classmethod
reject_deprecated_version_field with a ModelOnexError that includes an ONEX
domain error code (e.g., "E_ONEX_DOMAIN_DEPRECATED_FIELD") and the same
descriptive message; ensure ModelOnexError is imported where used and update any
tests that assert ValueError to expect ModelOnexError and the corresponding
error code instead.
…mVer [OMN-1436] Fix type mismatch in handler_contract.schema.json where prerelease and build fields were incorrectly defined as simple strings instead of arrays: - prerelease: string|null -> array[string|integer]|null - build: string|null -> array[string]|null This aligns with ModelSemVer Python definition: - prerelease: tuple[str | int, ...] | None - build: tuple[str, ...] | None
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/omnibase_core/schemas/handler_contract.schema.json`:
- Around line 444-496: Update any top-level description and example text that
still refer to a "version" string to instead reference the required
"contract_version" object shape; specifically change documentation and example
snippets that mention a version string to illustrate the contract_version
structure (fields: major, minor, patch, optional prerelease and build) used by
the schema defined under "contract_version" and update the example at the other
noted location (lines around 588-592) to show contract_version JSON instead of a
version string. Ensure descriptive text, examples, and titles consistently use
"contract_version" and reflect its object form and defaults (prerelease/build
can be null).
- Around line 463-490: The "prerelease" and "build" array schemas currently
allow empty arrays which are invalid per SemVer; update the JSON schema for the
prerelease and build definitions (the anyOf branches that use "type": "array"
and "items") to include "minItems": 1 so empty arrays are rejected—ensure you
add minItems: 1 inside both the prerelease array schema and the build array
schema (leave the null anyOf branch and defaults untouched).
…OMN-1436] - Fix 7 test failures in test_contract_versioning.py by renaming version -> contract_version in test models and fixtures - Fix 6 test failures in test_protocol_handler_contracts.py by adding contract_version: ModelSemVer property to DuckTypedHandlerContract - Replace ValueError with ModelOnexError in model_handler_contract.py reject_deprecated_version_field validator - Add deprecation check for legacy 'version' field in validate-contracts.py - Fix context assertions in test_compute_contract_fingerprint.py - Add minItems:1 to prerelease/build arrays in handler_contract.schema.json - Update schema documentation to reference contract_version object format - Add from __future__ import annotations to 4 test/source files
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
tests/unit/orchestrator/test_contract_versioning.py (1)
151-170: Add explicitmodel_configto ModelMinimalContract.This BaseModel lacks the required ConfigDict. Per coding guidelines, all Pydantic models must have explicit
model_configwith ConfigDict (no empty ConfigDict).♻️ Proposed fix
class ModelMinimalContract(BaseModel): """Minimal contract model for version field testing.""" name: str = Field(default="minimal") contract_version: ModelContractVersion = Field( default_factory=lambda: ModelContractVersion(major=0, minor=0, patch=0) ) `@field_validator`("contract_version", mode="before") `@classmethod` def convert_version(cls, v: object) -> ModelContractVersion: """Convert string versions to ModelContractVersion.""" if isinstance(v, ModelContractVersion): return v if isinstance(v, str): return ModelContractVersion.from_string(v) if isinstance(v, dict): return ModelContractVersion.model_validate(v) raise ValueError(f"Cannot convert {type(v).__name__} to ModelContractVersion") + + model_config = ConfigDict( + extra="forbid", + validate_assignment=True, + )tests/unit/validation/phases/test_expanded_validator.py (1)
266-284: Change exception type fromValueErrortoModelOnexError.The
ModelHandlerContract.reject_deprecated_version_fieldvalidator raisesModelOnexError, notValueError. Update the test to match the actual exception thrown.Proposed fix
- with pytest.raises(ValueError, match="contract_version"): + with pytest.raises(ModelOnexError, match="contract_version"): ModelHandlerContract( handler_id="node.test.compute", name="Test Handler", version="1.0.0", # Deprecated field - should be rejectedtests/unit/models/contracts/test_model_handler_contract.py (1)
204-214: Add required NOTE comment above# type: ignoreCoding guidelines require a
# NOTE(OMN-XXXX): ...line above each type-ignore explaining why it is necessary.As per coding guidelines.♻️ Proposed fix
ModelHandlerContract( handler_id="node.test", name="Test", - version="1.0.0", # type: ignore[call-arg] + # NOTE(OMN-1436): Intentional deprecated field to assert rejection. + version="1.0.0", # type: ignore[call-arg] descriptor=ModelHandlerBehavior(handler_kind="compute"), input_model="a.Input", output_model="a.Output", )
jonahgabriel
commented
Jan 22, 2026
| "Handler contracts must use 'contract_version', not 'version'. " | ||
| "The 'version' field was renamed per ONEX specification (OMN-1436)." | ||
| ), | ||
| error_code=EnumCoreErrorCode.VALIDATION_ERROR, |
Contributor
Author
There was a problem hiding this comment.
Inconsistent error code: This uses VALIDATION_ERROR while util_contract_loader.py and compute_contract_fingerprint.py use CONTRACT_VALIDATION_ERROR for the same validation (rejecting deprecated version field). See CLAUDE.md error handling guidelines.
Suggested change
| error_code=EnumCoreErrorCode.VALIDATION_ERROR, | |
| error_code=EnumCoreErrorCode.CONTRACT_VALIDATION_ERROR, |
…field rejection [OMN-1436] Aligns error code with util_contract_loader.py and compute_contract_fingerprint.py which use CONTRACT_VALIDATION_ERROR for the same validation.
…on rejection [OMN-1436] The model validator in ModelHandlerContract now raises ModelOnexError (with CONTRACT_VALIDATION_ERROR) instead of ValueError when the deprecated 'version' field is used. Update the test to expect the correct exception type.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Remove backward compatibility for deprecated
versionfield in node contracts. The codebase now fails hard with clear error messages when deprecated fields are used.contract_hash_registry.pymodel_validatortoModelYamlContractrejectingversionfieldutil_contract_loader.pywithModelOnexErrorcompute_contract_fingerprint.pyto reject deprecated fieldregenerate_fingerprints.pyto skip files with deprecated fieldcontract_versionBreaking Change
Node contracts using
version:will now fail with clear error message directing users to usecontract_version:instead (per OMN-1431).Handler contracts (
ModelHandlerContract) intentionally keepversionfield - this is correct behavior.Test plan
Related
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.