M1: Implement Pydantic schema models v1 (Day Close + Ticket Contract)

[jonahgabriel]

Implements OMN-962 - Complete schema models for drift control artifacts.

Deliverables

• All enum files following naming conventions (Enum*/enum_*.py):

  • EnumDriftCategory - drift categories (scope, architecture, interfaces, dependencies, infra, process)
  • EnumEvidenceKind - evidence types (tests, docs, ci, benchmark, manual)
  • EnumInterfaceSurface - interface surfaces (events, topics, protocols, envelopes, public_api)
  • EnumInvariantStatus - invariant check status (pass, fail, unknown)
  • EnumPRState - PR states (merged, open)

• All model files following naming conventions (Model*/model_*.py):

  • ModelDayClose + 7 supporting models (ProcessChange, PlanItem, PR, ActualRepo, DriftDetected, InvariantsChecked, Risk)
  • ModelTicketContract + 2 supporting models (EvidenceRequirement, EmergencyBypass)

• SemVer validation for schema_version fields using regex pattern

• Schema purity: Models only import Pydantic and local enums (no runtime/infra coupling)

• 97% test coverage (12 tests, all passing)

• Models validate existing drift reports (tested with 2025-12-20.yaml)

Acceptance Criteria

✅ Unit tests cover core structural validation
✅ Models are schema-pure (no runtime/infra coupling)
✅ Naming conventions aligned with omnibase_core
✅ schema_version validated as SemVer

Testing

• All 12 tests passing
• 97% code coverage
• Models successfully parse existing YAML drift reports
• All linting and type checking passes

Refs: OMN-962

Summary by CodeRabbit

• New Features

  • Machine-checked schemas for Day Close reports and Ticket Contracts with SemVer and date validation, immutability, and DoS-resistant limits.
  • New typed enums for drift categories, evidence kinds, interface surfaces, invariant status, and PR state.
  • Shared validation patterns (SemVer) exposed for reuse.

• Tests

  • Comprehensive tests for validation, serialization (JSON/YAML) round-trips, security constraints, and YAML parsing.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Walkthrough

Adds new Pydantic schemas for day-close reports and ticket contracts, five enums, shared validation patterns, package-level re-exports, a revised day-close YAML, and a comprehensive test suite covering parsing, validation, security limits, and (de)serialization.

Changes

Cohort / File(s)	Summary
Configuration & Data drift/day_close/2025-12-20.yaml	Updated day-close report content: drift items regrouped (interfaces-focused), actual_by_repo expanded, invariants/priorities revised, corrections_for_tomorrow adjusted, and risks rephrased.
Package exports src/onex_change_control/__init__.py	Re-exports ModelDayClose and ModelTicketContract and updates __all__ to include them alongside __version__.
Enums (new) src/onex_change_control/enums/__init__.py, src/onex_change_control/enums/enum_drift_category.py, src/onex_change_control/enums/enum_evidence_kind.py, src/onex_change_control/enums/enum_interface_surface.py, src/onex_change_control/enums/enum_invariant_status.py, src/onex_change_control/enums/enum_pr_state.py	Added string-backed, @unique enums with __str__() for: EnumDriftCategory, EnumEvidenceKind, EnumInterfaceSurface, EnumInvariantStatus, EnumPRState; package init re-exports them.
Validation utilities (new) src/onex_change_control/validation/__init__.py, src/onex_change_control/validation/patterns.py	Added SEMVER_PATTERN (compiled regex) and re-exported it from package validation namespace for reuse in model validators.
Pydantic Models (new) src/onex_change_control/models/__init__.py, src/onex_change_control/models/model_day_close.py, src/onex_change_control/models/model_ticket_contract.py	Added ModelDayClose and nested submodels (process changes, plan items, PRs, repos, drift, invariants, risks) and ModelTicketContract with submodels (evidence requirement, emergency bypass). Enforced SemVer/date validators, list/string size limits, frozen (immutable) models, and cross-field validations (e.g., interface_change ↔ interfaces_touched, emergency bypass constraints).
Tests (new) tests/test_models.py, tests/test_yaml_parsing.py, tests/test_security_constraints.py, tests/test_serialization.py	Added comprehensive tests covering valid/invalid model construction, YAML parsing of day-close, SemVer/date validation, DoS-related limits (string/list max sizes), emergency bypass and cross-field constraints, enum serialization, JSON/YAML round-trips, and immutability behavior.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

• Pay extra attention to:
  • SemVer and ISO-date validators and their error messages
  • Cross-field validators: interface_change ↔ interfaces_touched, and emergency bypass logic
  • Size limits constants and tests exercising boundary/over-limit cases
  • Import/export ordering to avoid circular imports between enums and models
  • Tests that assert specific ValidationError messages (Pydantic version sensitivity)

Possibly related PRs

• M0: Bootstrap onex-change-control Poetry package + repo structure + baseline CI #1 — earlier bootstrap PR that modified package exports (__version__) and is related to the exported API changes in this PR.

"I nibbled at schemas late at night,
Validators snug and enums polite.
YAML trimmed and tests set right,
A rabbit cheered — the models bite. 🥕"

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'M1: Implement Pydantic schema models v1 (Day Close + Ticket Contract)' directly and clearly summarizes the main change: implementing Pydantic schema models for Day Close and Ticket Contract artifacts as part of the M1 milestone.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch jonah/omn-962-m1-implement-pydantic-schema-models-v1-day-close-ticket

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (5)

tests/test_yaml_parsing.py (1)

11-29: Consider adding file existence guard or using pytest fixture.

The test assumes drift/day_close/2025-12-19.yaml exists. If this file is modified or removed in future PRs, this test will fail. Consider using pytest.mark.skipif with a path existence check, or defining the test data inline like test_parse_ticket_contract_template does.

🔎 Optional: Add existence check

+import pytest
+
+YAML_PATH = Path(__file__).parent.parent / "drift" / "day_close" / "2025-12-19.yaml"
+
+@pytest.mark.skipif(not YAML_PATH.exists(), reason="YAML fixture not present")
 def test_parse_existing_day_close_yaml() -> None:
     """Test parsing existing day_close.yaml file."""
-    yaml_path = Path(__file__).parent.parent / "drift" / "day_close" / "2025-12-19.yaml"
+    yaml_path = YAML_PATH
     with yaml_path.open() as f:

src/onex_change_control/models/model_ticket_contract.py (2)

14-15: Consider extracting shared SemVer pattern.

This pattern is duplicated in model_day_close.py. Consider extracting to a shared module (e.g., _validators.py or _patterns.py) to maintain DRY and ensure consistency.

---

62-68: Redundant Field() definition.

The schema_version field uses both Annotated[str, Field(...)] and a separate = Field(...) assignment, which duplicates the description. Use one approach:

🔎 Proposed fix using Annotated only

-    schema_version: Annotated[
-        str,
-        Field(
-            ...,
-            description="Schema version (SemVer format, e.g., '1.0.0')",
-        ),
-    ] = Field(..., description="Schema version (SemVer format)")
+    schema_version: Annotated[
+        str,
+        Field(..., description="Schema version (SemVer format, e.g., '1.0.0')"),
+    ]

src/onex_change_control/models/model_day_close.py (2)

94-100: Redundant Field definition for schema_version.

The field is defined twice - once in the Annotated wrapper and again as the default value. This duplication is unnecessary and could cause confusion about which description takes precedence.

🔎 Suggested fix

-    schema_version: Annotated[
-        str,
-        Field(
-            ...,
-            description="Schema version (SemVer format, e.g., '1.0.0')",
-        ),
-    ] = Field(..., description="Schema version (SemVer format)")
+    schema_version: str = Field(
+        ..., description="Schema version (SemVer format, e.g., '1.0.0')"
+    )

---

139-147: Move date pattern to module level for consistency and performance.

The date_pattern regex is compiled inside the validator function on every call, unlike _SEMVER_PATTERN which is compiled once at module level.

🔎 Suggested fix

Add at module level (after line 16):

_DATE_PATTERN = re.compile(r"^\d{4}-\d{2}-\d{2}$")

Then update the validator:

     @field_validator("date")
     @classmethod
     def validate_date(cls, v: str) -> str:
         """Validate date is ISO format (YYYY-MM-DD)."""
-        date_pattern = re.compile(r"^\d{4}-\d{2}-\d{2}$")
-        if not date_pattern.match(v):
+        if not _DATE_PATTERN.match(v):
             msg = f"Invalid date format: {v}. Expected ISO format (YYYY-MM-DD)"
             raise ValueError(msg)
         return v

Note: The regex only validates format, not semantic validity (e.g., 2025-02-30 would pass). If calendar validity is needed, consider using datetime.strptime instead.

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 290c4e2 and 079cf40.

📒 Files selected for processing (13)

