feat(models): CDQA-08 — add ModelGoldenPath and golden_path to ModelTicketContract [OMN-2980]

[jonahgabriel]

Summary

Implements CDQA-08 (OMN-2980): extends ModelTicketContract with a formal golden_path field for declaring event chain test contracts.

• New file: src/onex_change_control/models/model_golden_path.py — introduces four frozen Pydantic models:
  • ModelGoldenPathAssertion — single field assertion (6 ops: eq, neq, gte, lte, in, contains)
  • ModelGoldenPathInput — input topic + fixture declaration
  • ModelGoldenPathOutput — output topic, optional schema name, assertions
  • ModelGoldenPath — top-level declaration; timeout_ms lives here only (single source of truth, default 30000ms)
• Modified: ModelTicketContract gains golden_path: ModelGoldenPath | None = None after emergency_bypass
• Exports: all new models exported from models/__init__.py and top-level __init__.py
• Template: templates/ticket_contract.template.yaml updated with commented golden_path example
• Tests: tests/unit/test_model_golden_path.py — 44 unit tests covering all spec requirements (126 total passing)
• Markers: pyproject.toml gains markers config for unit, integration, slow

Spec compliance

• All 4 models are frozen (frozen=True)
• timeout_ms at ModelGoldenPath level only (not in input/output)
• schema_name=None is valid (best-effort validation)
• input_correlation_id_field and output_correlation_id_field are independent
• All ops round-trip through model_validate
• ModelTicketContract with and without golden_path both valid
• check-schema-purity passes (no env/fs/network in new models)
• Template updated with commented example

Dependencies

