Repository navigation
feat: add wire schema CI gate test generator [OMN-7371] - #136
Merged
Merged
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 2 minutes and 37 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThis PR introduces two new compliance violation scanners—Model Dump Drift and Wire Schema Mismatch—that validate consistency between Pydantic models and wire schema contracts. It extends the Changes
Sequence Diagram(s)sequenceDiagram
participant Contract as Wire Schema<br/>Contract
participant Generator as Test Generator
participant Discoverer as Contract<br/>Discoverer
participant Importer as Model<br/>Importer
participant Schema as JSON Schema
Generator->>Discoverer: discover_wire_schema_contracts(search_dirs)
Discoverer->>Contract: parse YAML files
Discoverer-->>Generator: list[Contract]
Generator->>Contract: extract contract fields
Generator->>Importer: import consumer model FQN
Importer->>Schema: model_json_schema()
Schema-->>Importer: JSON schema dict
Importer-->>Generator: schema properties
Generator->>Generator: generate baseline cases<br/>(contract_valid,<br/>no_duplicate_fields)
Generator->>Generator: check consumer fields<br/>against contract
Generator->>Generator: check model dump drift
Generator-->>Generator: list[WireSchemaTestCase]
sequenceDiagram
participant Scanner as Wire Schema<br/>Scanner
participant Discoverer as Contract<br/>Discoverer
participant Contract as ModelWireSchemaContract
participant ProducerModel as Producer<br/>Model
participant ConsumerModel as Consumer<br/>Model
participant Checker as Mismatch<br/>Checker
Scanner->>Discoverer: discover_wire_schema_contracts(search_dirs)
Discoverer-->>Scanner: list[(path, contract)]
loop For Each Contract
Scanner->>Contract: extract required/optional fields
Scanner->>ProducerModel: import FQN & extract fields
ProducerModel-->>Scanner: field set
Scanner->>ConsumerModel: import FQN & extract fields
ConsumerModel-->>Scanner: field set
Scanner->>Checker: check_wire_schema_mismatch(contract, ...)
Checker->>Checker: compare required fields<br/>check renamed aliases<br/>validate undeclared fields
Checker-->>Scanner: list[ModelWireSchemaViolation]
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (11)
src/onex_change_control/scanners/model_dump_drift.py (2)
19-25: Remove unused import and emptyTYPE_CHECKINGblock.
EnumWireFieldTypeis imported but never used. TheTYPE_CHECKINGblock contains onlypassand serves no purpose.♻️ Proposed fix
from onex_change_control.models.model_wire_schema_contract import ( - EnumWireFieldType, ModelWireSchemaContract, ) - -if TYPE_CHECKING: - pass🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/onex_change_control/scanners/model_dump_drift.py` around lines 19 - 25, Remove the unused import EnumWireFieldType from the import tuple in model_dump_drift.py and delete the empty TYPE_CHECKING block; update the import statement to only import ModelWireSchemaContract and remove the entire "if TYPE_CHECKING: pass" section so there are no dead imports or no-op type-checking blocks remaining.
41-44: Consider extracting_PYDANTIC_INTERNALto a shared constant.This exact set is duplicated in
src/onex_change_control/scanners/wire_schema_compliance.py(line 204). Consider defining it once in a shared location to maintain consistency if the set of internal fields changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/onex_change_control/scanners/model_dump_drift.py` around lines 41 - 44, The _PYDANTIC_INTERNAL frozenset is duplicated; extract it into a single shared constant (e.g., PYDANTIC_INTERNAL_FIELDS) in a common module and replace the local definitions in model_dump_drift.py and wire_schema_compliance.py with imports of that symbol; update any references to use the new constant name, run tests/import checks, and ensure you export the constant from the shared module so both modules (previously declaring _PYDANTIC_INTERNAL) import the single source of truth.tests/unit/models/test_model_wire_schema_contract.py (1)
148-170: Consider usingpytest.raises(ValidationError)instead of broadException.Catching
Exceptionis overly broad and could mask unexpected errors. Sinceload_wire_schema_contractuses Pydantic'smodel_validate(), missing required fields raisepydantic.ValidationError.♻️ Proposed fix
+from pydantic import ValidationError + class TestModelWireSchemaContractInvalid: """Invalid contract rejection.""" def test_missing_required_fields_section(self) -> None: data = _minimal_contract() del data["required_fields"] - with pytest.raises(Exception): + with pytest.raises(ValidationError): load_wire_schema_contract(data) def test_missing_topic(self) -> None: data = _minimal_contract() del data["topic"] - with pytest.raises(Exception): + with pytest.raises(ValidationError): load_wire_schema_contract(data) def test_missing_producer(self) -> None: data = _minimal_contract() del data["producer"] - with pytest.raises(Exception): + with pytest.raises(ValidationError): load_wire_schema_contract(data) def test_missing_consumer(self) -> None: data = _minimal_contract() del data["consumer"] - with pytest.raises(Exception): + with pytest.raises(ValidationError): load_wire_schema_contract(data)Also update line 210:
def test_invalid_field_type(self) -> None: data = _minimal_contract( required_fields=[ {"name": "id", "type": "invalid_type"}, ] ) - with pytest.raises(Exception): + with pytest.raises(ValidationError): load_wire_schema_contract(data)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/models/test_model_wire_schema_contract.py` around lines 148 - 170, Replace the broad exception assertions in the tests so they specifically expect Pydantic validation failures: change the pytest.raises(Exception) calls to pytest.raises(pydantic.ValidationError) in the test_missing_required_fields_section, test_missing_topic, test_missing_producer, and test_missing_consumer tests that call load_wire_schema_contract; also scan the same test file for other uses of pytest.raises(Exception) (e.g., the other assertion noted around the previous change) and update them to pytest.raises(pydantic.ValidationError) so tests assert the correct error type from model_validate().tests/unit/scanners/test_wire_schema_compliance.py (1)
13-23: Remove unused importModelWireSchemaViolation.Static analysis correctly identifies that
ModelWireSchemaViolationis imported but never used. The tests access violation attributes directly without type annotations. Note thatpytestis actually used implicitly via thetmp_pathfixture.🧹 Proposed fix
import pytest import yaml from onex_change_control.models.model_wire_schema_contract import ( load_wire_schema_contract, ) from onex_change_control.scanners.wire_schema_compliance import ( - ModelWireSchemaViolation, check_wire_schema_mismatch, discover_wire_schema_contracts, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/scanners/test_wire_schema_compliance.py` around lines 13 - 23, Remove the unused import ModelWireSchemaViolation from the import list in the test module; update the import statement that currently brings in ModelWireSchemaViolation, check_wire_schema_mismatch, and discover_wire_schema_contracts so it only imports check_wire_schema_mismatch and discover_wire_schema_contracts, and run tests to ensure no references to ModelWireSchemaViolation remain (tests already access violation attributes directly).src/onex_change_control/models/model_wire_schema_contract.py (1)
76-88: Consider using an enum forshim_status.The
shim_statusfield accepts any string but per the docstring should only be"active"or"retired". Using aLiteral["active", "retired"]or a dedicated enum would provide compile-time validation.🧹 Proposed improvement
+from typing import Literal + class ModelWireRenamedField(BaseModel): """A renamed field tracking an active or retired shim.""" model_config = ConfigDict(frozen=True, extra="forbid") producer_name: str = Field(..., description="Name emitted by the producer") canonical_name: str = Field(..., description="Canonical name in the contract") - shim_status: str = Field( - ..., description="active or retired" - ) + shim_status: Literal["active", "retired"] = Field( + ..., description="Shim status" + ) retirement_ticket: str = Field( default="", description="Ticket tracking shim retirement" )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/onex_change_control/models/model_wire_schema_contract.py` around lines 76 - 88, The shim_status field on ModelWireRenamedField currently accepts any string; restrict it to the allowed values by changing its type to a Literal["active", "retired"] or an Enum (e.g., ShimStatus with ACTIVE/RETIRED) and update the Field declaration accordingly; import typing.Literal (or the Enum class) and ensure model_config still enforces validation so only "active" or "retired" are accepted at runtime.src/onex_change_control/scanners/wire_schema_compliance.py (1)
202-218: Move_PYDANTIC_INTERNALto module-level constant.The set
_PYDANTIC_INTERNALis recreated on every call to_check_side. Moving it to module level avoids repeated allocation and makes it easier to maintain.♻️ Proposed fix
At module level (e.g., after line 30):
# Internal Pydantic fields to exclude from contract validation _PYDANTIC_INTERNAL = frozenset({"model_config", "model_fields", "model_computed_fields"})Then in
_check_side:# Check: model field not in contract (undeclared emission/consumption) - # Exclude internal Pydantic fields - _PYDANTIC_INTERNAL = {"model_config", "model_fields", "model_computed_fields"} for field_name in sorted(model_fields - _PYDANTIC_INTERNAL):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/onex_change_control/scanners/wire_schema_compliance.py` around lines 202 - 218, The _PYDANTIC_INTERNAL set is being recreated each call inside _check_side; move it to a module-level constant to avoid repeated allocation and centralize maintenance by defining _PYDANTIC_INTERNAL = frozenset({"model_config","model_fields","model_computed_fields"}) at module scope (near other constants) and then remove the local definition inside _check_side so the loop that iterates model_fields - _PYDANTIC_INTERNAL uses the module constant; ensure references to _PYDANTIC_INTERNAL and behavior in the loop (including the producer rename_producer_names logic and appended ModelWireSchemaViolation) remain unchanged.src/onex_change_control/testing/wire_schema_test_generator.py (4)
62-69: Consider logging import failures for debugging.The bare
except Exceptionsilently swallows all errors. While graceful degradation is appropriate for non-importable models, logging at DEBUG level would aid troubleshooting when models fail to resolve unexpectedly.🔧 Proposed improvement
def _resolve_model_json_schema(module_path: str, class_name: str) -> dict[str, Any] | None: """Import a model and return its JSON schema, or None if not resolvable.""" try: mod = importlib.import_module(module_path) cls = getattr(mod, class_name) return cls.model_json_schema() - except Exception: + except Exception as e: + logger.debug("Could not resolve model %s.%s: %s", module_path, class_name, e) return None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/onex_change_control/testing/wire_schema_test_generator.py` around lines 62 - 69, The helper _resolve_model_json_schema currently swallows all import errors; update it to log failures at DEBUG so unexpected resolution problems are visible during troubleshooting: catch Exception as e, obtain a logger (e.g., logging.getLogger(__name__) or a module-level logger), and call logger.debug with a clear message including module_path, class_name and the exception information (or use logger.exception/stack_info if available) before returning None; ensure _resolve_model_json_schema remains returning None on failure.
23-28: Remove unused importsyamlandload_wire_schema_contract.Static analysis correctly identifies these as unused. The module uses
discover_wire_schema_contractswhich handles YAML loading internally.🧹 Proposed fix
-import yaml - from onex_change_control.models.model_wire_schema_contract import ( ModelWireSchemaContract, - load_wire_schema_contract, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/onex_change_control/testing/wire_schema_test_generator.py` around lines 23 - 28, Remove the unused imports from the top of the module: delete the import of yaml and the import of load_wire_schema_contract from ModelWireSchemaContract's import block; keep only ModelWireSchemaContract (or other actually used symbols) since the code uses discover_wire_schema_contracts which handles YAML loading internally.
72-85: Remove unused parametermodel_name.The
model_nameparameter is never used in the function body. If it was intended for future use, consider removing it until needed to avoid confusion.🧹 Proposed fix
-def _infer_module_from_file(file_path: str, model_name: str) -> str | None: +def _infer_module_from_file(file_path: str) -> str | None: """Infer a Python module path from a contract's file path. E.g., "src/omnibase_infra/models/model_foo.py" -> "omnibase_infra.models.model_foo" """And update the call site at line 137-138:
consumer_module = _infer_module_from_file( - contract.consumer.file, contract.consumer.model + contract.consumer.file )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/onex_change_control/testing/wire_schema_test_generator.py` around lines 72 - 85, The function _infer_module_from_file has an unused parameter model_name; remove that parameter from its signature and any internal references so the function becomes _infer_module_from_file(file_path: str) -> str | None, and then update all call sites that currently pass a model_name (calls to _infer_module_from_file) to only pass the file_path argument. Ensure imports/type hints and any tests referencing the old signature are updated accordingly.
195-215: Add type parameter tosearch_dirsfor clarity.The
search_dirsparameter is typed as barelistwithout a type parameter. For consistency withdiscover_wire_schema_contractsand better IDE support, specify the element type.🧹 Proposed fix
+from pathlib import Path + def generate_all_test_cases( - search_dirs: list, + search_dirs: list[Path], ) -> list[WireSchemaTestCase]:Similarly for
pytest_params_from_contracts:def pytest_params_from_contracts( - search_dirs: list, + search_dirs: list[Path], ) -> list[tuple[str, str, bool, str]]:Note: Move the
Pathimport to the top-level imports and remove the in-function import on line 206.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/onex_change_control/testing/wire_schema_test_generator.py` around lines 195 - 215, The parameter search_dirs in generate_all_test_cases is typed as a bare list; update its annotation to list[Path | str] (or list[Path] if you only accept Paths) to match discover_wire_schema_contracts and improve IDE support, and do the same for pytest_params_from_contracts if it also uses a bare list; also move the Path import out of the function into the module-level imports and remove the in-function import to avoid repeated imports. Use the function name generate_all_test_cases, the parameter search_dirs, and reference discover_wire_schema_contracts and pytest_params_from_contracts when making these edits.tests/unit/testing/test_wire_schema_test_generator.py (1)
13-18: Remove unused importWireSchemaTestCase.Static analysis correctly identifies that
WireSchemaTestCaseis imported but not directly referenced in type annotations or assertions. The tests work with the returned instances via attribute access.🧹 Proposed fix
from onex_change_control.testing.wire_schema_test_generator import ( - WireSchemaTestCase, generate_all_test_cases, generate_test_cases_for_contract, pytest_params_from_contracts, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/testing/test_wire_schema_test_generator.py` around lines 13 - 18, The import list includes an unused symbol WireSchemaTestCase; remove WireSchemaTestCase from the from-import tuple in test_wire_schema_test_generator.py so only used symbols (generate_all_test_cases, generate_test_cases_for_contract, pytest_params_from_contracts) are imported, ensuring the import statement remains syntactically correct and tests continue to use returned instances via attribute access.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/unit/models/test_model_wire_schema_contract.py`:
- Around line 223-250: The test test_parse_routing_decision_contract uses a
hardcoded local path in yaml_path which will not exist in CI; instead update the
test to load the routing_decision_v1.yaml from a project-relative test fixture
or resource (e.g., place the YAML into the tests/fixtures or tests/data
directory) and open it using a relative path or a pytest fixture/env var to
locate the file, then call load_wire_schema_contract(data) unchanged and assert
on contract as before; ensure the test fails fast if the fixture is missing (do
not silently pytest.skip) so CI verifies the contract parsing behavior.
In `@tests/unit/testing/test_wire_schema_test_generator.py`:
- Around line 137-155: The test
TestRoutingDecisionV1Compatibility::test_generates_passing_tests_for_routing_decision
contains a hardcoded developer path; replace this with a configurable approach:
remove the absolute Path and instead read a path from an environment variable or
pytest fixture (e.g., use an env var like OMNIBASE_INFRA_PATH or a fixture that
yields a Path), fall back to pytest.skip when not provided, and ensure you still
call generate_all_test_cases([path]) and filter routing_cases as before;
alternatively delete the test and rely on integration tests if you prefer not to
support an optional local dependency.
---
Nitpick comments:
In `@src/onex_change_control/models/model_wire_schema_contract.py`:
- Around line 76-88: The shim_status field on ModelWireRenamedField currently
accepts any string; restrict it to the allowed values by changing its type to a
Literal["active", "retired"] or an Enum (e.g., ShimStatus with ACTIVE/RETIRED)
and update the Field declaration accordingly; import typing.Literal (or the Enum
class) and ensure model_config still enforces validation so only "active" or
"retired" are accepted at runtime.
In `@src/onex_change_control/scanners/model_dump_drift.py`:
- Around line 19-25: Remove the unused import EnumWireFieldType from the import
tuple in model_dump_drift.py and delete the empty TYPE_CHECKING block; update
the import statement to only import ModelWireSchemaContract and remove the
entire "if TYPE_CHECKING: pass" section so there are no dead imports or no-op
type-checking blocks remaining.
- Around line 41-44: The _PYDANTIC_INTERNAL frozenset is duplicated; extract it
into a single shared constant (e.g., PYDANTIC_INTERNAL_FIELDS) in a common
module and replace the local definitions in model_dump_drift.py and
wire_schema_compliance.py with imports of that symbol; update any references to
use the new constant name, run tests/import checks, and ensure you export the
constant from the shared module so both modules (previously declaring
_PYDANTIC_INTERNAL) import the single source of truth.
In `@src/onex_change_control/scanners/wire_schema_compliance.py`:
- Around line 202-218: The _PYDANTIC_INTERNAL set is being recreated each call
inside _check_side; move it to a module-level constant to avoid repeated
allocation and centralize maintenance by defining _PYDANTIC_INTERNAL =
frozenset({"model_config","model_fields","model_computed_fields"}) at module
scope (near other constants) and then remove the local definition inside
_check_side so the loop that iterates model_fields - _PYDANTIC_INTERNAL uses the
module constant; ensure references to _PYDANTIC_INTERNAL and behavior in the
loop (including the producer rename_producer_names logic and appended
ModelWireSchemaViolation) remain unchanged.
In `@src/onex_change_control/testing/wire_schema_test_generator.py`:
- Around line 62-69: The helper _resolve_model_json_schema currently swallows
all import errors; update it to log failures at DEBUG so unexpected resolution
problems are visible during troubleshooting: catch Exception as e, obtain a
logger (e.g., logging.getLogger(__name__) or a module-level logger), and call
logger.debug with a clear message including module_path, class_name and the
exception information (or use logger.exception/stack_info if available) before
returning None; ensure _resolve_model_json_schema remains returning None on
failure.
- Around line 23-28: Remove the unused imports from the top of the module:
delete the import of yaml and the import of load_wire_schema_contract from
ModelWireSchemaContract's import block; keep only ModelWireSchemaContract (or
other actually used symbols) since the code uses discover_wire_schema_contracts
which handles YAML loading internally.
- Around line 72-85: The function _infer_module_from_file has an unused
parameter model_name; remove that parameter from its signature and any internal
references so the function becomes _infer_module_from_file(file_path: str) ->
str | None, and then update all call sites that currently pass a model_name
(calls to _infer_module_from_file) to only pass the file_path argument. Ensure
imports/type hints and any tests referencing the old signature are updated
accordingly.
- Around line 195-215: The parameter search_dirs in generate_all_test_cases is
typed as a bare list; update its annotation to list[Path | str] (or list[Path]
if you only accept Paths) to match discover_wire_schema_contracts and improve
IDE support, and do the same for pytest_params_from_contracts if it also uses a
bare list; also move the Path import out of the function into the module-level
imports and remove the in-function import to avoid repeated imports. Use the
function name generate_all_test_cases, the parameter search_dirs, and reference
discover_wire_schema_contracts and pytest_params_from_contracts when making
these edits.
In `@tests/unit/models/test_model_wire_schema_contract.py`:
- Around line 148-170: Replace the broad exception assertions in the tests so
they specifically expect Pydantic validation failures: change the
pytest.raises(Exception) calls to pytest.raises(pydantic.ValidationError) in the
test_missing_required_fields_section, test_missing_topic, test_missing_producer,
and test_missing_consumer tests that call load_wire_schema_contract; also scan
the same test file for other uses of pytest.raises(Exception) (e.g., the other
assertion noted around the previous change) and update them to
pytest.raises(pydantic.ValidationError) so tests assert the correct error type
from model_validate().
In `@tests/unit/scanners/test_wire_schema_compliance.py`:
- Around line 13-23: Remove the unused import ModelWireSchemaViolation from the
import list in the test module; update the import statement that currently
brings in ModelWireSchemaViolation, check_wire_schema_mismatch, and
discover_wire_schema_contracts so it only imports check_wire_schema_mismatch and
discover_wire_schema_contracts, and run tests to ensure no references to
ModelWireSchemaViolation remain (tests already access violation attributes
directly).
In `@tests/unit/testing/test_wire_schema_test_generator.py`:
- Around line 13-18: The import list includes an unused symbol
WireSchemaTestCase; remove WireSchemaTestCase from the from-import tuple in
test_wire_schema_test_generator.py so only used symbols
(generate_all_test_cases, generate_test_cases_for_contract,
pytest_params_from_contracts) are imported, ensuring the import statement
remains syntactically correct and tests continue to use returned instances via
attribute access.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d3a376e7-b650-4f9c-9875-d50bbfa2ea22
📒 Files selected for processing (14)
docs/wire-schema-contract-spec.mdsrc/onex_change_control/enums/enum_compliance_violation.pysrc/onex_change_control/models/__init__.pysrc/onex_change_control/models/model_wire_schema_contract.pysrc/onex_change_control/scanners/model_dump_drift.pysrc/onex_change_control/scanners/wire_schema_compliance.pysrc/onex_change_control/testing/__init__.pysrc/onex_change_control/testing/wire_schema_test_generator.pytests/unit/models/test_model_wire_schema_contract.pytests/unit/scanners/test_model_dump_drift.pytests/unit/scanners/test_wire_schema_compliance.pytests/unit/test_compliance_models.pytests/unit/testing/__init__.pytests/unit/testing/test_wire_schema_test_generator.py
jonahgabriel
enabled auto-merge
April 2, 2026 21:53
jonahgabriel
force-pushed
the
jonah/omn-7371-ci-gate-generator
branch
from
April 2, 2026 21:59
977df53 to
c86de13
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/onex_change_control/scanners/model_dump_drift.py (1)
40-42: Consider addingmodel_extrato internal fields list.The
_PYDANTIC_INTERNALset excludes common Pydantic internals, butmodel_extramay also appear in JSON schemas whenextra="allow"is configured. This is a minor edge case since the wire schema contracts useextra="forbid"orextra="ignore".♻️ Optional: expand internal fields list
_PYDANTIC_INTERNAL: frozenset[str] = frozenset( - {"model_config", "model_fields", "model_computed_fields"} + {"model_config", "model_fields", "model_computed_fields", "model_extra"} )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/onex_change_control/scanners/model_dump_drift.py` around lines 40 - 42, The _PYDANTIC_INTERNAL frozenset currently lists "model_config", "model_fields", and "model_computed_fields" but should also include "model_extra" to cover cases where Pydantic emits that key (e.g., when extra="allow"); update the _PYDANTIC_INTERNAL definition to add "model_extra" so functions that filter or compare JSON schemas (referencing _PYDANTIC_INTERNAL) will treat it as an internal field.src/onex_change_control/testing/wire_schema_test_generator.py (1)
211-213: Defensive type coercion is acceptable but consider tightening the type.Line 212 converts elements to
Patheven thoughsearch_dirsis typed aslist[Path]. While this defensive approach handles misuse, it may mask type errors. Consider either removing the conversion or updating the type hint tolist[Path | str]for clarity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/onex_change_control/testing/wire_schema_test_generator.py` around lines 211 - 213, The code coerces elements of search_dirs into Path objects before calling discover_wire_schema_contracts, but search_dirs is annotated as list[Path]; to fix, either remove the defensive conversion and pass search_dirs directly to discover_wire_schema_contracts (if search_dirs should always be Path), or update the type annotation on search_dirs to list[Path | str] and keep the conversion to Path for safety; update the function signature or its callers accordingly and ensure references to discover_wire_schema_contracts, search_dirs, and Path are adjusted to match the chosen approach.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/onex_change_control/testing/wire_schema_test_generator.py`:
- Around line 184-196: When the consumer model isn't importable the code
currently appends only a WireSchemaTestCase with check_name
"consumer_fields_match" and skips adding "consumer_model_drift", causing
inconsistent counts; update the else branch that constructs the
WireSchemaTestCase (look for WireSchemaTestCase usage with contract_path,
contract and the message referencing contract.consumer.model) to also append a
second WireSchemaTestCase for check_name "consumer_model_drift" with passed=True
and details indicating the same skipped reason (e.g. "Skipped: consumer model
{contract.consumer.model} not importable"), preserving the same
contract_path/contract context.
In `@tests/unit/testing/test_wire_schema_test_generator.py`:
- Around line 23-24: Update _make_contract to use typed generics: import Any and
Dict from typing, change the signature to def _make_contract(**overrides: Any)
-> Dict[str, Any], and change the local base variable annotation from base: dict
= to base: Dict[str, Any] = so no bare dict is used; also update any other
occurrences in that function that use unparameterized dict to use Dict[str,
Any].
---
Nitpick comments:
In `@src/onex_change_control/scanners/model_dump_drift.py`:
- Around line 40-42: The _PYDANTIC_INTERNAL frozenset currently lists
"model_config", "model_fields", and "model_computed_fields" but should also
include "model_extra" to cover cases where Pydantic emits that key (e.g., when
extra="allow"); update the _PYDANTIC_INTERNAL definition to add "model_extra" so
functions that filter or compare JSON schemas (referencing _PYDANTIC_INTERNAL)
will treat it as an internal field.
In `@src/onex_change_control/testing/wire_schema_test_generator.py`:
- Around line 211-213: The code coerces elements of search_dirs into Path
objects before calling discover_wire_schema_contracts, but search_dirs is
annotated as list[Path]; to fix, either remove the defensive conversion and pass
search_dirs directly to discover_wire_schema_contracts (if search_dirs should
always be Path), or update the type annotation on search_dirs to list[Path |
str] and keep the conversion to Path for safety; update the function signature
or its callers accordingly and ensure references to
discover_wire_schema_contracts, search_dirs, and Path are adjusted to match the
chosen approach.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0c67e3ac-38cc-4ab8-a528-854268f7894f
📒 Files selected for processing (13)
src/onex_change_control/enums/enum_compliance_violation.pysrc/onex_change_control/models/model_wire_schema_contract.pysrc/onex_change_control/scanners/model_dump_drift.pysrc/onex_change_control/scanners/wire_schema_compliance.pysrc/onex_change_control/testing/__init__.pysrc/onex_change_control/testing/wire_schema_test_generator.pytests/unit/models/test_model_wire_schema_contract.pytests/unit/scanners/__init__.pytests/unit/scanners/test_model_dump_drift.pytests/unit/scanners/test_wire_schema_compliance.pytests/unit/test_compliance_models.pytests/unit/testing/__init__.pytests/unit/testing/test_wire_schema_test_generator.py
✅ Files skipped from review due to trivial changes (5)
- tests/unit/scanners/init.py
- src/onex_change_control/testing/init.py
- src/onex_change_control/enums/enum_compliance_violation.py
- tests/unit/scanners/test_model_dump_drift.py
- tests/unit/scanners/test_wire_schema_compliance.py
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/unit/test_compliance_models.py
- src/onex_change_control/scanners/wire_schema_compliance.py
Create wire_schema_test_generator.py that discovers wire schema contract YAMLs and generates parametrized pytest test cases. Each contract produces checks for: contract validity, duplicate fields, consumer field matching (Check 5), and model dump drift (Check 6). Models that aren't importable in the test env are gracefully skipped. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…or [OMN-7371] - Add tests/unit/scanners/__init__.py (INP001 namespace package fix) - Remove unused imports: EnumWireFieldType, yaml, load_wire_schema_contract, ModelWireSchemaContract, WireSchemaTestCase, pytest, ModelDumpDriftViolation, ModelWireSchemaViolation (F401 across scanner/test files) - Replace blind except Exception with specific exceptions (BLE001) - Fix __slots__ ordering to natural sort order (RUF023) - Move ModelWireSchemaContract to TYPE_CHECKING block (TC001) - Remove unused model_name param from _infer_module_from_file (ARG001) - Move Path import to top level, remove in-function import (PLC0415) - Make passed a keyword-only argument in WireSchemaTestCase (FBT001) - Fix E501 line length violations - shim_status constrained to Literal["active", "retired"] - Handle multiple active producer aliases per canonical field in _check_side - scan_wire_schema_compliance derives FQNs from contract file paths - Fix hardcoded /Users/jonah path in tests: try multiple candidate dirs - Fix potentially uninitialized data variable in routing_decision test Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ype params [OMN-7371] - Add SPDX header to tests/unit/testing/__init__.py (pre-commit SPDX gate) - Add dict[str, Any] type parameters to _make_contract in test generator (mypy type-arg requirement) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
jonahgabriel
force-pushed
the
jonah/omn-7371-ci-gate-generator
branch
from
April 2, 2026 22:38
30856fe to
845a599
Compare
… [OMN-7371] - Replace hardcoded developer paths with OMNI_HOME env var + fallback candidates - Add missing consumer_model_drift skip case when model not importable - Addresses CodeRabbit Major + Minor review comments
jonahgabriel
added this pull request to the merge queue
Apr 2, 2026
2 tasks done
jonahgabriel
added a commit
that referenced
this pull request
May 26, 2026
…elf-extending-agent fixes (#1689) Adds OCC contracts and receipt evidence for two onex-self-extending-agent tickets: - OMN-11827: max_tokens truncation fix (PRs #136 merged, #138 open) - OMN-12124: clean ADK auth error handling (PR #137 open) These contracts unblock the receipt gate on those PRs — both reference OCC#<this PR>.
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
2 participants
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
wire_schema_test_generator.pythat discovers wire schema contract YAMLs and generates parametrized pytest test casespytest_params_from_contracts()for easy@pytest.mark.parametrizeintegrationrouting_decision_v1.yaml— generates passing testsTest plan
test_wire_schema_test_generator.py)routing_decision_v1.yamlDepends on: #134, #135
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Improvements