• drift/day_close/2025-12-20.yaml (1 hunks)
• src/onex_change_control/__init__.py (1 hunks)
• src/onex_change_control/enums/__init__.py (1 hunks)
• src/onex_change_control/enums/enum_drift_category.py (1 hunks)
• src/onex_change_control/enums/enum_evidence_kind.py (1 hunks)
• src/onex_change_control/enums/enum_interface_surface.py (1 hunks)
• src/onex_change_control/enums/enum_invariant_status.py (1 hunks)
• src/onex_change_control/enums/enum_pr_state.py (1 hunks)
• src/onex_change_control/models/__init__.py (1 hunks)
• src/onex_change_control/models/model_day_close.py (1 hunks)
• src/onex_change_control/models/model_ticket_contract.py (1 hunks)
• tests/test_models.py (1 hunks)
• tests/test_yaml_parsing.py (1 hunks)

🧰 Additional context used

🧬 Code graph analysis (5)

tests/test_yaml_parsing.py (2)

src/onex_change_control/models/model_day_close.py (1)

• ModelDayClose (88-147)

src/onex_change_control/models/model_ticket_contract.py (1)

• ModelTicketContract (55-110)

src/onex_change_control/models/model_ticket_contract.py (3)

src/onex_change_control/enums/enum_evidence_kind.py (1)

• EnumEvidenceKind (10-38)

src/onex_change_control/enums/enum_interface_surface.py (1)

• EnumInterfaceSurface (10-38)

src/onex_change_control/models/model_day_close.py (1)

• validate_schema_version (132-137)

src/onex_change_control/enums/__init__.py (5)

src/onex_change_control/enums/enum_drift_category.py (1)

• EnumDriftCategory (10-42)

src/onex_change_control/enums/enum_evidence_kind.py (1)

• EnumEvidenceKind (10-38)

src/onex_change_control/enums/enum_interface_surface.py (1)

• EnumInterfaceSurface (10-38)

src/onex_change_control/enums/enum_invariant_status.py (1)

• EnumInvariantStatus (10-30)

src/onex_change_control/enums/enum_pr_state.py (1)

• EnumPRState (10-26)

src/onex_change_control/models/__init__.py (2)

src/onex_change_control/models/model_day_close.py (1)

• ModelDayClose (88-147)

src/onex_change_control/models/model_ticket_contract.py (1)

• ModelTicketContract (55-110)

src/onex_change_control/models/model_day_close.py (4)

src/onex_change_control/enums/enum_drift_category.py (1)

• EnumDriftCategory (10-42)

src/onex_change_control/enums/enum_invariant_status.py (1)

• EnumInvariantStatus (10-30)

src/onex_change_control/enums/enum_pr_state.py (1)

• EnumPRState (10-26)

src/onex_change_control/models/model_ticket_contract.py (1)

• validate_schema_version (94-99)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)

• GitHub Check: claude-review

🔇 Additional comments (17)

src/onex_change_control/enums/enum_evidence_kind.py (1)

1-38: LGTM!

Clean enum implementation with proper @unique decorator, str inheritance for serialization, and comprehensive docstrings. The __str__ method correctly returns the underlying value for YAML/JSON compatibility.

src/onex_change_control/enums/enum_pr_state.py (1)

1-26: LGTM!

Consistent implementation pattern with the other enums. The two-state model (merged/open) is appropriate for daily close report tracking.

src/onex_change_control/__init__.py (1)

11-19: LGTM!

Clean public API exposure. The package exports the two main models while keeping implementation details (sub-models, enums) accessible via subpackages for users who need them.

tests/test_yaml_parsing.py (1)

32-70: LGTM with minor observation.

Good approach creating inline test data rather than relying on template files with placeholders. The test validates the critical fields and the model's constraint checking.

Consider adding an assertion on evidence_requirements content (e.g., assert contract.evidence_requirements[0].kind.value == "tests") for completeness, but this is optional since the model validation already confirms the structure is correct.

src/onex_change_control/enums/enum_invariant_status.py (1)

1-30: LGTM!

Clean implementation consistent with the other enums. The # noqa: S105 suppression on line 19 is appropriate—Bandit flags "pass" as a potential hardcoded password, but here it's clearly a status value.

src/onex_change_control/enums/__init__.py (1)

1-18: LGTM!

Clean package initialization with proper re-exports and __all__ declaration. The absolute imports align with Python best practices for package modules.

src/onex_change_control/models/__init__.py (1)

1-12: LGTM!

Consistent package structure mirroring the enums module. Public API surface is well-defined.

src/onex_change_control/models/model_ticket_contract.py (3)

18-27: LGTM!

Simple, well-documented model with appropriate field types and descriptions.

---

29-53: LGTM!

Good use of model_validator(mode="after") for conditional field requirements. The validation logic correctly enforces that justification and follow_up_ticket_id are non-empty when bypass is enabled.

---

101-110: Verify: asymmetric constraint is intentional.

The validator prevents interfaces_touched when interface_change=False, but doesn't require interfaces_touched to be non-empty when interface_change=True. This allows marking a ticket as changing interfaces without specifying which surfaces. Confirm this is the intended behavior.

src/onex_change_control/enums/enum_interface_surface.py (1)

1-38: LGTM!

Well-structured enum with proper @unique decorator, str mixin for serialization compatibility, and comprehensive docstrings. Consistent with the other enums in this PR.

src/onex_change_control/enums/enum_drift_category.py (1)

1-42: LGTM!

Consistent enum implementation following the established pattern. Well-documented drift categories with clear semantic meanings.

tests/test_models.py (2)

27-135: LGTM!

The TestModelDayClose class provides comprehensive coverage for the ModelDayClose model:

• Valid construction with minimal required fields
• Invalid schema version format validation
• Invalid date format validation
• Complete construction with all nested models populated

The test assertions are appropriate and match the expected model behavior.

---

137-225: LGTM!

The TestModelTicketContract class provides thorough validation coverage:

• Valid construction with defaults
• Schema version validation
• Cross-field constraint validation (interface_change vs interfaces_touched)
• Emergency bypass nested model validation for both required fields
• Complete construction with all optional fields populated

Good practice using a named constant for expected_interfaces_count to avoid magic numbers.

src/onex_change_control/models/model_day_close.py (2)

1-16: LGTM!

Good practices observed:

• Module-level compiled regex _SEMVER_PATTERN avoids recompilation on each validation call
• Imports are minimal and schema-pure (only Pydantic and local enums)

---

19-86: LGTM!

The sub-models are well-designed:

• Clear field descriptions for documentation
• Appropriate use of ge=1 constraint on PR numbers (line 37)
• Correct use of default_factory=list for mutable defaults
• Enum types properly referenced for typed fields

drift/day_close/2025-12-20.yaml (1)

1-132: LGTM!

The day close report structure correctly aligns with the ModelDayClose schema:

• Valid SemVer schema_version and ISO date format
• All nested structures match their corresponding Pydantic models
• Enum values (merged, open, interfaces, pass, unknown) are valid
• The report effectively documents the schema models implementation work and related drift resolutions

[claude]

PR Review: M1 - Pydantic Schema Models v1

Summary

This PR successfully implements foundational Pydantic schema models for the ONEX drift control system. The implementation follows the repository's naming conventions and provides a solid foundation for machine-checkable drift detection artifacts.

---

✅ Strengths

1. Excellent Naming Convention Adherence

• All models follow Model* / model_*.py pattern
• All enums follow Enum* / enum_*.py pattern
• Fully aligned with omnibase_core conventions per README.md

2. Schema Purity Maintained

• Models only import Pydantic and local enums
• No runtime/infrastructure coupling - excellent separation of concerns
• Clean dependencies that will make schema distribution straightforward

3. Strong Validation Logic

• SemVer validation using regex patterns (schema_version)
• Date format validation (ISO YYYY-MM-DD)
• Business logic validation (e.g., emergency bypass requires justification + follow-up ticket)
• Interface constraints properly enforced

4. Good Test Coverage (97%)

• 12 tests covering core structural validation
• Tests validate both happy paths and error cases
• Real YAML parsing tests prove models work with actual drift reports
• Edge case coverage (invalid formats, constraint violations)

5. Well-Documented Models

• Clear docstrings on all enums and models
• Field-level descriptions in Pydantic models
• Inline comments explaining business rules

---

🔍 Code Quality Issues & Recommendations

1. Duplicate Field Descriptions (Minor)

Location: src/onex_change_control/models/model_day_close.py:96-99

Issue: Field description is duplicated in both the Annotated type hint and the default field assignment.

Recommendation: Remove one of the duplicate descriptions. The standard Pydantic pattern is to use one or the other, not both.

Same issue in: model_ticket_contract.py:62-68

---

2. Potential SemVer Pattern Limitation (Medium)