Depends on CDQA-02 (OMN-2976, PR #400) — golden-path-validate skill defines the declaration format.

Test plan

• poetry run pytest tests/unit/test_model_golden_path.py -v — 44 tests pass
• poetry run pytest tests/ -v — 126 tests pass
• poetry run check-schema-purity — 10 files, all pass
• pre-commit run --all-files — all hooks pass

Summary by CodeRabbit

• New Features

  • Added public types to declare optional "golden-path" input→output contract tests and the ability to attach a golden-path declaration to ticket contracts.
  • Added a commented template example for pipeline contract test declarations.

• Tests

  • Added a comprehensive unit test suite covering golden-path declarations, validation, immutability, and round-trip serialization.

• Chores

  • Configured pytest markers (unit, integration, slow) with strict marker handling.

[coderabbitai]

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6874c56 and c361bad.

📒 Files selected for processing (2)

• src/onex_change_control/models/__init__.py
• src/onex_change_control/models/model_golden_path.py

---

📝 Walkthrough

Walkthrough

Adds four frozen Pydantic models to declare “golden path” input→output contract tests, exports them via package APIs, adds an optional golden_path field to ModelTicketContract, updates test config markers, supplies a commented YAML template snippet, and includes comprehensive unit tests.

Changes

Cohort / File(s)	Summary
Golden Path models src/onex_change_control/models/model_golden_path.py	New file: defines frozen Pydantic models ModelGoldenPath, ModelGoldenPathInput, ModelGoldenPathOutput, ModelGoldenPathAssertion (fields, defaults, validation rules, operators, timeout/infra/test_file).
Model exports src/onex_change_control/models/__init__.py, src/onex_change_control/__init__.py	Re-exports the four new models via package __init__ modules and updates __all__ to include them.
Ticket contract integration src/onex_change_control/models/model_ticket_contract.py	Adds optional `golden_path: ModelGoldenPath
YAML template & config templates/ticket_contract.template.yaml, pyproject.toml	Adds a fully commented golden_path example block to the YAML template; pyproject.toml adds pytest marker definitions (unit, integration, slow) and enables strict marker handling.
Tests tests/unit/__init__.py, tests/unit/test_model_golden_path.py	Adds unit test package init and an extensive test suite covering model validation, defaults, immutability, round-trip serialization, edge cases, and ModelTicketContract interactions (many test cases).

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

🐰 I nibble specs and stitch each field so neat,
Four frozen models march in tidy rows,
A golden path of inputs, outputs meet,
Tests hop after where the pipeline goes.
Tiny rabbit cheers — contracts now complete! ✨

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The PR title accurately describes the primary change: adding ModelGoldenPath models and a golden_path field to ModelTicketContract for event chain testing contracts.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

• 📝 Generate docstrings (stacked PR)
• 📝 Generate docstrings (commit on current branch)

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch epic/OMN-2974/OMN-2980/40167c6b

---

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

[claude]

PR Review: CDQA-08 — ModelGoldenPath + golden_path field [OMN-2980]

Overall this is a clean, well-structured addition. The design is sound, purity is maintained, and the test suite is comprehensive. I have a few issues worth addressing before merge, plus some minor observations.

---

Issues

1. Redundant frozen=True specification (consistency violation)

Every new model in model_golden_path.py specifies frozen two ways simultaneously:

class ModelGoldenPathAssertion(BaseModel, frozen=True):   # <-- here
    model_config = ConfigDict(frozen=True)                 # <-- and here

The existing codebase uses only model_config = ConfigDict(frozen=True) (see ModelEvidenceRequirement, ModelEmergencyBypass). In Pydantic v2 these are equivalent — having both is redundant and inconsistent with repo style. Remove the frozen=True from the class signature on all four new models.

2. No upper bound on timeout_ms

timeout_ms: int = Field(
    default=30000,
    ge=1,          # lower bound only
)

A caller can supply timeout_ms=2_147_483_647. The runner presumably enforces its own cap, but having no schema-level ceiling means a contract can be authored with a nonsensical timeout and pass validation. Consider adding le=3_600_000 (1 hour) or whatever the runner's hard limit is, so the schema itself rejects clearly absurd values.

3. infra defaults to "real" — potentially dangerous default

infra: Literal["real", "mock"] = Field(default="real", ...)

Defaulting to live Kafka means any minimally-specified ModelGoldenPath (e.g. in a unit test or CI dry-run) will attempt a real broker connection if the runner honours this flag without an explicit override. "mock" is the safer default. If real is intentional product behaviour, at minimum add a code comment documenting why it is the default.

4. schema_name accepts arbitrary strings with no format validation

schema_name: str | None = Field(default=None, max_length=_MAX_STRING_LENGTH, ...)

schema_name is described as a "Pydantic model class name". Any string is accepted — including "my schema name with spaces", "../../etc/passwd", etc. A simple regex validator (e.g. ^[A-Za-z_][A-Za-z0-9_]*$) would tighten this to valid Python identifiers, matching the field's semantic intent and preventing confusing values.

---

Minor observations (non-blocking)

Test constant naming is unusual and noisy

_EXPECTED_ASSERTIONS_COUNT_2 = 2
_EXPECTED_ASSERTIONS_COUNT_3 = 3
_EXPECTED_ASSERTIONS_COUNT_1 = 1

These constants exist purely to suppress PLR2004 (magic value) lint warnings, but the names are less readable than the literal values themselves. Common alternatives: inline # noqa: PLR2004 where the assertion count is obvious from context, or use descriptive names that communicate why (_SINGLE_ASSERTION, _TWO_ASSERTIONS, _THREE_ASSERTIONS).

Test helper **kwargs: object forces # type: ignore at every call site

def _make_input(**kwargs: object) -> ModelGoldenPathInput:
    ...
    return ModelGoldenPathInput(**defaults)  # type: ignore[arg-type]

Typing kwargs as object is overly restrictive — Any is the conventional choice for this pattern and avoids the suppression comments.

value: Any is intentional but worth a comment

The value field on ModelGoldenPathAssertion is typed Any, which means JSON Schema output for this field will have no type constraint. That is probably correct given the operator set (in, contains, etc.), but a short comment explaining the deliberate choice would prevent future reviewers from flagging it.

---

What's done well

• All four models are correctly frozen per the spec, with full round-trip coverage.
• timeout_ms lives exclusively on ModelGoldenPath (single source of truth — spec-compliant).
• golden_path: ModelGoldenPath | None = None on ModelTicketContract is additive and backward-compatible.
• Schema purity is maintained — no env/fs/network/time in any new model.
• 44 tests cover the full spec surface including all six assertion ops, correlation-ID independence, immutability, and both contract with/without golden path.
• Pytest markers config is a good addition for future test filtering.
• Template comment is clear and well-annotated.

---

Summary: Items 1–3 are straightforward to fix; item 4 is a hardening recommendation. Happy to re-review once those are addressed.

[claude]

PR Review: CDQA-08 - ModelGoldenPath + golden_path on ModelTicketContract

Overall this is a solid, well-scoped addition. The models are pure (D-008 compliant), the schema is backward-compatible, the test coverage is thorough, and the template update is clear. A few items worth addressing before merge:

Issues

1. frozen=True syntax inconsistency

All existing models in the repo use model_config = ConfigDict(frozen=True). The new models switch to the class-keyword form. Both are valid Pydantic v2 syntax, but mixing the two styles inside a single package adds cognitive overhead. Recommend aligning with the existing pattern used throughout model_ticket_contract.py.

2. timeout_ms has no upper bound

Only ge=1 is enforced. A value of sys.maxsize is technically valid per the schema but would make any runner hang indefinitely. Consider adding a reasonable ceiling, e.g. le=300_000 (5 minutes).

3. No cross-field validation between op and value types

value: Any is necessary for flexibility, but operators gte/lte are semantically undefined when value is None, a list, or a boolean. A model_validator(mode=after) could catch the most obviously wrong combinations, or the description should explicitly document that type coherence is delegated to the runner.

4. infra defaults to real (live Kafka)

Defaulting to live infrastructure means accidental omission of the infra key silently targets real Kafka in any environment. Consider whether mock is a safer default, or add a comment documenting why real is intentional.

Minor Nits

5. No min_length=1 on required string fields

topic, fixture, and field currently accept the empty string, which is not a valid Kafka topic name or file path. Adding min_length=1 would catch empty-string inputs at validation time.

6. No validation on schema_name format

schema_name is documented as a Pydantic model class name, but an empty string or a value that is not a valid Python identifier would pass. A simple @field_validator using str.isidentifier() would catch obvious mistakes early.

7. Overly indirected test constants

_EXPECTED_ASSERTIONS_COUNT_1 = 1, _EXPECTED_ASSERTIONS_COUNT_2 = 2, etc. add indirection without clarity. The PLR2004 rule has a common exception for test code.

Positives

• All four models are correctly frozen - immutability enforced end-to-end.
• timeout_ms lives only on ModelGoldenPath, not on input/output - single source of truth per spec.
• schema_name=None default and best-effort validation note in the docstring accurately reflect the runner behavior.
• input_correlation_id_field and output_correlation_id_field are independent with independent defaults - correct.
• Backward-compatible: golden_path: ModelGoldenPath | None = None does not break any existing YAML artifacts.
• 44 tests covering round-trip, immutability, all ops, and edge cases - solid coverage for the surface area added.
• Template example is well-commented and accurately reflects schema defaults.
• Marker definitions in pyproject.toml are required for --strict-markers to pass - correct fix.

The items I would want resolved before merge are #1 (style alignment with existing models), #3 (op/value contract clarification or validation), and #5 (min_length on required string fields). The rest are suggestions.