diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index dac9a7f2c11..c995b39e6f9 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -65,10 +65,12 @@ repos: pass_filenames: true # Only check changed files, not entire codebase stages: [pre-push] # Moved to pre-push: ~1-5s for changed files only - id: validate-string-versions - name: Reject hardcoded __version__ in __init__.py + name: Reject string version anti-patterns (Python + YAML) language: system entry: uv run python scripts/validation/validate_string_versions.py - types: [python] + types_or: [python, yaml] + exclude: >- + ^(\.github/|contracts/(?!OMN-)|drift/|eval_suites/|schemas/|standards/|templates/|src/onex_change_control/nodes/.*/contract\.yaml$) pass_filenames: true stages: [pre-commit] - id: check-ai-slop diff --git a/scripts/validation/validate_string_versions.py b/scripts/validation/validate_string_versions.py old mode 100644 new mode 100755 index d37a64c26da..3849adaa5db --- a/scripts/validation/validate_string_versions.py +++ b/scripts/validation/validate_string_versions.py @@ -2,11 +2,21 @@ # SPDX-FileCopyrightText: 2025 OmniNode.ai Inc. # SPDX-License-Identifier: MIT -"""Validate that __init__.py files do not contain hardcoded __version__ strings. - -Versions should come from pyproject.toml via importlib.metadata, never from -hardcoded string literals in source code. This hook prevents reintroduction -of the anti-pattern after OMN-3831 removed existing instances. +"""Validate version fields across Python and YAML files. + +Checks: +1. Python __init__.py files must not contain hardcoded __version__ strings. +2. YAML files must not use string-shaped version fields + (e.g. ``schema_version: "1.0.0"``). + +POLICY — Ticket contract exemption (OMN-9593): + Files matching ``contracts/OMN-*.yaml`` are governance artifacts validated + by ``ModelTicketContract.schema_version`` (a ``str`` field checked against + ``SEMVER_PATTERN``). Their ``schema_version`` is intentionally string-shaped + because ticket contracts are authored by humans in YAML, consumed by the + validate-yaml CLI, and never deserialized into a ModelSemVer at runtime. + The field_validator on ``ModelTicketContract`` provides equivalent SemVer + enforcement, so the generic string-version ban does not apply. Adapted from omnibase_core/scripts/validation/validate-string-versions.py. """ @@ -14,9 +24,33 @@ from __future__ import annotations import ast +import re import sys from pathlib import Path +_SEMVER_RE = re.compile( + r"""^(?P['"]?)(0|[1-9]\d*)\.(0|[1-9]\d*)\.(0|[1-9]\d*)(?P=quote)$""" +) + +# Governance artifacts whose schema_version is intentionally string-shaped +# and validated by ModelTicketContract.field_validator instead. +TICKET_CONTRACT_GLOB = re.compile(r"^OMN-\d+\.yaml$") +_MIN_TICKET_CONTRACT_PARTS = 2 + + +def _is_ticket_contract(path: Path) -> bool: + parts = path.as_posix().replace("\\", "/").split("/") + return ( + len(parts) >= _MIN_TICKET_CONTRACT_PARTS + and parts[-2] == "contracts" + and bool(TICKET_CONTRACT_GLOB.fullmatch(parts[-1])) + ) + + +# --------------------------------------------------------------------------- +# Python __init__.py checks +# --------------------------------------------------------------------------- + def _is_inside_except_handler(node: ast.AST, tree: ast.Module) -> bool: """Return True if *node* is inside an ``except`` handler (fallback pattern).""" @@ -44,33 +78,99 @@ def _has_hardcoded_version(path: Path) -> list[tuple[int, str]]: for node in ast.walk(tree): if isinstance(node, ast.Assign): for target in node.targets: - if isinstance(target, ast.Name) and target.id == "__version__": - if isinstance(node.value, (ast.Constant, ast.JoinedStr)): - if _is_inside_except_handler(node, tree): - continue - lineno = node.lineno - lines = source.splitlines() - src_line = lines[lineno - 1] if lineno <= len(lines) else "" - violations.append((lineno, src_line.strip())) + if not ( + isinstance(target, ast.Name) + and target.id == "__version__" + and isinstance(node.value, (ast.Constant, ast.JoinedStr)) + ): + continue + if _is_inside_except_handler(node, tree): + continue + lineno = node.lineno + lines = source.splitlines() + src_line = lines[lineno - 1] if lineno <= len(lines) else "" + violations.append((lineno, src_line.strip())) return violations -def main() -> int: - """Check files passed by pre-commit for hardcoded __version__.""" - # Only check __init__.py files (pre-commit passes filenames as args) - init_files = [ - Path(f) for f in sys.argv[1:] if Path(f).name == "__init__.py" - ] +# --------------------------------------------------------------------------- +# YAML string-version checks +# --------------------------------------------------------------------------- + +_VERSION_FIELD_NAMES = { + "version", + "contract_version", + "node_version", + "protocol_version", +} + + +def _has_string_version_in_yaml(path: Path) -> list[tuple[int, str]]: + """Return (line_number, message) for YAML files with string version fields. + + Skips files matching the ticket-contract exemption (OMN-9593). + """ + if _is_ticket_contract(path): + return [] + + violations: list[tuple[int, str]] = [] + try: + lines = path.read_text(encoding="utf-8").splitlines() + except (UnicodeDecodeError, OSError): + return violations + + for line_num, raw in enumerate(lines, 1): + stripped = raw.strip() + if not stripped or stripped.startswith("#"): + continue + colon = stripped.find(":") + if colon == -1: + continue + field = stripped[:colon].strip() + value = stripped[colon + 1 :].strip() + if field not in _VERSION_FIELD_NAMES and "version" not in field.lower(): + continue + if _SEMVER_RE.match(value): + violations.append( + ( + line_num, + f"string version '{value}' — use ModelSemVer mapping " + f"{{major: X, minor: Y, patch: Z}}", + ) + ) + return violations + +# --------------------------------------------------------------------------- +# Entry point +# --------------------------------------------------------------------------- + + +def main() -> int: + """Check files passed by pre-commit for version anti-patterns.""" found = 0 - for path in init_files: + + paths = [Path(f) for f in sys.argv[1:]] + + # Python __init__.py check + for path in paths: + if path.name != "__init__.py" or path.suffix != ".py": + continue for lineno, src_line in _has_hardcoded_version(path): print( f"{path}:{lineno}: hardcoded __version__ found: {src_line}\n" - f" Use importlib.metadata.version(\"package-name\") instead." + f' Use importlib.metadata.version("package-name") instead.' ) found += 1 + # YAML string-version check + for path in paths: + if path.suffix not in (".yaml", ".yml"): + continue + for lineno, msg in _has_string_version_in_yaml(path): + print(f"{path}:{lineno}: {msg}") + found += 1 + return 1 if found else 0 diff --git a/tests/unit/test_validate_string_versions.py b/tests/unit/test_validate_string_versions.py new file mode 100644 index 00000000000..536770bb866 --- /dev/null +++ b/tests/unit/test_validate_string_versions.py @@ -0,0 +1,116 @@ +# SPDX-FileCopyrightText: 2025 OmniNode.ai Inc. +# SPDX-License-Identifier: MIT + +"""Tests for scripts/validation/validate_string_versions.py. + +Covers: +- Python __init__.py hardcoded __version__ detection +- YAML string version detection +- Ticket-contract exemption (OMN-9593): contracts/OMN-*.yaml files are exempt + because their schema_version is validated by ModelTicketContract.field_validator +""" + +from pathlib import Path + +from scripts.validation.validate_string_versions import ( + _has_hardcoded_version, + _has_string_version_in_yaml, + _is_ticket_contract, +) + +SCRIPT_DIR = Path(__file__).resolve().parent.parent / "scripts" / "validation" + + +def _write(tmp_path: Path, name: str, content: str) -> Path: + p = tmp_path / name + p.parent.mkdir(parents=True, exist_ok=True) + p.write_text(content, encoding="utf-8") + return p + + +class TestPythonInitVersion: + def test_detects_hardcoded_version(self, tmp_path: Path) -> None: + p = _write(tmp_path, "__init__.py", '__version__ = "1.2.3"\n') + violations = _has_hardcoded_version(p) + assert len(violations) == 1 + assert "1.2.3" in violations[0][1] + + def test_clean_init_passes(self, tmp_path: Path) -> None: + p = _write( + tmp_path, + "__init__.py", + 'from importlib.metadata import version\n__version__ = version("pkg")\n', + ) + assert _has_hardcoded_version(p) == [] + + def test_except_fallback_allowed(self, tmp_path: Path) -> None: + src = ( + "try:\n" + " from importlib.metadata import version\n" + " __version__ = version('pkg')\n" + "except Exception:\n" + " __version__ = '0.0.0'\n" + ) + p = _write(tmp_path, "__init__.py", src) + assert _has_hardcoded_version(p) == [] + + +class TestYamlStringVersion: + def test_detects_string_version_in_yaml(self, tmp_path: Path) -> None: + p = _write(tmp_path, "config.yaml", 'version: "1.0.0"\n') + violations = _has_string_version_in_yaml(p) + assert len(violations) == 1 + assert "ModelSemVer" in violations[0][1] + + def test_detects_single_quoted_string_version_in_yaml(self, tmp_path: Path) -> None: + p = _write(tmp_path, "config.yaml", "version: '1.0.0'\n") + violations = _has_string_version_in_yaml(p) + assert len(violations) == 1 + assert "ModelSemVer" in violations[0][1] + + def test_detects_schema_version_in_generic_yaml(self, tmp_path: Path) -> None: + p = _write(tmp_path, "service.yaml", 'schema_version: "2.3.1"\n') + violations = _has_string_version_in_yaml(p) + assert len(violations) == 1 + + def test_mapping_version_passes(self, tmp_path: Path) -> None: + p = _write( + tmp_path, "config.yaml", "version:\n major: 1\n minor: 0\n patch: 0\n" + ) + assert _has_string_version_in_yaml(p) == [] + + +class TestTicketContractExemption: + def test_contracts_dir_exempt(self, tmp_path: Path) -> None: + p = _write( + tmp_path, + "contracts/OMN-9593.yaml", + 'schema_version: "1.0.0"\nticket_id: OMN-9593\n', + ) + assert _is_ticket_contract(p) is True + assert _has_string_version_in_yaml(p) == [] + + def test_non_contract_yaml_not_exempt(self, tmp_path: Path) -> None: + p = _write(tmp_path, "deploy.yaml", 'schema_version: "1.0.0"\n') + assert _is_ticket_contract(p) is False + violations = _has_string_version_in_yaml(p) + assert len(violations) == 1 + + def test_similarly_named_contracts_dir_not_exempt(self, tmp_path: Path) -> None: + p = _write(tmp_path, "mycontracts/OMN-9593.yaml", 'schema_version: "1.0.0"\n') + assert _is_ticket_contract(p) is False + violations = _has_string_version_in_yaml(p) + assert len(violations) == 1 + + def test_windows_contract_path_exempt(self) -> None: + p = Path(r"C:\repo\contracts\OMN-9593.yaml") + assert _is_ticket_contract(p) is True + + def test_template_not_exempt(self, tmp_path: Path) -> None: + p = _write( + tmp_path, + "templates/ticket_contract.yaml", + 'schema_version: "1.0.0"\n', + ) + assert _is_ticket_contract(p) is False + assert len(_has_string_version_in_yaml(p)) == 1