Location: src/onex_change_control/models/model_day_close.py:16

Issue: Current pattern only supports basic SemVer (e.g., "1.0.0") but doesn't support:

• Pre-release versions: 1.0.0-alpha, 1.0.0-beta.1
• Build metadata: 1.0.0+20130313144700

Recommendation:

• If you only need basic versions, document this limitation clearly
• If you need full SemVer support, consider using a proper SemVer library

Same issue in: model_ticket_contract.py:15

---

3. Missing Inverse Validation Logic (Medium)

Location: src/onex_change_control/models/model_ticket_contract.py:102-110

Issue: The inverse is not validated. If interface_change=True but interfaces_touched=[], should that be allowed?

Recommendation: Consider adding validation for the inverse case and add test coverage.

---

4. Noqa Comment Needs Better Explanation (Minor)

Location: src/onex_change_control/enums/enum_invariant_status.py:19

Issue: The # noqa: S105 suppresses the "Possible hardcoded password" warning, but it's not immediately clear why PASS would trigger this.

Recommendation: Add an inline explanation like: # noqa: S105 - Not a password, it's an invariant status value

---

5. Date Validation Could Be Stricter (Low)

Location: src/onex_change_control/models/model_day_close.py:139-146

Issue: Current regex accepts invalid dates like "2025-99-99" or "2025-02-30".

Recommendation: Use Python's datetime.strptime() for proper date validation that checks calendar validity.

---

🔒 Security Considerations

✅ No Critical Security Issues Found

• No SQL injection vectors (no database interaction)
• No command injection risks (validation only)
• No unsafe file operations
• No hardcoded credentials (except false positive on enum value)
• Input validation is present for user-provided data
• Already using yaml.safe_load() correctly

Minor Security Recommendations:

1. ReDoS Risk (Low): The SemVer regex pattern is simple and unlikely to cause ReDoS
2. YAML Bomb Protection: Already using yaml.safe_load() - good practice!

---

⚡ Performance Considerations

✅ No Performance Issues

• Models are lightweight data containers
• Validation is efficient (simple regex + field checks)
• No expensive operations in validators
• YAML parsing uses safe_load (appropriate for config files)
• Regex patterns compiled at module level (good!)

---

🧪 Test Coverage Analysis

Current Coverage: 97% ✅

Well-tested areas:

• ✅ Valid model construction
• ✅ Invalid schema version format
• ✅ Invalid date format
• ✅ Emergency bypass validation
• ✅ Interface constraint validation
• ✅ Real YAML parsing

Missing test coverage suggestions:

1. PR number validation - Test pr: 0 or pr: -1 (should fail due to ge=1)
2. Inverse interface validation - Test interface_change=True with interfaces_touched=[]
3. Model serialization - Test model_dump() and model_dump_json()
4. Enum string serialization - Test that enums serialize correctly to YAML

---

📝 Documentation Suggestions

1. Add module-level usage examples in docstrings
2. Document validation constraints in class docstrings
3. Document SemVer pattern limitations if using simplified version

---

🎯 Summary & Recommendations

Must Fix Before Merge:

None - the code is functional and well-structured.

Should Fix (High Priority):

1. Add inverse validation for interface_change=True with empty interfaces_touched
2. Remove duplicate field descriptions in Annotated types
3. Clarify SemVer pattern scope - document limitations

Nice to Have (Medium Priority):

4. Improve date validation to reject invalid calendar dates
5. Add missing test coverage (serialization, edge cases)
6. Add module-level usage examples in docstrings

Low Priority:

7. Consistent magic number handling in tests
8. Better noqa comment explanations

---

✨ Overall Assessment

Recommendation: ✅ APPROVE with suggested improvements

This is excellent foundational work that successfully delivers on all stated acceptance criteria:

• ✅ Schema-pure models (no runtime coupling)
• ✅ Naming conventions aligned with omnibase_core
• ✅ SemVer validation for schema_version
• ✅ 97% test coverage
• ✅ Models validate existing drift reports

The code quality is high, follows best practices, and provides a solid foundation for the drift control system. The identified issues are mostly minor refinements that can be addressed in follow-up work or before merge, depending on team preference.

Great work! 🚀

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (2)

src/onex_change_control/models/model_ticket_contract.py (1)

13-17: Consider extracting the SemVer pattern to a shared module.

The _SEMVER_PATTERN regex is duplicated identically in model_day_close.py (line 18). Extract to a shared constants or validators module to maintain DRY and ensure consistent validation across models.

🔎 Suggested approach

Create a shared module, e.g., src/onex_change_control/validators.py:

import re

# SemVer pattern for schema_version validation
# Note: This pattern supports basic SemVer (major.minor.patch) only.
SEMVER_PATTERN = re.compile(r"^\d+\.\d+\.\d+$")


def validate_semver(v: str, field_name: str = "schema_version") -> str:
    """Validate a string is SemVer format."""
    if not SEMVER_PATTERN.match(v):
        msg = f"Invalid {field_name} format: {v}. Expected SemVer (e.g., '1.0.0')"
        raise ValueError(msg)
    return v

Then import and use in both model files.

tests/test_models.py (1)

214-228: Consider adding a test for valid emergency bypass when enabled.

The current tests validate that invalid configurations raise errors, but there's no test confirming a valid enabled bypass succeeds. This would complete the positive/negative test pair.

🔎 Suggested test

def test_emergency_bypass_valid_when_enabled(self) -> None:
    """Test valid emergency bypass configuration when enabled."""
    bypass = ModelEmergencyBypass(
        enabled=True,
        justification="Critical production fix required",
        follow_up_ticket_id="OMN-999",
    )
    assert bypass.enabled is True
    assert bypass.justification == "Critical production fix required"
    assert bypass.follow_up_ticket_id == "OMN-999"

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 079cf40 and 9b78a3e.

📒 Files selected for processing (5)

• src/onex_change_control/enums/enum_invariant_status.py (1 hunks)
• src/onex_change_control/models/model_day_close.py (1 hunks)
• src/onex_change_control/models/model_ticket_contract.py (1 hunks)
• tests/test_models.py (1 hunks)
• tests/test_yaml_parsing.py (1 hunks)

🚧 Files skipped from review as they are similar to previous changes (1)

• tests/test_yaml_parsing.py

🧰 Additional context used

🧬 Code graph analysis (1)

src/onex_change_control/models/model_ticket_contract.py (3)

src/onex_change_control/enums/enum_evidence_kind.py (1)

• EnumEvidenceKind (10-38)

src/onex_change_control/enums/enum_interface_surface.py (1)

• EnumInterfaceSurface (10-38)

src/onex_change_control/models/model_day_close.py (1)

• validate_schema_version (133-138)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)

• GitHub Check: claude-review

🔇 Additional comments (9)

src/onex_change_control/enums/enum_invariant_status.py (1)

1-30: LGTM!

Clean enum implementation following consistent patterns:

• str mixin enables JSON serialization
• @unique prevents duplicate values
• The # noqa: S105 correctly suppresses the Bandit false positive for "pass"
• __str__ method aligns with other enums in the codebase

src/onex_change_control/models/model_ticket_contract.py (2)

31-54: LGTM!

The ModelEmergencyBypass validator correctly enforces business rules: justification and follow_up_ticket_id are required when enabled=True. Using empty string defaults with validation is a clean approach.

---

57-117: LGTM!

Well-structured model with clear validation:

• SemVer validation for schema_version
• Proper constraint enforcement between interface_change and interfaces_touched
• Good documentation in the validator explaining the asymmetric constraint

tests/test_models.py (2)

28-165: LGTM!

Comprehensive test coverage for ModelDayClose:

• Validates SemVer and date format constraints
• Tests nested model construction
• Covers PR number boundary validation (ge=1)

---

257-273: LGTM!

Good practice documenting intentional edge-case behavior with a dedicated test. The docstring clearly explains the business rationale for allowing interface_change=True with an empty interfaces_touched list.

src/onex_change_control/models/model_day_close.py (4)

14-21: LGTM on pattern compilation.

Compiling regex patterns at module level is the right approach for performance. The comments clearly document the SemVer limitation.

---

140-151: Date format validation allows invalid calendar dates.

As documented, "2025-02-30" passes validation. If calendar validity is eventually needed, consider using datetime.strptime or Pydantic's built-in date type.

The current format-only validation is acceptable if downstream consumers handle invalid dates gracefully. Verify this is intentional for your use case.

---

24-91: LGTM!

Clean nested model hierarchy with:

• Consistent use of Field(...) for required fields
• Proper default_factory=list for collection defaults
• Clear descriptions on all fields
• Appropriate use of enum types for constrained values

---

93-129: LGTM!

ModelDayClose is well-designed:

• Required fields (schema_version, date, invariants_checked) enforce minimum viable reports
• Optional list fields default to empty, allowing incremental population
• Field descriptions provide good documentation

