Repository navigation
feat(OMN-9752): batch-validate all node contracts via model_validate() with CI gate - #1533
Conversation
…) with CI gate Wires ModelNodeTypeProbe.model_validate() over all nodes/*/contract.yaml files as a required CI step. Every contract that fails node_type validation or YAML parsing now blocks merge. - scripts/batch_validate_node_contracts.py: CLI that walks all contract.yaml files and runs ModelNodeTypeProbe.model_validate() on each one, reporting per-contract pass/fail and exiting non-zero on any failure - tests/unit/contracts/test_batch_validate_node_contracts.py: 10 unit tests covering valid/invalid node_type, lowercase aliases, missing field, bad YAML, mixed counts, CLI flags, and a smoke test against all real infra node contracts - .github/workflows/ci.yml: adds "Batch-validate all node contracts" step to the onex-validation job as a blocking CI gate
📝 WalkthroughWalkthroughThis PR introduces a batch validation system for node contracts. It adds a new CLI script ( ChangesNode Contract Validation Feature
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
scripts/batch_validate_node_contracts.py (1)
6-9: ⚡ Quick winDocstring still mentions
load_and_validate_contract_yaml()despite directmodel_validate()flow.Lines 6-9 are stale and can mislead future maintenance/debugging.
Suggested edit
-Walks all nodes/*/contract.yaml files under a given directory and runs -load_and_validate_contract_yaml() (the OMN-9746 choke-point that uses -model_validate()) on each one. Any contract that fails node_type validation, +Walks all nodes/*/contract.yaml files under a given directory and runs +ModelNodeTypeProbe.model_validate() on each one. Any contract that fails node_type validation, YAML parsing, or structure checks causes a non-zero exit.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/batch_validate_node_contracts.py` around lines 6 - 9, Update the stale docstring in the batch_validate_node_contracts module: remove the reference to load_and_validate_contract_yaml() and instead describe the current direct use of model_validate() (or whichever function the script now invokes) and the exact behavior (walking nodes/*/contract.yaml, calling model_validate() on each file, and exiting non-zero on validation/YAML/structure failures); locate the module/function that contains the top-of-file docstring and replace the outdated description with the concise, accurate flow mentioning model_validate() and the failure exit behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/batch_validate_node_contracts.py`:
- Around line 153-156: The code currently only checks
Path(args.directory).exists(); update the validation for the CLI flag so that
when parsing args.directory you explicitly reject non-directory paths: after
creating directory = Path(args.directory) verify directory.exists() and
directory.is_dir(), and if not a directory print an error to stderr (including
the provided path) and return a non-zero exit code (e.g., 2). Locate the check
around the directory variable in scripts/batch_validate_node_contracts.py (the
block that prints "ERROR: Directory not found") and modify it to handle the
"exists but is a file" case by failing loudly instead of continuing.
- Around line 115-116: The except block that currently catches all exceptions
and appends (path, str(exc)) is misclassifying ImportError from
_validate_contract_node_type() as a contract failure; update the handler so it
re-raises ImportError (and other fatal exceptions if desired) instead of
recording them: inside the except Exception as exc around the call to
_validate_contract_node_type(), check isinstance(exc, ImportError) and raise it
(or simply re-raise for ImportError) before appending to failures so ImportError
propagates and triggers the runtime error exit path.
---
Nitpick comments:
In `@scripts/batch_validate_node_contracts.py`:
- Around line 6-9: Update the stale docstring in the
batch_validate_node_contracts module: remove the reference to
load_and_validate_contract_yaml() and instead describe the current direct use of
model_validate() (or whichever function the script now invokes) and the exact
behavior (walking nodes/*/contract.yaml, calling model_validate() on each file,
and exiting non-zero on validation/YAML/structure failures); locate the
module/function that contains the top-of-file docstring and replace the outdated
description with the concise, accurate flow mentioning model_validate() and the
failure exit behavior.
🪄 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: 53d316cb-4d88-4e25-85e5-2fb4baeaaf65
📒 Files selected for processing (3)
.github/workflows/ci.ymlscripts/batch_validate_node_contracts.pytests/unit/contracts/test_batch_validate_node_contracts.py
Evidence
Evidence-Source: OCC#817
Evidence-Ticket: OMN-9752
Summary
scripts/batch_validate_node_contracts.pythat walks allnodes/*/contract.yamlfiles and runsModelNodeTypeProbe.model_validate()on each, using the OMN-9746 choke-point introduced in Task 8node_type, invalidnode_type, bad YAML, mixed pass/fail counts, CLI argument handling, and a smoke test against all 138 real infra node contractsonex-validationCI job — any contract with an invalidnode_typenow blocks mergeDesign decision
Uses
ModelNodeTypeProbe.model_validate()directly rather thanload_and_validate_contract_yaml(). The latter requires ahandler_routingsection which is legitimately absent in many COMPUTE/REDUCER contracts (they use FSM or operation_match patterns). OMN-9752's mandate is specificallynode_typevalidation viamodel_validate().Test plan
uv run pytest tests/unit/contracts/test_batch_validate_node_contracts.py -v— 10/10 passeduv run pytest tests/ -m unit -q— 12548 passed (pre-existingtest_kernel.py::TestIntegrationfailure confirmed on main before this PR)pre-commit run --all-files— all hooks passeduv run mypy scripts/batch_validate_node_contracts.py --ignore-missing-imports— no issuesCloses OMN-9752
Summary by CodeRabbit
Chores
Tests