[claude]

PR Review: M1 - Pydantic Schema Models v1

Overview

This PR implements comprehensive Pydantic schema models for Day Close and Ticket Contract artifacts, achieving the stated goal of machine-checkable drift control schemas. The implementation is well-structured, follows established conventions, and includes solid test coverage.

---

✅ Strengths

Code Quality & Best Practices

• Excellent naming conventions: All models follow Model* pattern and enums follow Enum* pattern, consistent with omnibase_core standards
• Schema purity: Models correctly avoid runtime/infra coupling, importing only Pydantic and local enums
• Strong typing: All code uses proper type hints with mypy compliance
• Comprehensive docstrings: Clear, informative documentation for all classes, methods, and enum values
• Clean module structure: Well-organized init.py files with proper all exports

Test Coverage

• 97% coverage is excellent for schema validation code
• Tests cover both positive cases (valid inputs) and negative cases (validation errors)
• Real-world validation with existing YAML drift reports (2025-12-20.yaml)
• Edge cases covered (e.g., PR number validation, emergency bypass validation)

Security & Validation

• SemVer validation: Proper regex pattern for schema_version fields
• Date format validation: ISO 8601 format enforcement for date fields
• Business logic constraints: PR numbers must be >= 1, emergency bypass requires justification + follow-up ticket, interface change constraints properly enforced
• String enum inheritance: All enums inherit from str for proper serialization

---

🔍 Issues & Suggestions

1. Date Validation ⚠️

Location: src/onex_change_control/models/model_day_close.py:144-145

The regex validation will accept invalid dates like "2025-13-99" or "2025-02-30".

Recommendation: Use datetime.date.fromisoformat() for actual calendar validation without adding dependencies.

2. Test Coverage Gaps ⚠️

While 97% coverage is excellent, consider adding tests for:

• Calendar-invalid dates (e.g., "2025-02-30")
• SemVer edge cases (e.g., "1.0.0-alpha" should be rejected)
• Empty strings for required fields
• Unicode/special characters in string fields
• Serialization round-trip tests

3. Interface Change Constraint Design ⚠️

Location: src/onex_change_control/models/model_ticket_contract.py:76-87

Is it intentional that interface_change=True with empty interfaces_touched is valid? The test comment says this supports "categorization pending" but consider if this should require eventual validation.

4. Security Considerations

Consider adding max length constraints on string fields and max items constraints on lists to prevent DoS via massive inputs.

5. Performance Suggestions

• Consider adding frozen=True to Pydantic model configs if these are meant to be immutable
• Regex patterns compiled at module level is excellent (already done)

---

📋 Checklist Review

Based on the PR description:

✅ All enum files following naming conventions (Enum*/enum_.py)
✅ All model files following naming conventions (Model/model_*.py)
✅ SemVer validation for schema_version fields
✅ Schema purity (no runtime/infra coupling)
✅ 97% test coverage (exceeds 80% target)
✅ Models validate existing drift reports
✅ All linting and type checking passes

---

🎓 Recommendations Summary

Must Fix

1. Date validation: Use datetime.date.fromisoformat() for calendar-valid date checking

Should Consider

2. Test coverage: Add tests for calendar-invalid dates, SemVer edge cases, and boundary conditions
3. Security: Add max length/items constraints to prevent DoS
4. Immutability: Consider adding frozen=True to model configs
5. Interface change validation: Document steady state vs temporary for interface_change=True with empty interfaces_touched

Nice to Have

6. Serialization tests: Add round-trip serialization tests
7. JSON Schema export: Next step (OMN-963)

---

🏆 Final Assessment

Overall Quality: ⭐⭐⭐⭐⭐ (Excellent)

This is high-quality schema implementation work that:

• Follows established conventions meticulously
• Provides comprehensive validation
• Includes excellent test coverage
• Maintains schema purity as designed
• Has clear documentation

Recommendation: ✅ Approve with minor suggestions

The only blocking issue is the date validation which should use calendar validation. Other suggestions are enhancements that can be addressed in follow-up PRs if needed.

Great work! This provides a solid foundation for the drift control system.

[claude]

Code Review: PR #4 - M1: Pydantic Schema Models v1

Summary

This is a high-quality implementation that successfully delivers machine-checkable schemas for Day Close and Ticket Contract artifacts. The code demonstrates strong engineering practices with comprehensive test coverage (97%), excellent documentation, and thoughtful security constraints.

✅ Strengths

1. Code Quality & Best Practices

• Excellent naming conventions: Enum* and Model* prefixes align with stated conventions
• Comprehensive docstrings: Every class, field, and validator is well-documented with clear rationale
• Frozen models: Immutability via ConfigDict(frozen=True) ensures thread-safety and prevents accidental mutations
• Schema purity: Models only import Pydantic and local enums (no runtime/infra coupling) - critical for long-term maintainability
• Type annotations: Proper use of Python 3.10+ type hints throughout

2. Security

• DoS prevention: String and list length constraints (_MAX_STRING_LENGTH = 10000, _MAX_LIST_ITEMS = 1000) prevent resource exhaustion attacks
• Input validation: SemVer regex validation, ISO date validation with calendar checks
• No injection risks: All fields are properly validated through Pydantic
• Security test suite: Dedicated test_security_constraints.py validates DoS protection

3. Test Coverage (97%)

• Comprehensive test suites:
  • test_models.py (360 lines) - structural validation, edge cases
  • test_security_constraints.py (147 lines) - DoS prevention
  • test_serialization.py (300 lines) - JSON/YAML round-trip, enum serialization, immutability
  • test_yaml_parsing.py (75 lines) - real-world YAML parsing
• Edge case coverage: Calendar validation (Feb 30, month 13), PR number validation (0, negative), SemVer edge cases
• Real-world validation: Successfully parses existing 2025-12-20.yaml drift report

4. Validation Logic

• Multi-stage date validation: Format check + calendar validity check with clear error messages
• Business rule enforcement:
  • ModelEmergencyBypass: Requires justification + follow-up ticket when enabled
  • ModelTicketContract: Enforces interfaces_touched must be empty when interface_change=False
  • ModelDayClosePR: PR numbers must be >= 1
• Thoughtful flexibility: Allows interface_change=True with empty interfaces_touched for temporary/incomplete states (well-documented)

5. Documentation

• Clear module docstrings: Every file explains its purpose
• Inline comments: Complex patterns like SemVer regex include limitations (pre-release not supported)
• Validator documentation: Each validator explains what it validates and why

---

🔍 Issues & Recommendations

1. Potential Bug: Redundant String Length Validation (Priority: Low)

Location: model_day_close.py:62-71

class ModelDayClosePlanItem(BaseModel):
    requirement_id: str = Field(..., max_length=_MAX_STRING_LENGTH)
    summary: str = Field(..., max_length=_MAX_STRING_LENGTH)

    @model_validator(mode="after")
    def validate_string_lengths(self) -> "ModelDayClosePlanItem":
        """Validate string lengths to prevent DoS attacks."""
        if len(self.requirement_id) > _MAX_STRING_LENGTH:
            raise ValueError(...)
        if len(self.summary) > _MAX_STRING_LENGTH:
            raise ValueError(...)
        return self

Issue: The @model_validator duplicates validation already provided by Field(..., max_length=...). Pydantic enforces max_length constraints automatically.

Impact:

• Code duplication and maintenance burden
• Potential confusion about which validation applies
• The test in test_security_constraints.py:36-72 even acknowledges this uncertainty with a pytest.skip when Pydantic doesn't enforce max_length

Recommendation:

• Option A (Preferred): Remove the @model_validator and rely on Pydantic's max_length enforcement
• Option B: If custom validation is needed, remove max_length from Field definitions and keep only the validator
• Add a comment explaining why one approach was chosen over the other

Note: This pattern only appears in ModelDayClosePlanItem - other models correctly rely solely on Field(max_length=...).

---

2. Performance: Redundant Regex Compilation (Priority: Low)

Location: model_day_close.py:19,23 and model_ticket_contract.py:21

# model_day_close.py
_SEMVER_PATTERN = re.compile(r"^\d+\.\d+\.\d+$")
_DATE_PATTERN = re.compile(r"^\d{4}-\d{2}-\d{2}$")

# model_ticket_contract.py
_SEMVER_PATTERN = re.compile(r"^\d+\.\d+\.\d+$")

Issue: The SemVer pattern is duplicated across files. While regex compilation is cached, this creates maintenance burden (if pattern needs updating, must change in 2 places).

Impact: Minor - performance is already optimized via module-level compilation, but DRY principle violated.

Recommendation:

• Create a shared validators.py or patterns.py module with:

  # onex_change_control/validation/patterns.py
  import re
  
  SEMVER_PATTERN = re.compile(r"^\d+\.\d+\.\d+$")
  DATE_ISO_PATTERN = re.compile(r"^\d{4}-\d{2}-\d{2}$")

• Import from both model files
• Alternatively, document that duplication is intentional to keep models independent (matches "schema purity" principle)

---

3. SemVer Validation Limitations (Priority: Info)

Location: model_day_close.py:15-18, model_ticket_contract.py:17-20

The inline comments already acknowledge this limitation, which is good:

# Note: This pattern supports basic SemVer (major.minor.patch) only.
# Pre-release versions (e.g., "1.0.0-alpha") and build metadata (e.g., "1.0.0+build")
# are not supported. If full SemVer support is needed, consider using a SemVer library.

Additional Issues:

• Pattern accepts leading zeros (e.g., "01.0.0") which violate SemVer spec
• The test in test_models.py:248-260 acknowledges this limitation

Recommendation:

• If full SemVer compliance is needed, use semantic-version or packaging library:

  from packaging.version import Version
  
  @field_validator("schema_version")
  @classmethod
  def validate_schema_version(cls, v: str) -> str:
      try:
          Version(v)
      except Exception as e:
          raise ValueError(f"Invalid schema_version: {v}") from e
      return v

• If basic pattern is sufficient, update the comment to explicitly list accepted invalid cases (leading zeros)
• Current state is acceptable for M1 if documented

---

4. Test Coverage: Potential False Positive (Priority: Info)

Location: test_security_constraints.py:36-72

def test_excessive_string_length(self) -> None:
    """Test that strings exceeding limit are rejected."""
    too_long_string = "x" * (_MAX_STRING_LENGTH + 1)
    
    try:
        item = ModelDayClosePlanItem(...)
        # If validation passes, skip the test rather than fail
        pytest.skip("max_length constraint not enforced...")
    except (ValidationError, ValueError) as e:
        # Expected behavior
        assert "max length" in str(e).lower() or ...

Issue: This test uses pytest.skip() when validation doesn't work as expected. This could hide actual bugs where Pydantic fails to enforce constraints.

Impact: 97% coverage may be misleading if tests are being skipped.

Recommendation:

• Option A (Preferred): Use pytest.mark.xfail instead of pytest.skip to mark known issues:

  @pytest.mark.xfail(reason="Pydantic max_length enforcement unclear in v2", strict=False)
  def test_excessive_string_length(self) -> None:
      ...
      with pytest.raises(ValidationError):
          ModelDayClosePlanItem(summary=too_long_string)

• Option B: Remove the test entirely if enforcement is uncertain
• Option C: Add a CI check to verify the test doesn't get skipped (fail if skipped)

---

5. Minor: Missing Type Hint for Enum __str__ (Priority: Very Low)

Location: All enum files (e.g., enum_drift_category.py:40)

def __str__(self) -> str:
    """Return the string value for serialization."""
    return self.value

Issue: The __str__ method is implemented but unnecessary since str(Enum) already returns the enum name. Pydantic serializes enums to their .value automatically.

Impact: None - works correctly, but adds unnecessary code.

Recommendation:

• Remove __str__ methods unless there's a specific reason to override default behavior
• If kept for clarity, add a comment explaining why (e.g., "Explicit for documentation purposes")

---

📊 Performance Considerations

1. Regex compilation: Already optimized via module-level compilation ✅
2. Frozen models: No performance penalty, actually enables optimizations (hashability) ✅
3. Validation overhead: Appropriate for schema validation use case ✅
4. List constraints: 1000-item limit prevents pathological cases ✅

Verdict: Performance is well-considered for the use case.

---

🧪 Test Coverage Analysis

Test Suite	Lines	Coverage Focus
test_models.py	360	Structural validation, edge cases, business rules
test_security_constraints.py	147	DoS prevention, length limits
test_serialization.py	300	JSON/YAML round-trip, immutability
test_yaml_parsing.py	75	Real-world YAML parsing
Total	882 lines	97% coverage

Missing Coverage Areas (if any):

• Error messages for all validation failures (could add snapshot tests)
• Enum edge cases (e.g., case-insensitivity) - though Pydantic handles this
• Performance tests for max list size (1000 items) - already covered in test_security_constraints.py:77-96

Verdict: Excellent test coverage with real-world validation.

---

🔐 Security Review

Threat Model

Threat	Mitigation	Status
DoS via large strings	max_length=10000 on all string fields	✅ Mitigated
DoS via large lists	max_length=1000 on all list fields	✅ Mitigated
Injection attacks	Pydantic validation, no dynamic execution	✅ Not vulnerable
Schema poisoning	SemVer validation, frozen models	✅ Mitigated
Type confusion	Strict Pydantic validation	✅ Mitigated

Verdict: Security is well-addressed for a schema validation library.

---

🎯 Acceptance Criteria Validation

From PR description:

Criterion	Status	Evidence
✅ Unit tests cover core structural validation	PASS	12 tests, 97% coverage
✅ Models are schema-pure	PASS	Only Pydantic + local enum imports
✅ Naming conventions aligned	PASS	Enum* and Model* prefixes used
✅ schema_version validated as SemVer	PASS	Regex validation (basic SemVer)

Verdict: All acceptance criteria met.

---

🚀 Overall Verdict

APPROVED with minor recommendations for future improvement.

This PR demonstrates excellent engineering practices:

• Clean, maintainable code with strong type safety
• Comprehensive test coverage with real-world validation
• Thoughtful security constraints
• Clear documentation throughout

The identified issues are minor and don't block merge. They can be addressed in follow-up PRs if desired.

Recommended Next Steps (from PR description):

1. ✅ Merge this PR
2. Proceed with OMN-963 (JSON Schema export script)
3. Proceed with OMN-965 (YAML validation CLI)
4. Consider addressing the redundant validator in ModelDayClosePlanItem in a cleanup PR

---

Great work on M1! 🎉 The schema foundation is solid and ready for downstream consumption.

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (2)

tests/test_security_constraints.py (1)

17-19: Consider importing constants from the source module to avoid drift.

These constants duplicate the values defined in model_day_close.py. If the source values change, these tests could silently become out-of-sync.

🔎 Proposed fix

-# Security constraint limits
-_MAX_STRING_LENGTH = 10000
-_MAX_LIST_ITEMS = 1000
+from onex_change_control.models.model_day_close import (
+    _MAX_STRING_LENGTH,
+    _MAX_LIST_ITEMS,
+)

Note: This requires exporting these constants from model_day_close.py (they're already module-level, so they should be importable). Alternatively, define them in a shared constants module.

src/onex_change_control/models/model_day_close.py (1)

62-72: Redundant validation: max_length is already enforced by Pydantic v2.

The model_validator duplicates the max_length constraint already defined on the Field declarations (lines 56, 59). Pydantic v2 enforces max_length before the model validator runs, so this check will never trigger.

🔎 Proposed fix: Remove redundant validator

 class ModelDayClosePlanItem(BaseModel):
     """Plan item in daily close report."""

     model_config = ConfigDict(frozen=True)

     requirement_id: str = Field(
         ..., description="Requirement identifier", max_length=_MAX_STRING_LENGTH
     )
     summary: str = Field(
         ..., description="Summary of the requirement", max_length=_MAX_STRING_LENGTH
     )
-
-    @model_validator(mode="after")
-    def validate_string_lengths(self) -> "ModelDayClosePlanItem":
-        """Validate string lengths to prevent DoS attacks."""
-        if len(self.requirement_id) > _MAX_STRING_LENGTH:
-            msg = f"requirement_id exceeds max length of {_MAX_STRING_LENGTH}"
-            raise ValueError(msg)
-        if len(self.summary) > _MAX_STRING_LENGTH:
-            msg = f"summary exceeds max length of {_MAX_STRING_LENGTH}"
-            raise ValueError(msg)
-        return self

If you prefer defense-in-depth, keep it but add a comment explaining the intentional redundancy.

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 9b78a3e and cf8489d.

📒 Files selected for processing (5)

• src/onex_change_control/models/model_day_close.py (1 hunks)
• src/onex_change_control/models/model_ticket_contract.py (1 hunks)
• tests/test_models.py (1 hunks)
• tests/test_security_constraints.py (1 hunks)
• tests/test_serialization.py (1 hunks)

🚧 Files skipped from review as they are similar to previous changes (2)

• tests/test_models.py
• src/onex_change_control/models/model_ticket_contract.py

🧰 Additional context used

🧬 Code graph analysis (3)

tests/test_security_constraints.py (3)

src/onex_change_control/enums/enum_invariant_status.py (1)

• EnumInvariantStatus (10-30)

src/onex_change_control/models/model_day_close.py (3)

• ModelDayClose (160-241)
• ModelDayCloseInvariantsChecked (128-144)
• ModelDayClosePlanItem (50-71)

src/onex_change_control/models/model_ticket_contract.py (2)

• ModelEmergencyBypass (40-67)
• ModelTicketContract (70-156)

tests/test_serialization.py (7)

src/onex_change_control/enums/enum_drift_category.py (1)

• EnumDriftCategory (10-42)

src/onex_change_control/enums/enum_evidence_kind.py (1)

• EnumEvidenceKind (10-38)

src/onex_change_control/enums/enum_interface_surface.py (1)

• EnumInterfaceSurface (10-38)

src/onex_change_control/enums/enum_invariant_status.py (1)

• EnumInvariantStatus (10-30)

src/onex_change_control/enums/enum_pr_state.py (1)

• EnumPRState (10-26)

src/onex_change_control/models/model_day_close.py (3)

• ModelDayClose (160-241)
• ModelDayClosePlanItem (50-71)
• ModelDayClosePR (74-86)

src/onex_change_control/models/model_ticket_contract.py (3)

• ModelEmergencyBypass (40-67)
• ModelEvidenceRequirement (24-37)
• ModelTicketContract (70-156)

src/onex_change_control/models/model_day_close.py (4)

src/onex_change_control/enums/enum_drift_category.py (1)

• EnumDriftCategory (10-42)

src/onex_change_control/enums/enum_invariant_status.py (1)

• EnumInvariantStatus (10-30)

src/onex_change_control/enums/enum_pr_state.py (1)

• EnumPRState (10-26)

src/onex_change_control/models/model_ticket_contract.py (1)

• validate_schema_version (125-130)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)

• GitHub Check: claude-review

🔇 Additional comments (20)

tests/test_security_constraints.py (4)

77-96: LGTM!

Good boundary test validating that exactly 1000 items (the limit) are accepted.

---

98-117: LGTM!

Correctly tests that lists exceeding the 1000-item limit are rejected.

---

123-146: LGTM!

Good test for ticket_id length constraint: valid 7-char ID passes, 54-char ID (exceeding 50-char limit) is rejected.

---

36-71: Test approach is sound and appropriately defensive given Pydantic v2 enforcement.

Pydantic v2 enforces max_length on string fields, so the validation will reliably fail. ModelDayClosePlanItem includes both Field(max_length=_MAX_STRING_LENGTH) constraints and a redundant @model_validator(mode="after") that checks the same lengths. The test correctly validates this defense-in-depth approach using try/except with pytest.skip() as a fallback. The fallback documents expected behavior without introducing false failures, making the test a solid defensive pattern.

tests/test_serialization.py (8)

1-26: LGTM!

Imports are well-organized and all appear to be used in the tests.

---

32-63: LGTM!

Solid JSON round-trip test using proper Pydantic v2 serialization methods.

---

65-106: LGTM!

Correct YAML round-trip pattern using model_dump(mode="json") to ensure JSON-serializable output.

---

108-152: LGTM!

Comprehensive enum serialization test verifying both JSON string representation and proper deserialization back to enum values.

---

158-194: LGTM!

Proper JSON round-trip test for ModelTicketContract with nested structures.

---

196-228: LGTM!

Good YAML round-trip test including the enabled emergency bypass scenario with justification and follow-up ticket.

---

230-263: LGTM!

Thorough enum serialization test for ticket contract enums.

---

266-299: LGTM!

Good immutability tests verifying that frozen models raise ValidationError when modification is attempted. The type: ignore comments are appropriate since the type checker would otherwise flag the invalid assignments.

src/onex_change_control/models/model_day_close.py (8)

15-27: LGTM!

Well-documented constants with clear comments explaining the SemVer pattern limitations.

---

30-48: LGTM!

Clean frozen model with appropriate max_length constraints.

---

74-87: LGTM!

Clean model with appropriate constraints including ge=1 for positive PR numbers.

---

89-102: LGTM!

Proper nested structure with list length constraint.

---

104-126: LGTM!

Well-structured drift detection model with consistent constraints.

---

128-145: LGTM!

Clean invariants model with appropriate enum types.

---

147-158: LGTM!

Simple and clean risk model.

---

212-241: LGTM!

Well-implemented validators with clear error messages. The two-step date validation (format check first, then calendar validity) provides better error messages for users.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Missing max_length constraint on corrections_for_tomorrow string items.

Unlike other string fields in this model, individual items in corrections_for_tomorrow don't have a length constraint. A malicious payload could include very long strings within this list, bypassing your DoS protections.

🔎 Proposed fix: Add item-level constraint

You can use Annotated with StringConstraints to limit individual string lengths:

+from typing import Annotated
+from pydantic import StringConstraints
+
+# Type alias for constrained strings
+_ConstrainedStr = Annotated[str, StringConstraints(max_length=_MAX_STRING_LENGTH)]
+
 class ModelDayClose(BaseModel):
     # ...
     corrections_for_tomorrow: list[str] = Field(
-        default_factory=list,
-        description="Actionable corrections for tomorrow",
-        max_length=_MAX_LIST_ITEMS,
+    corrections_for_tomorrow: list[_ConstrainedStr] = Field(
+        default_factory=list,
+        description="Actionable corrections for tomorrow",
+        max_length=_MAX_LIST_ITEMS,
     )

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents

In src/onex_change_control/models/model_day_close.py around lines 201 to 205,
the list field corrections_for_tomorrow has a max_length on the list but no
per-item string length constraint; update the type to an Annotated/Constrained
string (e.g., Annotated[str, Field(max_length=<same_max_as_other_strings>)]) or
use pydantic.constr for items, preserve default_factory=list and the list
max_length, and import Annotated/constr as needed so each list element is
limited to the same per-string max length used elsewhere to prevent oversized
entries.

[claude]

Code Review: M1 Pydantic Schema Models Implementation

Summary

This PR successfully implements Pydantic schema models for Day Close and Ticket Contract artifacts with excellent attention to detail, comprehensive testing, and strong security considerations.

---

Strengths

1. Excellent Code Quality

• Clean Architecture: Schema models are properly isolated with no runtime/infra coupling
• Naming Conventions: Perfectly follows omnibase_core conventions
• Type Safety: Full type annotations with strict mypy configuration
• Immutability: All models use frozen=True for thread-safety
• Documentation: Clear docstrings with detailed explanations

2. Comprehensive Testing (97% coverage, 29 tests)

• Structural validation, edge cases, round-trip serialization
• Security constraints (DoS prevention tests)
• Real-world validation against actual YAML drift reports

3. Security Considerations

• DoS Prevention: _MAX_STRING_LENGTH (10,000) and _MAX_LIST_ITEMS (1,000) constraints
• Input Validation: SemVer format, calendar-valid dates, PR number validation
• Emergency bypass validation (requires justification + follow-up ticket)

4. Performance Optimizations

• Regex patterns compiled at module level
• Efficient validation with two-stage date checking

---

Areas for Improvement (All Minor)

1. SemVer Validation Limitations (Minor)

• Basic pattern accepts leading zeros (01.0.0)
• Already well-documented in code comments
• Priority: Low

2. String Length Enforcement Uncertainty (Low)

• Test uses pytest.skip() fallback for max_length enforcement
• Pydantic v2 does enforce max_length by default
• Recommendation: Test should fail hard instead of skipping
• Priority: Low

3. Test Coverage Gap (Very Low)

• Missing test: emergency bypass disabled with partial fields
• Priority: Very Low

---

Security Review

• DoS Prevention: Max length constraints
• Input Validation: Multi-stage validation
• Immutability: frozen=True prevents mutation
• No Code Injection: No eval(), exec(), or dynamic imports
• Safe Date Parsing: Uses date.fromisoformat()

No Security Concerns Identified

---

Performance Review

• Regex Pre-compilation at module level
• Efficient validation (two-stage date checking)
• Pydantic V2 Rust-based core

No Performance Concerns

---

Test Coverage: 97% (29 tests)

• test_models.py: 14 tests
• test_security_constraints.py: 3 tests
• test_serialization.py: 8 tests
• test_yaml_parsing.py: 4 tests

---

Recommendations Summary

Must Address Before Merge

• None - PR is ready to merge

Should Consider (Non-Blocking)

1. Strengthen SemVer validation to reject leading zeros
2. Verify string max_length enforcement by removing pytest.skip()

Nice-to-Have (Future Work)

1. Add is_complete property to ModelTicketContract
2. Add test for emergency bypass edge case

---

Final Verdict

APPROVED - This is excellent work that meets all acceptance criteria:

• Unit tests cover core structural validation (97% coverage)
• Models are schema-pure (no runtime/infra coupling)
• Naming conventions aligned with omnibase_core
• schema_version validated as SemVer
• Comprehensive security constraints
• Immutable models for thread-safety
• Models validate existing drift reports

Merge Recommendation: APPROVE & MERGE

This PR demonstrates exceptional attention to detail with iterative improvements based on feedback. The minor observations above are suggestions for future enhancement, not blockers.

Great work!

---

Next Steps (Post-Merge)

1. Implement OMN-963: JSON Schema export script
2. Implement OMN-965: YAML validation CLI
3. Consider adding schema evolution documentation

[claude]

Code Review: M1 Pydantic Schema Models Implementation

Overall Assessment

Summary: This is a high-quality implementation with excellent software engineering practices. The PR successfully implements machine-checkable schemas for Day Close and Ticket Contract artifacts with 97% test coverage and strong attention to security, immutability, and validation.

Recommendation: ✅ Approve with minor suggestions

---

Strengths

1. Excellent Code Quality

• Naming conventions: Perfect adherence to omnibase_core conventions (Model*/Enum* prefixes)
• Type safety: Full type annotations with strict mypy configuration
• Immutability: Proper use of frozen=True for all models, ensuring thread-safety and historical integrity
• Documentation: Comprehensive docstrings with clear explanations

2. Security Considerations ⭐

• DoS prevention: Smart use of _MAX_STRING_LENGTH (10,000) and _MAX_LIST_ITEMS (1,000) constraints
• Input validation: All security constraints are properly tested (test_security_constraints.py)
• No injection vulnerabilities: Models are schema-pure with no runtime/infra coupling

3. Validation Logic

• SemVer validation: Regex pattern correctly rejects leading zeros per spec (e.g., 01.0.0 invalid)
• Date validation: Two-stage validation (format check + calendar validity) with excellent error messages
• Business rules: ModelEmergencyBypass enforces that justification and follow_up_ticket_id are required when enabled
• Interface constraints: Properly validates that interface_change=False requires empty interfaces_touched

4. Test Coverage ⭐

• 97% coverage with 12 comprehensive tests
• Tests cover: happy paths, validation errors, edge cases, serialization, security constraints
• YAML parsing validation: Tests successfully parse existing drift reports (2025-12-19.yaml, 2025-12-20.yaml)
• Round-trip testing: JSON and YAML serialization/deserialization verified

5. Schema Purity

• Models only import Pydantic and local enums (no runtime/infra dependencies)
• Clean separation of concerns
• Ready for JSON Schema export (OMN-963)

---

Issues & Suggestions

1. Potential Bug: Whitespace-only strings in emergency bypass

📍 Location: src/onex_change_control/models/model_ticket_contract.py:58-68

Issue: The validator checks if not self.justification: which allows whitespace-only strings to pass.

Recommendation: Use .strip() to reject whitespace-only strings. Test at tests/test_models.py:456-463 currently documents this as should pass, but this seems unintentional based on the business logic.

2. Code Duplication: SemVer Pattern

📍 Locations: model_day_close.py:15-20, model_ticket_contract.py:17-22

Issue: The _SEMVER_PATTERN regex and comments are duplicated across both files.

Recommendation: Extract to a shared validation module for better maintainability.

3. Test Organization Opportunity

📍 Location: tests/test_models.py (464 lines)

Observation: Consider splitting into separate test files as the codebase grows: test_model_day_close.py and test_model_ticket_contract.py. This is optional for now but will help maintainability.

4. Documentation: SemVer Limitations

Suggestion: Consider adding a note in the model docstrings that schema_version only supports basic SemVer (no pre-release/build metadata).

---

Performance Considerations

✅ Excellent:

• Regex patterns compiled at module level (not per-call)
• Immutable models allow safe caching
• No performance concerns identified

---

Best Practices Observations

Excellent Patterns:

1. Frozen models: Ensures historical artifacts cannot be mutated
2. Default factories: Proper use of default_factory=list instead of mutable defaults
3. Validation ordering: Date validation does format check before expensive calendar check
4. Error messages: Clear, actionable error messages with examples
5. Enum str override: Ensures enums serialize to string values

---

Test Coverage Analysis

Covered:

• ✅ Happy paths
• ✅ Validation failures (schema_version, dates, constraints)
• ✅ Edge cases (leading zeros, calendar-invalid dates, empty/full lists)
• ✅ Round-trip serialization (JSON/YAML)
• ✅ Security constraints (DoS prevention)
• ✅ Real-world YAML parsing

---

API Design

Strengths:

• is_complete property: Excellent API design for ModelTicketContract - allows checking completeness without exceptions
• Enum inheritance from str: Enables direct string comparison and serialization
• Clear field descriptions: Every field has a clear description in Field()

The is_complete property is a computed property that does not trigger validation errors. This is intentional and well-documented, allowing work in progress contracts. Good design choice.

---

Security Review

✅ No security concerns identified

• Input validation is comprehensive
• DoS protections are in place and tested
• No code injection vectors
• No sensitive data exposure

---

Alignment with Repository Conventions

✅ Fully aligned with omnibase_core conventions:

• ✅ Naming: Model* / Enum* prefixes
• ✅ Files: model_.py / enum_.py
• ✅ Schema purity: No runtime/infra coupling
• ✅ Type annotations: Full coverage
• ✅ Immutability: Frozen models

---

Final Recommendation

✅ Approve with minor suggestions

This is excellent work that demonstrates strong software engineering fundamentals: comprehensive testing, security awareness, clean architecture, and excellent documentation.

The suggestions above are minor improvements that can be addressed in follow-up PRs if preferred. None are blocking issues.

Estimated merge readiness: Ready to merge after addressing the whitespace validation bug (Issue 1) or documenting it as intentional behavior.

---

Questions for Author

1. Is the whitespace-only string behavior in ModelEmergencyBypass intentional?
2. Would you like to extract the SemVer validation to a shared module now, or defer to a later refactoring?

Great work! 🎉

[coderabbitai]

Actionable comments posted: 0

♻️ Duplicate comments (1)

src/onex_change_control/models/model_day_close.py (1)

191-195: Missing per-item string length constraint (previously flagged).

As noted in a previous review, individual strings in the corrections_for_tomorrow list lack max_length constraints. While the list itself is limited to _MAX_LIST_ITEMS, each string could be arbitrarily long, potentially bypassing DoS protections.

🔎 Proposed fix using Annotated with StringConstraints

+from typing import Annotated
+from pydantic import StringConstraints
+
+# Type alias for constrained strings
+_ConstrainedStr = Annotated[str, StringConstraints(max_length=_MAX_STRING_LENGTH)]
+
 class ModelDayClose(BaseModel):
     # ...
-    corrections_for_tomorrow: list[str] = Field(
+    corrections_for_tomorrow: list[_ConstrainedStr] = Field(
         default_factory=list,
         description="Actionable corrections for tomorrow",
         max_length=_MAX_LIST_ITEMS,
     )

🧹 Nitpick comments (2)

src/onex_change_control/models/model_day_close.py (1)

202-214: Consider extracting shared validator to reduce duplication.

This validate_schema_version method is identical to the one in model_ticket_contract.py (lines 126-138). Consider extracting it to a shared validator function in the validation package.

🔎 Suggested refactoring approach

Add to src/onex_change_control/validation/patterns.py:

def validate_semver(v: str) -> str:
    """Validate that a string matches basic SemVer format.
    
    Note: Only basic SemVer (major.minor.patch) is supported.
    Pre-release versions and build metadata are not supported.
    Leading zeros are rejected per SemVer specification.
    """
    if not SEMVER_PATTERN.match(v):
        msg = f"Invalid schema_version format: {v}. Expected SemVer (e.g., '1.0.0')"
        raise ValueError(msg)
    return v

Then use it in both model files:

from onex_change_control.validation.patterns import validate_semver

class ModelDayClose(BaseModel):
    # ...
    
    @field_validator("schema_version")
    @classmethod
    def validate_schema_version(cls, v: str) -> str:
        """Validate schema_version is SemVer format."""
        return validate_semver(v)

src/onex_change_control/models/model_ticket_contract.py (1)

1-14: LGTM! Clean imports and security constraints.

The module maintains schema purity with appropriate imports.

Optional: Consider centralizing security constants.

The constants _MAX_STRING_LENGTH and _MAX_LIST_ITEMS are duplicated between model_day_close.py and this file. If these constraints should be consistent across all models, consider extracting them to a shared location like validation/patterns.py or a new constants.py module.

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between cf8489d and 84d5ee2.

📒 Files selected for processing (6)

• src/onex_change_control/models/model_day_close.py (1 hunks)
• src/onex_change_control/models/model_ticket_contract.py (1 hunks)
• src/onex_change_control/validation/__init__.py (1 hunks)
• src/onex_change_control/validation/patterns.py (1 hunks)
• tests/test_models.py (1 hunks)
• tests/test_security_constraints.py (1 hunks)

🚧 Files skipped from review as they are similar to previous changes (2)

• tests/test_security_constraints.py
• tests/test_models.py

🧰 Additional context used

🧬 Code graph analysis (2)

src/onex_change_control/models/model_day_close.py (4)

src/onex_change_control/enums/enum_drift_category.py (1)

• EnumDriftCategory (10-42)

src/onex_change_control/enums/enum_invariant_status.py (1)

• EnumInvariantStatus (10-30)

src/onex_change_control/enums/enum_pr_state.py (1)

• EnumPRState (10-26)

src/onex_change_control/models/model_ticket_contract.py (1)

• validate_schema_version (128-138)

src/onex_change_control/models/model_ticket_contract.py (3)

src/onex_change_control/enums/enum_evidence_kind.py (1)

• EnumEvidenceKind (10-38)

src/onex_change_control/enums/enum_interface_surface.py (1)

• EnumInterfaceSurface (10-38)

src/onex_change_control/models/model_day_close.py (1)

• validate_schema_version (204-214)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)

• GitHub Check: claude-review

🔇 Additional comments (10)

src/onex_change_control/validation/patterns.py (1)

1-13: LGTM! Well-documented SemVer pattern.

The regex correctly enforces basic SemVer format (major.minor.patch) with no leading zeros, and the documentation clearly explains the limitations regarding pre-release versions and build metadata.

src/onex_change_control/validation/__init__.py (1)

1-5: LGTM! Clean package initialization.

The re-export pattern correctly exposes SEMVER_PATTERN through the validation package namespace.

src/onex_change_control/models/model_day_close.py (4)

1-22: LGTM! Clean imports and well-defined security constraints.

The module maintains schema purity by importing only Pydantic and local enums, and the security constants provide clear DoS protection.

---

25-142: LGTM! Well-structured nested models with proper constraints.

All nested models consistently enforce immutability via frozen=True, apply appropriate max_length constraints on strings and lists, and include clear field descriptions. The pr field correctly uses ge=1 to prevent invalid PR numbers.

---

144-190: LGTM! Excellent model structure and documentation.

The comprehensive docstring clearly explains the rationale for immutability and SemVer validation. Field definitions are consistent and properly constrained.

---

216-236: LGTM! Excellent two-step date validation.

The validator provides clear error messages by first checking the format with regex, then validating calendar correctness with date.fromisoformat(). The exception chaining preserves error context appropriately.

src/onex_change_control/models/model_ticket_contract.py (4)

17-31: LGTM! Well-structured evidence requirement model.

The model properly uses immutability, applies appropriate constraints, and correctly handles the optional command field.

---

33-64: LGTM! Excellent validation logic for emergency bypass.

The model_validator correctly enforces that when enabled=True, both justification and follow_up_ticket_id must be non-empty and not just whitespace. This prevents incomplete bypass configurations.

---

67-139: LGTM! Comprehensive model with excellent documentation.

The model structure is well-designed with clear field definitions, appropriate constraints, and a comprehensive docstring that explains the validation rules and immutability rationale.

---

140-186: LGTM! Well-designed validation and completeness checking.

The validate_interface_constraints correctly enforces business rules while allowing temporary incomplete states during contract creation. The is_complete property provides a clear API for checking completion status, and the logic correctly accounts for both interface change states.

[claude]

PR Review: M1 - Pydantic Schema Models Implementation

Summary

This PR implements comprehensive Pydantic schema models for drift control artifacts (Day Close reports and Ticket Contracts). The implementation is well-structured, thoroughly tested (97% coverage), and follows the repository's naming conventions. Overall, this is high-quality work ready for merge with only minor suggestions for future consideration.

---

Strengths ✅

1. Excellent Code Quality

• Clean, well-documented Pydantic models with comprehensive docstrings
• Proper use of frozen=True for immutability - great for historical artifacts
• Consistent naming conventions (Model*, Enum*) aligned with omnibase_core
• Type hints are comprehensive and correct

2. Strong Security Posture

• DoS protection via _MAX_STRING_LENGTH (10,000) and _MAX_LIST_ITEMS (1,000)
• Dedicated security constraint tests (test_security_constraints.py)
• Input validation prevents malformed data (SemVer, ISO dates, calendar validation)
• Field validators properly reject edge cases (leading zeros, invalid dates)

3. Comprehensive Test Coverage

• 97% coverage across 4 test files (469 lines in test_models.py alone!)
• Tests cover: structural validation, serialization round-trips, security constraints, YAML parsing
• Edge case testing (e.g., Feb 30th, whitespace-only strings, leading zeros in SemVer)
• Real-world validation using existing 2025-12-20.yaml artifact

4. Well-Designed Validation Logic

• SemVer validation properly rejects leading zeros per SemVer spec
• Date validation has two-stage approach: format check first, then calendar validation
• Emergency bypass logic properly rejects whitespace-only strings via .strip() check

5. Thoughtful Design Decisions

• Interface change handling allows interface_change=True with empty interfaces_touched during creation
• Provides is_complete property to check if contract is in steady state
• Good separation of validation (hard constraints) vs. completeness (business logic)

---

Minor Suggestions 💡

1. Type Annotation Precision

In model_ticket_contract.py:51 and model_ticket_contract.py:141, the validators use forward references (strings) for return types. Consider using from __future__ import annotations for cleaner type hints. This is a very minor style point and the current approach is perfectly valid.

2. Pydantic Field Configuration

Consider adding min_length=1 for fields like ticket_id, drift_id, etc., to prevent empty strings (though Field(...) already requires non-None values).

3. Test Organization

Consider adding a conftest.py with shared fixtures for common objects (e.g., invariants_checked_fixture) to reduce duplication across tests.

---

Potential Issues ⚠️

None Found!

I did not identify any:

• Security vulnerabilities
• Logic errors or bugs
• Performance bottlenecks
• Type safety issues
• Test gaps in critical paths

---

Code-Specific Feedback

model_day_close.py

• Lines 202-214 (SemVer Validator): ✅ Excellent - Clear error message, uses pre-compiled pattern, follows SemVer spec
• Lines 216-236 (Date Validator): ✅ Excellent - Two-stage validation provides clear error messages
• Line 63 (PR Number Constraint): ✅ Good - Prevents invalid PR numbers (0, negative)

model_ticket_contract.py

• Lines 50-64 (Emergency Bypass): ✅ Excellent - Properly rejects whitespace-only strings
• Lines 140-164 (Interface Constraints): ✅ Well-designed - Good separation of concerns
• Lines 166-186 (is_complete): ✅ Clean - Simple boolean logic, well-documented

Enums

All enum files follow a consistent pattern:

• Use str, Enum for string-based enums (good for JSON/YAML serialization)
• Include @unique decorator (prevents duplicate values)
• Comprehensive docstrings
• __str__ method returns .value (good for serialization)

Tests

• test_models.py (469 lines): ✅ Comprehensive coverage of all validation logic
• test_security_constraints.py (132 lines): ✅ Dedicated security testing
• test_serialization.py (299 lines): ✅ JSON and YAML round-trip testing
• test_yaml_parsing.py (74 lines): ✅ Real-world integration test

---

Performance Considerations

1. Regex Compilation: ✅ Both SEMVER_PATTERN and _DATE_PATTERN compiled at module level
2. Immutability: ✅ Using frozen=True enables potential hashability and safe sharing
3. Validation Overhead: Minimal - validators are simple and efficient
4. Memory: Security constraints prevent unbounded growth

No performance concerns identified.

---

Security Assessment

1. DoS Protection: Max lengths prevent memory exhaustion attacks
2. Input Validation: Prevents injection of malformed data
3. Immutability: Prevents accidental/malicious modification of historical data
4. Type Safety: Pydantic validation prevents type confusion bugs

No vulnerabilities found. Security posture is strong for a schema package.

---

Repository Conventions Compliance

Based on README.md:

• ✅ Model naming: Model*
• ✅ Model files: model_*.py
• ✅ Enum naming: Enum*
• ✅ Enum files: enum_*.py
• ✅ Schema purity: No runtime/infra coupling

100% compliance with documented conventions.

---

Final Verdict

APPROVED ✅

This PR represents excellent software engineering with:

• Clean, maintainable code
• Comprehensive test coverage (97%)
• Strong security posture
• Thoughtful design decisions
• Zero critical issues

The implementation successfully delivers on OMN-962's acceptance criteria:

• ✅ Unit tests cover core structural validation
• ✅ Models are schema-pure (no runtime/infra coupling)
• ✅ Naming conventions aligned with omnibase_core
• ✅ schema_version validated as SemVer

Great work! 🎉

---

Checklist

• Code quality and best practices - Excellent
• Potential bugs or issues - None found
• Performance considerations - Optimal
• Security concerns - Well-addressed
• Test coverage - 97%, comprehensive