Repository navigation
feat(ci): add purity/naming/determinism gates for schema modules [OMN-966] - #8
Conversation
…-966] This PR implements CI hardening per D-008 purity requirements: ## New CI Gates ### Schema Purity & Naming Check (new job) - Enforces purity: forbids env/fs/network/time usage in schema modules - Enforces naming: Model*/model_*, Enum*/enum_* - Detects forbidden imports: os, time, requests, socket, etc. - Detects forbidden calls: datetime.now(), os.getenv(), Path.cwd() ### Manifest Validation (added to schema-determinism) - Validates manifest.json structure - Verifies SHA256 hashes match actual file content - Ensures export_script_version is present for traceability ## New Scripts - scripts/check_schema_purity.py - AST-based purity and naming checker - scripts/validate_manifest.py - Manifest hash and traceability validator ## Tests - 17 tests for purity/naming enforcement - 13 tests for manifest validation Closes OMN-966
|
Warning Rate limit exceeded@jonahgabriel has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 6 minutes and 37 seconds before requesting another review. ⌛ 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. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR transitions from exporting JSON schemas from Pydantic models to exposing models directly for downstream consumption. It removes the schema export infrastructure (scripts and generated files) while introducing a new schema purity checker that enforces module isolation and naming conventions via AST analysis. CI workflows are simplified with a custom setup-poetry GitHub Action, and CLI tooling is refactored to use packaged entrypoints instead of direct script invocation. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
Comment |
Pull Request Review: CI Hardening for Schema Purity and DeterminismSummaryThis PR implements comprehensive CI gates to enforce schema purity, naming conventions, and deterministic schema exports. The implementation is well-structured and production-ready with excellent test coverage (30 tests total). The code quality is high, with proper error handling, type hints, and documentation. ✅ Strengths1. Excellent Code Quality
2. Strong Test Coverage
3. Well-Designed Architecture
4. User Experience
Code Review Findingsscripts/check_schema_purity.pyMinor Issues
scripts/validate_manifest.pyMinor Issues
Security ConsiderationsGood Practices Observed
Minor ConcernFile reading in chunks (validate_manifest.py:49): Consider adding maximum file size check for robustness, though schemas should be small. Priority: Low Performance ConsiderationsObservations
Test Coverage AssessmentCoverage HighlightsWell-covered scenarios:
Missing Test Cases (Medium Priority)Consider adding tests for:
Specific RecommendationsHigh Priority
Medium Priority
Low Priority
CI Workflow ReviewGood Practices
Final VerdictCode Quality: A-Well-structured, type-safe, and maintainable. Minor issues don't impact functionality. Test Coverage: A30 tests covering major scenarios. Minor gaps in edge cases. Security: ANo vulnerabilities. Follows best practices. Performance: B+Efficient for current scale. Double-parsing could be optimized. Recommendation: APPROVE with Minor ImprovementsThis PR is ready to merge as-is. The identified issues are minor and can be addressed in follow-up PRs if desired. The implementation successfully achieves its goals:
Optional Follow-up Tasks
Great work on this implementation! The code is production-ready and follows best practices. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.github/workflows/ci.ymlscripts/check_schema_purity.pyscripts/validate_manifest.pytests/test_check_schema_purity.pytests/test_validate_manifest.py
🧰 Additional context used
🧬 Code graph analysis (3)
tests/test_validate_manifest.py (1)
scripts/validate_manifest.py (2)
validate_manifest(69-152)calculate_file_hash(37-51)
scripts/validate_manifest.py (1)
scripts/check_schema_purity.py (1)
main(365-411)
tests/test_check_schema_purity.py (1)
scripts/check_schema_purity.py (3)
PurityChecker(100-221)check_purity(301-338)check_naming_conventions(224-298)
🪛 GitHub Actions: CI
tests/test_validate_manifest.py
[error] 1-1: ruff: End-of-file-fixer reformatted files; review changes prior to commit.
[error] 24-24: S603 subprocess call: check for execution of untrusted input
[warning] 66-66: PLC0415 import should be at the top-level of a file
scripts/validate_manifest.py
[error] 1-1: ruff: End-of-file-fixer reformatted files; review changes prior to commit.
[error] 1-1: EXE001 Shebang is present but file is not executable
scripts/check_schema_purity.py
[error] 1-1: ruff: End-of-file-fixer reformatted files; review changes prior to commit.
[error] 1-1: EXE001 Shebang is present but file is not executable
[error] 58-58: ERA001 Found commented-out code. Remove commented-out code to satisfy lint rules.
[error] 61-61: ERA001 Found commented-out code. Remove commented-out code to satisfy lint rules.
[error] 179-179: PIE810 Call startswith once with a tuple
[error] 193-193: SIM102 Use a single if statement instead of nested if statements
[error] 264-264: E501 Line too long (89 > 88)
[error] 288-288: SIM102 Use a single if statement instead of nested if statements
tests/test_check_schema_purity.py
[error] 1-1: ruff: End-of-file-fixer reformatted files; review changes prior to commit.
[error] 23-23: S603 subprocess call: check for execution of untrusted input
[warning] 56-56: PLC0415 import should be at the top-level of a file
🪛 Ruff (0.14.10)
tests/test_validate_manifest.py
26-26: subprocess call: check for execution of untrusted input
(S603)
scripts/validate_manifest.py
56-56: Unused noqa directive (non-enabled: T201)
Remove unused noqa directive
(RUF100)
61-61: Unused noqa directive (non-enabled: T201)
Remove unused noqa directive
(RUF100)
66-66: String contains ambiguous ℹ (INFORMATION SOURCE). Did you mean i (LATIN SMALL LETTER I)?
(RUF001)
66-66: Unused noqa directive (non-enabled: T201)
Remove unused noqa directive
(RUF100)
167-167: Unused noqa directive (non-enabled: T201)
Remove unused noqa directive
(RUF100)
181-181: Unused noqa directive (non-enabled: T201)
Remove unused noqa directive
(RUF100)
189-189: Unused noqa directive (non-enabled: T201)
Remove unused noqa directive
(RUF100)
190-190: Unused noqa directive (non-enabled: T201)
Remove unused noqa directive
(RUF100)
191-191: Unused noqa directive (non-enabled: T201)
Remove unused noqa directive
(RUF100)
192-192: Unused noqa directive (non-enabled: T201)
Remove unused noqa directive
(RUF100)
195-195: Unused noqa directive (non-enabled: T201)
Remove unused noqa directive
(RUF100)
scripts/check_schema_purity.py
114-114: Unused noqa directive (non-enabled: N802)
Remove unused noqa directive
(RUF100)
134-134: Unused noqa directive (non-enabled: N802)
Remove unused noqa directive
(RUF100)
159-159: Unused noqa directive (non-enabled: N802)
Remove unused noqa directive
(RUF100)
185-185: Unused noqa directive (non-enabled: N802)
Remove unused noqa directive
(RUF100)
362-362: Unused noqa directive (non-enabled: T201)
Remove unused noqa directive
(RUF100)
376-376: Unused noqa directive (non-enabled: T201)
Remove unused noqa directive
(RUF100)
381-381: Unused noqa directive (non-enabled: T201)
Remove unused noqa directive
(RUF100)
382-382: Unused noqa directive (non-enabled: T201)
Remove unused noqa directive
(RUF100)
394-394: Unused noqa directive (non-enabled: T201)
Remove unused noqa directive
(RUF100)
395-395: Unused noqa directive (non-enabled: T201)
Remove unused noqa directive
(RUF100)
403-403: Unused noqa directive (non-enabled: T201)
Remove unused noqa directive
(RUF100)
406-406: Unused noqa directive (non-enabled: T201)
Remove unused noqa directive
(RUF100)
410-410: Unused noqa directive (non-enabled: T201)
Remove unused noqa directive
(RUF100)
tests/test_check_schema_purity.py
25-25: subprocess call: check for execution of untrusted input
(S603)
⏰ 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)
.github/workflows/ci.yml (3)
88-111: LGTM! Well-structured CI job following established patterns.The new
schema-purityjob correctly mirrors the setup pattern used by other jobs (Poetry caching, Python 3.12, pinned Poetry version). The job will effectively gate schema module changes.
115-115: Correct dependency chain for schema validation.Adding
schema-purityto theneedsarray ensures determinism checks only run after purity validation passes, maintaining a logical CI flow.
145-146: Good addition of manifest hash validation step.This completes the integrity chain: purity → determinism export → manifest hash verification.
scripts/validate_manifest.py (3)
37-51: LGTM! Robust chunked hash calculation.The implementation correctly uses chunked reading (4096 bytes) to handle large files efficiently without loading them entirely into memory.
69-152: LGTM! Comprehensive manifest validation logic.The validation covers all critical aspects: required fields, schema version matching, traceability (export_script_version), file existence, and hash verification. Error messages are clear and actionable.
155-201: LGTM! Clean orchestration with clear exit codes.The main function provides good user feedback, handles missing manifests gracefully, and displays a useful summary on success.
tests/test_validate_manifest.py (4)
15-32: LGTM! Safe subprocess helper for integration tests.The S603 warning is a false positive here—the command is fully controlled (hardcoded script path, no user input). The helper correctly sets working directory and captures output for assertions.
35-60: LGTM! Good integration test coverage.Tests verify the script's output behavior against the real manifest, ensuring CI will catch regressions.
63-209: LGTM! Thorough unit test coverage for validation logic.Tests cover all error paths: missing files, hash mismatches, missing fields, empty export version (traceability), empty schemas list, and invalid JSON. The local imports (PLC0415) are intentional to keep test isolation clean.
212-238: LGTM! Hash determinism tests are valuable.Verifying that hash calculation is deterministic is important for the manifest validation guarantee.
tests/test_check_schema_purity.py (4)
14-31: LGTM! Safe subprocess helper for integration tests.Same pattern as the manifest validation tests—S603 is a false positive since the command is fully controlled.
34-48: LGTM! Integration tests verify script behavior.These tests ensure the existing schema files pass validation, serving as a regression guard.
51-142: LGTM! Comprehensive purity violation detection tests.Good coverage of forbidden imports (
os,time,requests), forbidden calls (datetime.now), and allowed patterns (fromisoformat,pydantic,re). The test structure makes it easy to add new cases.
145-251: LGTM! Thorough naming convention tests.Tests cover both positive and negative cases for models and enums, plus edge cases like
__init__.pyskipping and private class allowances.scripts/check_schema_purity.py (6)
32-88: LGTM! Well-organized forbidden/allowed lists.The categorization (environment, filesystem, network, time, etc.) is clear and comprehensive. The
ALLOWED_DATETIME_USAGEset enables legitimate parsing operations while blocking non-deterministic time access.
91-98: LGTM! Clean data structure for violations.Using
NamedTupleprovides immutability and clear field access.
100-158: LGTM! Solid AST visitor for import checking.The visitor correctly tracks import aliases to resolve full module names and checks both top-level modules and full paths against the forbidden list.
185-221: LGTM! Attribute chain resolution for detection.The recursive attribute chain building correctly identifies patterns like
os.environ.getregardless of import style.
301-338: LGTM! Robust purity checking with proper error handling.The function handles file read errors and syntax errors gracefully, returning violations rather than crashing.
365-411: LGTM! Clean orchestration with grouped violation output.The main function provides excellent user feedback: file count, categorized violations, and a clear pass/fail summary.
- Remove shebangs from scripts (EXE001) - Remove inline comments in frozensets (ERA001) - Use tuple for startswith() calls (PIE810) - Combine nested if statements (SIM102) - Fix line too long (E501) - Add __init__.py to scripts/ (INP001) - Move imports to top-level in tests (PLC0415) - Add noqa for subprocess and complex functions - Remove unused imports (re, CLI_VERSION) - Use non-ambiguous characters in output
Pull Request Review: CI Hardening - Purity/Naming/Determinism GatesOverall AssessmentStatus: ✅ Approved with Minor Suggestions This is an excellent implementation of CI hardening that enforces schema purity and naming conventions. The code is well-structured, thoroughly tested, and aligns with the repository's design goals (D-008 purity requirements). The PR adds significant value by preventing non-deterministic behavior in schema modules. Code Quality: ✅ ExcellentStrengths
Test Coverage: ✅ Excellent
Suggestions for Test Improvement
Potential Issues:
|
…tection [OMN-966] - Add docstring to scripts/__init__.py to fix D104 linting error - Fix nested class detection: use tree.body instead of ast.walk() to only check top-level classes (addresses PR review comment) - Update comment to accurately reflect behavior
Pull Request Review: CI Hardening for Schema Purity & DeterminismSummaryThis PR implements comprehensive CI hardening to enforce schema module purity, naming conventions, and determinism. The implementation is well-structured and aligns with the stated D-008 purity requirements. Overall, this is high-quality work with excellent test coverage and clear implementation. Strengths1. Excellent Test Coverage
2. Clear Documentation
3. Robust AST-Based AnalysisThe purity checker uses proper AST traversal to detect:
4. Good Security Practices
5. CI Integration
Issues & RecommendationsCRITICAL Issues1. Incomplete Test File (test_check_schema_purity.py:220) Impact: High - May indicate incomplete implementation HIGH Priority Issues2. Hardcoded Schema Version (scripts/validate_manifest.py:24) Issue: The schema version is hardcoded in multiple places:
Impact: When the schema version needs to increment, it must be updated in multiple locations, increasing the risk of inconsistency. Recommendation: Create a single source of truth for the schema version (e.g., src/onex_change_control/version.py) and import it in both scripts. 3. Inconsistent Error Handling Between Scripts Issue:
Recommendation: Standardize on a consistent exit code strategy across all validation scripts. Consider:
MEDIUM Priority Issues4. Potential Performance Issue: Multiple File Reads (scripts/check_schema_purity.py:358-365) Issue: For each file, the script:
Recommendation: Refactor to read each file once and perform all checks on the parsed AST in a single pass. 5. No Validation of Purity Checker Against Existing Files Issue: The new purity checks are added but there's no verification that existing schema files actually pass these checks before enforcing them in CI. Recommendation: Add a test that explicitly validates all existing schema files pass the purity checks. 6. Unused Security Constraints (scripts/check_schema_purity.py:20-22) Issue: Security constraints (_MAX_STRING_LENGTH, _MAX_LIST_ITEMS) are defined but never used in the purity checker itself. Recommendation: Remove unused constants or add a comment explaining their purpose. Code Quality Assessment
Security ReviewNo security concerns identified The implementation follows security best practices:
Performance ConsiderationsCurrent Performance
Expected ImpactFor a typical codebase with 10-20 schema files, the duplicate parsing adds ~50-100ms overhead. Not critical but worth optimizing. Action ItemsMust Fix Before Merge
Should Fix Before Merge
Consider for Follow-up PR
ConclusionThis PR represents excellent work implementing critical CI hardening for schema purity and determinism. The implementation is thorough, well-tested, and follows good software engineering practices. Recommendation: APPROVE with minor changesThe issues identified are primarily about consistency and optimization rather than correctness. The critical test file truncation should be verified, and the schema version duplication should be addressed, but otherwise this is ready for merge. Great job on the comprehensive test coverage and clear documentation! Review completed by Claude Code - focused on code quality, security, performance, and maintainability per D-008 purity requirements. |
… usage [OMN-966]
The JSON schema export was redundant since we use Pydantic models directly
for validation. Downstream packages can import and use the models:
from onex_change_control import ModelDayClose, ModelTicketContract
ModelTicketContract.model_validate(data)
Removed:
- scripts/export_json_schema.py
- scripts/validate_manifest.py
- schemas/ directory (JSON schema files)
- schema-determinism CI job
- Related tests
Updated:
- README with downstream usage examples
- __init__.py docstring
- CI workflow (removed schema-determinism job)
The Pydantic models are the source of truth and are properly exported.
This simplifies the package and removes unnecessary complexity.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/ci.yml (1)
88-111: Inconsistency: PR objectives claim manifest validation addition, but the script does not exist.The PR objectives reference "Manifest validation added to schema-determinism job via scripts/validate_manifest.py," but the schema-purity job in the workflow only runs
scripts/check_schema_purity.py. The repository contains novalidate_manifest.pyfile and no references to manifest validation anywhere in the codebase. Either add the validation script to this workflow or update the PR objectives to match the actual implementation.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
.github/workflows/ci.ymlREADME.mdREVIEW_READINESS.mdschemas/1.0.0/day_close.schema.jsonschemas/1.0.0/manifest.jsonschemas/1.0.0/ticket_contract.schema.jsonscripts/export_json_schema.pysrc/onex_change_control/__init__.pytests/test_export_json_schema.py
💤 Files with no reviewable changes (5)
- schemas/1.0.0/ticket_contract.schema.json
- tests/test_export_json_schema.py
- scripts/export_json_schema.py
- schemas/1.0.0/day_close.schema.json
- schemas/1.0.0/manifest.json
✅ Files skipped from review due to trivial changes (1)
- src/onex_change_control/init.py
🧰 Additional context used
🪛 GitHub Actions: CI
REVIEW_READINESS.md
[error] 1-1: end-of-file-fixer hook modified the file; pre-commit check failed (exit code 1) during 'poetry run pre-commit run --all-files'.
🪛 markdownlint-cli2 (0.18.1)
REVIEW_READINESS.md
90-90: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ 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 (3)
README.md (2)
48-67: Inconsistency: Documentation describes removal of export functionality, but PR objectives claim to add features.The PR objectives state this PR adds:
"New CI job 'Schema Purity & Naming Check'... Manifest validation added to schema-determinism job via scripts/validate_manifest.py"
However, the README changes show a shift from schema export to direct model usage, which aligns with the commit message stating this is a "refactor removing JSON schema export." The actual changes appear to simplify/remove functionality rather than add the manifest validation described in the objectives.
48-67: The referencedvalidate_yaml.pyscript exists and is functional.The script is present at
scripts/validate_yaml.pywith a complete CLI implementation including proper error handling, schema detection, and exit codes. The documentation accurately describes how to invoke it viapoetry run python scripts/validate_yaml.py <file.yaml>, which is the correct approach since the script is not exposed as a Poetry entrypoint. The command works as documented.The script is not invoked in the CI workflow, but this is not inherently problematic given that other validation mechanisms are in place (pre-commit, type-checking, unit tests). If CLI validation should be part of CI, that would be a separate enhancement to consider.
Likely an incorrect or invalid review comment.
REVIEW_READINESS.md (1)
22-48: Inconsistency: PR objectives claim scripts/validate_yaml.py is being added, but this document treats it as existing.The PR objectives describe adding new scripts and features, but this readiness document discusses
validate_yaml.pyas if it already exists and is evaluating whether to make it a CLI entrypoint. This suggests the PR objectives may be incorrectly describing what's actually happening in this PR.
| print(f"❌ {contract_file} is invalid: {e}") | ||
| sys.exit(1) | ||
| ``` | ||
| ``` |
There was a problem hiding this comment.
Add language specifier to fenced code block.
The markdownlint tool flags this code block as missing a language specifier. Add python after the opening backticks for proper syntax highlighting and to satisfy the linter.
🔎 Proposed fix
-```
+```python
# scripts/validate_contracts.py
from pathlib import Path
from onex_change_control import ModelTicketContract📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ``` |
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
90-90: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
In REVIEW_READINESS.md around line 90, the fenced code block is missing a
language specifier; update the opening fence to include "python" (i.e. change
``` to ```python) so the block reads as a Python code block and satisfies
markdownlint and proper syntax highlighting.
PR Review: CI Hardening - Purity/Naming/Determinism Gates [OMN-966]OverviewThis PR implements comprehensive CI gates to enforce schema purity, naming conventions, and determinism as specified in D-008. The implementation is well-structured with strong test coverage and clear documentation. ✅ Strengths1. Excellent AST-Based Purity CheckingThe check_schema_purity.py script uses AST traversal to detect impure operations:
This prevents non-deterministic behavior in schema modules, which is critical for reproducible builds. 2. Strong Test Coverage
3. Well-Designed Architecture
4. CI Integration is Clean
🔍 Code Quality Issues1. Inconsistent Error Messages Format (Medium)Location: scripts/check_schema_purity.py:111, 136, 152, 162 Different violation messages use different suffixes: (violates purity) vs (non-deterministic) Recommendation: Standardize to one format for consistency. 2. Broad OSError Catch (Medium)Location: scripts/check_schema_purity.py:296-304 The broad OSError catch makes debugging harder in CI. Recommendation: Include exception type in message for better diagnostics. 🐛 Potential Bugs1. Missing Validation: Empty SCHEMA_MODULE_PATHS (Medium)Location: scripts/check_schema_purity.py:22-25 Issue: If schema directories don't exist, script exits with success (code 0), potentially masking configuration errors. Recommendation: Add validation in main() to check directories exist before reporting No schema files found. ⚡ Performance ConsiderationsNo Performance Concerns ✅
🔒 Security Considerations1. Good: String Length Limits ✅Models have _MAX_STRING_LENGTH = 10000 to prevent DoS attacks via enormous YAML files. 2. Good: Input Validation on Emergency Bypass ✅Rejects whitespace-only justifications, preventing abuse. 3. Potential: Command Injection Risk (Low-Medium)Location: src/onex_change_control/models/model_ticket_contract.py:26-30 The command field in ModelEvidenceRequirement could enable command injection if downstream code executes it via shell. Recommendation: Add docstring warning: command field is for documentation only. Do NOT execute directly via shell. Current Risk: LOW - No evidence of command execution in this repo. 🧪 Test Coverage AnalysisCurrent Coverage: Excellent17 tests covering forbidden imports, calls, naming conventions. Gap: No Test for os.environ Attribute Access (Medium)Code handles os.environ[KEY] access but it's not explicitly tested. Recommendation: Add test to verify os.environ attribute access is caught. 🏗️ Architecture & Design Decisions1. Removed Schema Export/Diff CheckTrade-offs:
Recommendation: Add lightweight test that exports schemas to ensure they're valid. 2. Scripts in Root vs PackageCurrent: scripts/check_schema_purity.py Impact:
Verdict: Acceptable for now, address before publishing. 📋 Summary & RecommendationsMust Fix Before Merge: NONE ✅Should Fix (Medium Priority):
Nice to Have (Low Priority):
Future Work (Post-OMN-966):
✅ Final VerdictAPPROVE with minor suggestions This PR successfully implements D-008 purity requirements with:
The identified issues are minor and don't block functionality. Great work! Review completed: Code quality ✅ | Security ✅ | Tests ✅ | Performance ✅ |
- Combine check_purity and check_naming_conventions into single check_file function - Parse each file only once instead of twice - Update tests to use new combined function - Addresses PR review comment about performance optimization
Pull Request Review: CI Hardening with Purity & Naming GatesSummaryThis PR implements comprehensive CI hardening for schema modules, adding AST-based purity checks and naming convention enforcement. The implementation is well-architected, thoroughly tested, and aligns with the D-008 purity requirements. ✅ Strengths1. Excellent Implementation Quality
2. Strong Test Coverage
3. Good Security Practices
4. Clean CI Integration
🔍 Issues & Concerns1. Critical: Missing Validation Script Location
|
Pull Request Review: CI Hardening - Schema Purity & Naming Gates🎯 Overall AssessmentStatus: ✅ APPROVED - Excellent Implementation This PR successfully implements comprehensive CI hardening for schema purity and naming conventions per D-008 requirements. The implementation is production-ready with exceptional code quality, thorough testing, and clear documentation. Grade: A (Outstanding work) 📊 Review Summary
✅ Major Strengths1. Excellent Code Architecture
2. Comprehensive Purity EnforcementThe forbidden patterns list is thorough and well-designed:
3. Smart Naming Convention Enforcement
4. Excellent Test Coverage (30 tests)5. User-Friendly Error Messages❌ Found 2 violation(s):
forbidden_import (1):
src/.../model_test.py:5: [forbidden_import] Forbidden import: 'os' (violates purity)
naming_class (1):
src/.../model_bad.py:10: [naming_class] Class 'BadName' should start with 'Model'6. Clean RefactoringThe PR removes the now-unnecessary JSON schema export approach in favor of:
🔍 Code Quality Highlightsscripts/check_schema_purity.pyWell-designed patterns:
def visit_Import(self, node: ast.Import) -> None:
for alias in node.names:
module_name = alias.name
stored_name = alias.asname if alias.asname else alias.name
self._imported_names[stored_name] = module_name
# Check top-level module
top_module = module_name.split(".")[0]
if top_module in FORBIDDEN_IMPORTS or module_name in FORBIDDEN_IMPORTS:
# Report violation
def _get_attribute_chain(self, node: ast.Attribute) -> str | None:
parts: list[str] = []
current: ast.expr = node
while isinstance(current, ast.Attribute):
parts.append(current.attr)
current = current.value
if isinstance(current, ast.Name):
parts.append(current.id)
return ".".join(reversed(parts))
🎨 Best Practices Observed✅ Security
✅ Error Handling
✅ Code Style
✅ CI/CD Integration
📝 Minor Observations (Non-blocking)1. Excellent Progressive RefinementThe commit history shows great iterative improvement:
2. Well-Scoped ChangesThe PR correctly removes:
And adds focused new functionality:
🔒 Security AssessmentNo security concerns identified. ✅ Safe file operations (proper encoding, path handling) ⚡ Performance AssessmentCurrent: Efficient for current scale (< 20 schema files) Latest optimization (commit 2af0372):
Future consideration: If schema count exceeds 100 files, consider parallel processing with 🧪 Test Quality AssessmentExcellent coverage across multiple dimensions:
Missing tests (optional improvements):
Impact: Low - current coverage is excellent for production use. 📚 Documentation QualityREVIEW_READINESS.md - Excellent Self-AssessmentThis document demonstrates outstanding engineering maturity:
Key insight: Models are already usable; enhancements are nice-to-have. README.md Updates
🎯 Alignment with D-008 Requirements
💡 RecommendationsFor This PR: APPROVE AND MERGE ✅No blocking issues. The implementation is production-ready. Optional Follow-up Work (separate PRs)Low priority:
Medium priority (from REVIEW_READINESS.md):
🏆 Final VerdictThis is exemplary work. The implementation demonstrates: ✅ Deep Python expertise (AST manipulation, type safety) The PR successfully achieves its goals:
Recommended action: ✅ Approve and merge Great work on this implementation! 🎉 Reviewed by: Claude Code |
Code Review: PR #8 - Schema Purity & Naming GatesOverviewThis PR implements CI hardening to enforce purity constraints and naming conventions for schema modules. Overall, this is high-quality work with excellent engineering practices. ✅ StrengthsCode Quality & Best Practices
Security
Test Coverage
🔍 Issues & Recommendations1. Missing Validation: datetime Alias Resolution (Medium Priority)Location: scripts/check_schema_purity.py:184-204 Someone could bypass checks using aliases: from datetime import datetime as dt
now = dt.now() # Might slip throughThe _get_call_name() doesn't resolve aliases, so dt.now() returns "dt.now" instead of "datetime.now". Fix: Enhance _get_call_name() to resolve aliases using _imported_names, or add datetime to FORBIDDEN_IMPORTS. 2. Performance: Redundant AST Parsing (Minor)Location: scripts/check_schema_purity.py:246-248, 283-307 Files are parsed twice (once for naming, once for purity). Consider a parse_file() helper. Not critical for small codebases. 3. Architecture: Scripts Location (Medium Priority)Scripts are in root scripts/ directory, not importable as package modules. Recommendation:
4. Documentation: Version Mapping Missing (Medium)Package version 0.1.0, schema version 1.0.0 - relationship not documented. Add version mapping policy to README. 5. Test Gap: Alias ResolutionMissing test for aliased forbidden calls. Add test to verify alias handling. 6. CI Workflow Optimization (Optional)schema-purity job runs parallel to tests. Consider adding needs: [test] to save CI minutes on broken code. 🎯 Security & PerformanceSecurity: Excellent - purity enforcement, DoS protection, no command injection, no eval/exec Performance: File I/O twice per file negligible for ~5 files. Consider caching for 100+ files. 🧪 Test Coverage
📝 SummaryVerdict: APPROVE WITH MINOR SUGGESTIONS ✅Solid engineering. Core functionality correct, well-tested, best practices followed. Issues are minor enhancement opportunities. No Blocking Issues ✅Recommended Follow-ups (Post-Merge)
Excellent Work On
Great work! 🚀 |
Pull Request Review: CI Hardening - Purity/Naming/Determinism Gates [OMN-966]Overall AssessmentStatus: ✅ Approve with minor suggestions This PR successfully implements comprehensive CI hardening to enforce schema purity and naming conventions. The implementation is well-architected, thoroughly tested, and aligns with the D-008 purity requirements. The shift from JSON schema exports to direct Pydantic model validation is a solid architectural decision. Strengths1. Excellent AST-Based Purity Checking ⭐The
2. Robust Test Coverage ⭐17 tests for purity/naming enforcement demonstrate thoroughness:
3. Clear Documentation & Error MessagesThe script provides actionable error messages with clear violation categories. 4. Appropriate SimplificationRemoving the JSON schema export mechanism in favor of direct Pydantic model imports is pragmatic:
Issues & Concerns🔴 Critical IssuesNone identified. The implementation is solid. 🟡 Moderate Issues1. Incomplete AST Call Name Resolution (scripts/check_schema_purity.py:184-190)The
Impact: Low - these patterns are unlikely in schema modules, but could be used to bypass checks. Recommendation: Add a comment documenting known limitations. 2. No Validation of Field Default ValuesThe purity checker doesn't validate that Pydantic field defaults are pure. Example that would pass: class ModelBad(BaseModel):
timestamp: datetime = Field(default_factory=datetime.now) # Not detectedRecommendation: Add AST checking for 3. Scripts Not Packaged as ModuleThe Impact: Downstream users cannot easily import or reuse purity checking logic. Recommendation: Consider whether purity checking should be a development-only tool or a reusable utility. 🟢 Minor Issues1. Hardcoded Security Limits (model_ticket_contract.py:13-14)Security limits are defined per-file. Consider centralizing in a 2. CI Job Naming (.github/workflows/ci.yml:88)The job was renamed from 3. Repeated File ParsingFiles are parsed twice - once in Impact: Low - schema modules are small, and CI runs are infrequent. Security Considerations✅ Strengths
|
- Add test for aliased forbidden imports - Add test for aliased forbidden calls (documents current behavior) - Add test for from __future__ imports (should be allowed) - Add test for environment variable dict access - Add test for allowed Path basic operations Addresses PR review comments about missing test coverage.
…directory validation [OMN-966] - Standardize error message format to use '(violates purity)' consistently - Enhance OSError catch to show exception type for better diagnostics - Add directory existence validation to catch configuration errors - Add test for directory existence validation - All 23 tests passing
PR Review: CI Hardening with Purity/Naming/Determinism GatesExecutive SummaryOverall Assessment: ✅ Strong Implementation - Ready to Merge with Minor Recommendations This PR successfully implements comprehensive CI hardening for schema modules with excellent test coverage and thoughtful implementation. The AST-based purity checker is well-architected and the refactoring to remove JSON schema export in favor of direct Pydantic model usage is a smart simplification. Strengths ✅1. Excellent Architecture & Code Quality
2. Strong Test Coverage
3. Smart Simplification
4. Security Conscious
Issues & Recommendations🔴 Critical Issues (None Found)No blocking issues detected. 🟡 Medium Priority Improvements1. Incomplete Alias Tracking for Function CallsLocation: scripts/check_schema_purity.py:184-204 Issue: The checker detects aliased imports but doesn't track aliases through to function calls. For example, Recommendation: For MVP, this is acceptable since forbidden modules are already caught at import time. For future enhancement, use 2. Environment Variable Access via SubscriptLocation: scripts/check_schema_purity.py:167-182 Issue: Only attribute access to Recommendation: Add a 3. CI Job IndependenceLocation: .github/workflows/ci.yml:88-111 Current State: The Recommendation: Add Performance Considerations ⚡Positive
Security Analysis 🔒Strengths
No Security Issues Detected ✅Test Coverage Analysis 📊Coverage Quality: ExcellentWhat's Well Tested:
Minor Gap: Missing test for from-import aliases: Documentation Quality 📚Strengths
Recommendations
Code Style & Best Practices ✅
Specific File Ratingsscripts/check_schema_purity.py: 9/10
tests/test_check_schema_purity.py: 9.5/10
.github/workflows/ci.yml: 8/10
Recommendations SummaryBefore Merge (Optional)
After Merge (Future Enhancements)
ConclusionVerdict: ✅ APPROVED - Ready to Merge This PR represents high-quality engineering with:
The identified issues are minor and don't block merging. The recommendations would make a good follow-up PR, but the current implementation is production-ready. Excellent work! 🎉 Reviewed Files: scripts/check_schema_purity.py (373 lines), tests/test_check_schema_purity.py (318 lines), .github/workflows/ci.yml, src/onex_change_control/models/model_ticket_contract.py, README.md, REVIEW_READINESS.md, pyproject.toml (~1,200+ lines total) |
PR Review: Schema Purity & Naming Gates [OMN-966]Executive SummaryOverall Assessment: ✅ Approve with Minor Suggestions This PR implements robust CI hardening that enforces purity constraints and naming conventions for schema modules. The implementation is well-architected, thoroughly tested, and addresses the D-008 purity requirements effectively. Strengths 🎉1. Excellent Architecture & Design
2. Thorough Test Coverage
3. Security Considerations
4. Clean CI Integration
Code Quality - Minor Issues
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
scripts/check_schema_purity.py (1)
321-386: Remove unusednoqa: T201directives.Static analysis indicates the
T201rule (print statement check) is not enabled, making thesenoqacomments unnecessary. They appear on lines 324, 344, 345, 351, 356, 357, 365, 366, 374, 377, and 381.🔎 Proposed fix (example pattern)
- print(f"Checking {len(schema_files)} schema files...") # noqa: T201 + print(f"Checking {len(schema_files)} schema files...")Apply the same pattern to all affected lines.
REVIEW_READINESS.md (1)
66-91: Add language specifier to the outer fenced code block.The markdownlint tool flags line 90 because the closing fence doesn't match an opening fence with a language specifier. The issue is the nested code block structure—while the intent is clear, the linter sees line 90's closing fence as orphaned.
🔎 Proposed fix
Either flatten the nesting or ensure consistent fence matching. One option:
**Enhancement**: Add CI/pre-commit examples: -```markdown + ### Using in CI/Pre-commit Create a validation script in your repo: ```python # scripts/validate_contracts.py ...-```
This removes the outer markdown fence, presenting the content directly. </details> </blockquote></details> </blockquote></details> <details> <summary>🧹 Nitpick comments (1)</summary><blockquote> <details> <summary>tests/test_check_schema_purity.py (1)</summary><blockquote> `50-86`: **Consider simplifying the directory existence test.** The current approach of string-replacing `SCHEMA_MODULE_PATHS` in a copied script is clever but fragile—if the constant's formatting changes, the test breaks silently. Consider an alternative approach such as mocking or parameterizing the paths at runtime. That said, this works for now and tests an important error condition. </blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used**: defaults **Review profile**: CHILL **Plan**: Pro <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between f8b5b428ef7bd44f8a5ac568806d0cf5b0d76e54 and 440b8af1db6f0d21896217434874ed9481b7f159. </details> <details> <summary>📒 Files selected for processing (4)</summary> * `REVIEW_READINESS.md` * `drift/day_close/2025-12-25.yaml` * `scripts/check_schema_purity.py` * `tests/test_check_schema_purity.py` </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🧬 Code graph analysis (1)</summary> <details> <summary>tests/test_check_schema_purity.py (1)</summary><blockquote> <details> <summary>scripts/check_schema_purity.py (1)</summary> * `check_file` (207-300) </details> </blockquote></details> </details><details> <summary>🪛 markdownlint-cli2 (0.18.1)</summary> <details> <summary>REVIEW_READINESS.md</summary> 90-90: Fenced code blocks should have a language specified (MD040, fenced-code-language) </details> </details> <details> <summary>🪛 Ruff (0.14.10)</summary> <details> <summary>scripts/check_schema_purity.py</summary> 324-324: Unused `noqa` directive (non-enabled: `T201`) Remove unused `noqa` directive (RUF100) --- 344-344: Unused `noqa` directive (non-enabled: `T201`) Remove unused `noqa` directive (RUF100) --- 345-345: Unused `noqa` directive (non-enabled: `T201`) Remove unused `noqa` directive (RUF100) --- 351-351: Unused `noqa` directive (non-enabled: `T201`) Remove unused `noqa` directive (RUF100) --- 356-356: Unused `noqa` directive (non-enabled: `T201`) Remove unused `noqa` directive (RUF100) --- 357-357: Unused `noqa` directive (non-enabled: `T201`) Remove unused `noqa` directive (RUF100) --- 365-365: Unused `noqa` directive (non-enabled: `T201`) Remove unused `noqa` directive (RUF100) --- 366-366: Unused `noqa` directive (non-enabled: `T201`) Remove unused `noqa` directive (RUF100) --- 374-374: Unused `noqa` directive (non-enabled: `T201`) Remove unused `noqa` directive (RUF100) --- 377-377: Unused `noqa` directive (non-enabled: `T201`) Remove unused `noqa` directive (RUF100) --- 381-381: Unused `noqa` directive (non-enabled: `T201`) Remove unused `noqa` directive (RUF100) </details> </details> </details> <details> <summary>⏰ 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)</summary> * GitHub Check: claude-review </details> <details> <summary>🔇 Additional comments (11)</summary><blockquote> <details> <summary>scripts/check_schema_purity.py (5)</summary><blockquote> `1-19`: **LGTM on module structure and imports.** The docstring clearly documents the script's purpose, usage, and exit codes. Imports are minimal and appropriate for the AST-based checking approach. --- `21-70`: **Well-designed constant definitions.** The forbidden imports and calls lists comprehensively cover purity concerns (environment, filesystem, network, time, randomness). Using `frozenset` provides both immutability and efficient O(1) membership testing. --- `73-79`: **LGTM!** Clean, immutable data structure using `NamedTuple` for violation representation. --- `82-204`: **LGTM on the PurityChecker implementation.** The AST visitor correctly: - Tracks imported names with alias resolution - Detects forbidden imports (both `import x` and `from x import y`) - Catches forbidden function calls and `os.environ` access patterns - Handles attribute chains for nested access detection The `startswith(("os.environ", "environ."))` tuple form on line 156 is the correct fix from prior feedback. --- `207-300`: **Solid implementation of the unified check function.** Key design decisions are correct: - Single parse for all checks (performance optimization) - Graceful handling of `OSError` and `SyntaxError` with categorized violations - Iterating only `tree.body` for class naming correctly limits to top-level classes - Private class exemption (`class_name.startswith("_")`) is appropriate </blockquote></details> <details> <summary>tests/test_check_schema_purity.py (3)</summary><blockquote> `14-31`: **LGTM on the test helper function.** The `run_purity_check` helper correctly: - Uses the project root as cwd for consistent path resolution - Captures both stdout and stderr - Uses `check=False` to allow assertion on non-zero exit codes - The `S603` noqa is acceptable since this runs a known script from the test suite --- `89-261`: **Excellent purity violation test coverage.** The tests thoroughly exercise: - Forbidden module imports (os, time, requests) - Forbidden calls (datetime.now) - Allowed patterns (datetime.fromisoformat, pydantic, re, `__future__`) - Aliased imports and calls - Environment access via dict syntax - Path operations (allowing basic, forbidding `.cwd()`/`.home()`) Good use of `tmp_path` fixture and explicit violation category assertions. --- `264-355`: **LGTM on naming convention tests.** Comprehensive coverage of: - File prefix enforcement (`model_`/`enum_`) - Class prefix enforcement (`Model`/`Enum`) - `__init__.py` skip behavior - Private class exemption (`_PrivateHelper`) </blockquote></details> <details> <summary>REVIEW_READINESS.md (2)</summary><blockquote> `1-45`: **LGTM on the executive summary and critical gaps documentation.** Clear documentation of: - Current state (models complete, tested, exported) - Missing items (CLI entrypoint, version mapping policy, usage docs) - Priority levels for each gap - Code examples for workarounds and fixes --- `92-158`: **LGTM on architecture and action plan sections.** Clear documentation of: - Scripts location trade-offs - What works well (models, imports, validation, CI gates, tests) - Prioritized action plan with time estimates - Open questions for team discussion </blockquote></details> <details> <summary>drift/day_close/2025-12-25.yaml (1)</summary><blockquote> `1-92`: **LGTM on the drift report.** Well-structured daily tracking document with: - Clear plan/requirement references - Per-repo PR details with state and notes - Invariant check statuses (all passing) - Appropriate empty sections for drift/corrections/risks The PR notes accurately summarize the work described in the PR objectives. </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
- Resolve aliases in _get_call_name and _get_attribute_chain - Handle patterns like datetime.datetime.now -> datetime.now - Improves detection of forbidden calls through aliased imports - Addresses PR review recommendation for better alias tracking
- Replace magic number 3 with named constant - Fix line length issue - Remove unnecessary assignment before return
Pull Request Review: CI Hardening for Schema ModulesOverviewThis PR implements comprehensive CI hardening for schema modules with purity enforcement, naming conventions, and determinism checks. The implementation is well-designed and thorough, with strong test coverage and clear documentation. ✅ Strengths1. Code Quality & Architecture
2. Test Coverage
3. Security & Safety
4. Documentation
|
Code Review: PR #8 - Schema Purity & Naming GatesSummaryThis is a well-executed PR that implements comprehensive CI hardening for schema modules. The implementation demonstrates strong software engineering practices with robust AST-based validation, excellent test coverage, and thoughtful performance optimization. ✅ Strengths1. Code Quality & Best Practices
2. Security Considerations
3. Test Coverage
|
Code Review: PR #8 - Schema Purity & Naming CI GatesSummaryThis is an excellent PR that implements comprehensive CI hardening for schema modules. The code quality is high, the implementation is thoughtful, and the PR successfully addresses the purity enforcement requirements. The refactoring to remove JSON schema export in favor of direct Pydantic model usage is a smart architectural decision. Overall Assessment: ✅ APPROVE with minor suggestions Strengths1. Architecture & Design
2. Code Quality
3. Testing
4. Security
Issues & SuggestionsCritical IssuesNone - No blocking issues found. High Priority Suggestions1. Scripts Location Architecture (mentioned in REVIEW_READINESS.md:47) Current: Scripts are in root-level scripts/ directory. Issue: Not importable as package module, cannot add Poetry CLI entrypoint without refactoring. Recommendation: Move to src/onex_change_control/scripts/ in a follow-up PR to enable CLI entrypoints. 2. Documentation: Version Mapping Policy Issue: No documented relationship between package version (0.1.0) and schema version (1.0.0). Recommendation: Add a docs/VERSIONING.md or section in README explaining the versioning policy. Medium Priority Suggestions3. Test Coverage Gap: Nested Call Patterns Missing test case: os.path.join(os.getcwd(), file) - is the nested os.getcwd() call detected? Recommendation: Add a test for nested forbidden calls to ensure they are caught at any depth. Security Analysis - No Security Concerns FoundReviewed:
Performance Considerations - Well OptimizedStrengths:
Test Coverage Analysis - ExcellentFrom test_check_schema_purity.py:
CI/CD Analysis - Excellent CI ConfigurationStrengths (.github/workflows/ci.yml):
Note: The removal of schema-determinism job is correct given the architectural shift. Best Practices Adherence - Exemplary
Breaking Changes Analysis - Well-ManagedBreaking change: Removed JSON schema export (schemas/ directory deleted) Mitigation:
Recommendation: When publishing to PyPI (OMN-967), document this in CHANGELOG.md. Recommendations SummaryBefore Merge
Follow-up PRs (Nice to Have)
Final VerdictAPPROVE ✅ This PR represents high-quality engineering work:
The suggestions above are minor enhancements that can be addressed in follow-up PRs. This PR is ready to merge and will significantly improve the schema validation pipeline. Great work! Metrics
|
…-966] - Extract file reading logic into _read_file_safely() helper function to reduce complexity in check_file() - Add schema-purity job dependency on test job for fail-fast behavior - Addresses review suggestions for code organization and CI optimization
Code Review: CI Hardening - Purity/Naming/Determinism Gates [OMN-966]SummaryThis is a well-executed PR that successfully implements CI hardening with comprehensive purity and naming convention enforcement. The implementation is production-ready with excellent test coverage and clear documentation. I've identified several areas for improvement and one potential security consideration. ✅ Strengths1. Excellent Code Quality
2. Strong Test Coverage
3. Security-Conscious Design
4. Architectural Improvements
🔍 Issues & Recommendations1. Security: Unsafe File Reading Pattern 🔴 HIGH PRIORITYLocation: Issue: Using Recommendation: Add path validation to ensure files are within expected directories. While the current usage is safe (only reads from 2. Missing Test: AST Parsing Errors 🟡 MEDIUM PRIORITYIssue: While syntax errors are handled (scripts/check_schema_purity.py:346-357), there's no test case verifying this error path. Recommendation: Add test case for syntax error handling to improve coverage of error paths. 3. Code Smell: Magic Numbers 🟡 MEDIUM PRIORITYLocation: The magic number 4. Potential Bug: Incomplete Forbidden Call Detection 🟡 MEDIUM PRIORITYLocation: Issue: The check for Recommendation: Add a Note: The test at 5. Documentation: REVIEW_READINESS.md 🟢 LOW PRIORITYLocation: Issue: The document identifies gaps for OMN-967 but doesn't directly relate to this PR focused on CI hardening. Recommendation: This document is valuable but consider adding a note at the top clarifying it tracks readiness for OMN-967 (package publishing) and is included for visibility. 📊 Test Coverage AnalysisOverall Assessment: Excellent coverage (30 tests)
🏗️ Architectural DecisionsRemoved JSON Schema Export ✅ GOOD DECISIONRationale:
Scripts Location
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
scripts/check_schema_purity.py (1)
408-465: Remove unusednoqadirectives for cleaner code.All
# noqa: T201comments throughout theT201rule (print usage) is not enabled in your linter configuration. Removing these directives will reduce noise without affecting functionality.📝 Pattern to apply across all print statements
- print(f" {relative_path}:{v.line}: [{v.category}] {v.message}") # noqa: T201 + print(f" {relative_path}:{v.line}: [{v.category}] {v.message}")Apply this pattern to lines 408, 428, 429, 435, 440, 441, 449, 450, 458, 461, and 465.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/ci.ymlscripts/check_schema_purity.py
🧰 Additional context used
🪛 Ruff (0.14.10)
scripts/check_schema_purity.py
408-408: Unused noqa directive (non-enabled: T201)
Remove unused noqa directive
(RUF100)
428-428: Unused noqa directive (non-enabled: T201)
Remove unused noqa directive
(RUF100)
429-429: Unused noqa directive (non-enabled: T201)
Remove unused noqa directive
(RUF100)
435-435: Unused noqa directive (non-enabled: T201)
Remove unused noqa directive
(RUF100)
440-440: Unused noqa directive (non-enabled: T201)
Remove unused noqa directive
(RUF100)
441-441: Unused noqa directive (non-enabled: T201)
Remove unused noqa directive
(RUF100)
449-449: Unused noqa directive (non-enabled: T201)
Remove unused noqa directive
(RUF100)
450-450: Unused noqa directive (non-enabled: T201)
Remove unused noqa directive
(RUF100)
458-458: Unused noqa directive (non-enabled: T201)
Remove unused noqa directive
(RUF100)
461-461: Unused noqa directive (non-enabled: T201)
Remove unused noqa directive
(RUF100)
465-465: Unused noqa directive (non-enabled: T201)
Remove unused noqa directive
(RUF100)
⏰ 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 (6)
.github/workflows/ci.yml (1)
88-112: LGTM! Clean CI integration.The job rename clearly reflects its purpose, and the
needs: [test]dependency is a smart optimization for fail-fast behavior. The step correctly invokes the new purity check script.scripts/check_schema_purity.py (5)
1-27: LGTM! Excellent documentation and appropriate imports.The module docstring is comprehensive and transparently documents the alias detection limitations. Using only standard library imports ensures the script itself remains pure and has no external dependencies.
30-78: LGTM! Comprehensive purity constraints.The use of
frozensetfor forbidden imports/calls provides both immutability and efficient O(1) lookups. The coverage includes the key impurity vectors: environment access (os, dotenv), I/O (socket, urllib, requests), time (time, datetime methods), and non-deterministic sources (random, secrets).
81-238: LGTM! Sophisticated AST-based purity checking.The implementation effectively uses the AST visitor pattern to detect purity violations. Key strengths:
- Alias tracking via
_imported_nameshandles common obfuscation patterns- Checks forbidden imports at both module and top-level (line 112-113)
- Call name simplification (lines 164-181) catches patterns like
datetime.datetime.now()→datetime.now- Attribute chain resolution (lines 194-209) catches direct
os.environaccess- Both exact matches and pattern matching for comprehensive detection
240-384: LGTM! Robust file handling and efficient unified checking.Strong defensive programming in
_read_file_safelywith comprehensive exception handling. Thecheck_filefunction efficiently performs all checks in a single AST parse, and the logic to determine expected prefixes (lines 316-323) is safe since files are sourced exclusively fromfind_schema_files().
387-470: LGTM! Well-structured main flow with good UX.The
mainfunction provides excellent user experience:
- Validates preconditions (schema directories exist)
- Clear progress feedback
- Groups violations by category for easy triage
- Appropriate exit codes for CI integration
The assumption that the script is in
scripts/(line 418) is reasonable and matches the repository structure.
…t access detection [OMN-966] - Add path validation to _read_file_safely() to prevent path traversal attacks - Add visit_Subscript() method to detect os.environ['VAR'] patterns - Add test for syntax error detection - Update environment dict access test to verify subscript detection - Addresses high-priority security review items
PR Review: CI Hardening with Purity and Naming EnforcementOverall AssessmentStatus: Approve with Minor Suggestions ✅ This is a well-engineered PR that successfully implements comprehensive CI hardening for schema modules. The implementation demonstrates strong software engineering practices with thorough testing, good documentation, and thoughtful security considerations. Strengths1. Excellent Code Quality ⭐
2. Security-First Design 🔒
3. Outstanding Test Coverage ✅
4. Developer Experience
Code Quality ObservationsExcellent Practices
Minor Suggestions
Potential Issues1. Limited Alias Resolution for Complex Patterns (Acknowledged)The PR acknowledges this limitation (scripts/check_schema_purity.py:15-22): # Alias Detection Limitations:
# The script tracks imported aliases and resolves them for attribute chains.
# However, deeply nested alias patterns (e.g.,
# `import datetime as dt; dt.datetime.now()`) may not be fully detected.Current behavior: The test at line 216-238 shows this works correctly for Recommendation: Update the docstring to reflect that this case IS handled by the simplification logic at scripts/check_schema_purity.py:164-181. 2. No Validation of Removed FilesThe PR removes the old JSON schema export workflow but doesn't validate that:
Recommendation: Verify that removed JSON schemas aren't referenced elsewhere (or add a comment in the PR description confirming this). 3. Missing Error Code DocumentationExit codes are documented (scripts/check_schema_purity.py:11-13), but violation categories are not. Consider adding: # Violation Categories:
# forbidden_import: Import of impure module (os, time, requests, etc.)
# forbidden_call: Call to impure function (datetime.now, os.getenv, etc.)
# forbidden_access: Direct access to environment (os.environ)
# naming_file: File doesn't follow naming convention
# naming_class: Class doesn't follow naming convention
# file_error: Cannot read/parse file
# syntax_error: Python syntax errorPerformance ConsiderationsStrengths✅ Single AST parse per file (excellent) Minor Optimization OpportunityThe file reading includes multiple path validations. Consider caching def find_schema_files(project_root: Path) -> tuple[list[Path], list[Path]]:
"""Find all Python files and compute allowed directories."""
files: list[Path] = []
allowed_dirs: list[Path] = []
for module_path in SCHEMA_MODULE_PATHS:
full_path = project_root / module_path
if full_path.exists():
allowed_dirs.append(full_path.resolve())
files.extend(full_path.glob("*.py"))
return sorted(files), allowed_dirsThen pass Architecture & DesignExcellent Decisions
Questions
Security ConcernsWell-Handled ✅
No Issues Found 🎉The security posture is excellent. Path validation is thorough without being paranoid. Test Coverage AssessmentCoverage by Category
Missing Test Cases (Minor)
Documentation QualityStrengths
Suggestions
RecommendationsHigh Priority
Medium Priority (Follow-up PRs)
Low Priority
CI/CD IntegrationThe CI changes are clean and well-structured: schema-purity:
name: Schema Purity & Naming Check
runs-on: ubuntu-latest
needs: [test] # Fail fast if tests don't passExcellent:
Question: Should this run in parallel with Comparison to Previous ApproachOld: JSON Schema Export + Diff Check
New: Direct Pydantic Models + Purity Check
Verdict: Significant improvement in design ⭐ Final VerdictApprove and Merge ✅ This PR represents high-quality software engineering:
The minor suggestions above are quality-of-life improvements that can be addressed in follow-up PRs. Great work! This sets a strong foundation for downstream schema consumption. Actionable ItemsBefore Merge (Optional)
After Merge (Future PRs)
Reviewed by: Claude (Sonnet 4.5) |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
tests/test_check_schema_purity.py (1)
50-86: Consider a more maintainable approach for testing missing directories.This test modifies the script source via string replacement, which is brittle and depends on the exact formatting of
SCHEMA_MODULE_PATHS. Consider alternative approaches like mockingPath.exists()or parameterizing the script to accept paths via command-line arguments.scripts/check_schema_purity.py (2)
322-339: Consider more precise path traversal detection.The check
if ".." in path_str:would block legitimate filenames containing ".." (e.g., "model_v2..3.py"). While this provides defense-in-depth alongside the more robust directory validation above, a more precise check would usePathcomponents or regex to detect actual traversal patterns like/../or..as a path segment.🔎 Proposed refinement
# Additional security: Always check for path traversal patterns in the path string # This provides defense-in-depth even for test scenarios - path_str = str(file_path) - if ".." in path_str: + path_parts = file_path.parts + if ".." in path_parts: # Block path traversal patterns (e.g., ../../../etc/passwd) # This applies to both production and test scenarios for security violations.append(
427-436: Consider extracting path validation logic to a helper function.The logic to determine whether path validation is needed is correct but could be more maintainable as a separate helper function (e.g.,
_should_validate_path(file_path: Path) -> bool). This would improve testability and make thecheck_filefunction easier to follow.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
scripts/check_schema_purity.pytests/test_check_schema_purity.py
🧰 Additional context used
🧬 Code graph analysis (1)
tests/test_check_schema_purity.py (1)
scripts/check_schema_purity.py (1)
check_file(387-481)
🪛 GitHub Actions: CI
tests/test_check_schema_purity.py
[error] 264-264: PLR2004 Magic value used in comparison, consider replacing 2 with a constant variable
🪛 Ruff (0.14.10)
scripts/check_schema_purity.py
274-274: Unused noqa directive (non-enabled: PLR0911)
Remove unused noqa directive
(RUF100)
505-505: Unused noqa directive (non-enabled: T201)
Remove unused noqa directive
(RUF100)
525-525: Unused noqa directive (non-enabled: T201)
Remove unused noqa directive
(RUF100)
526-526: Unused noqa directive (non-enabled: T201)
Remove unused noqa directive
(RUF100)
532-532: Unused noqa directive (non-enabled: T201)
Remove unused noqa directive
(RUF100)
537-537: Unused noqa directive (non-enabled: T201)
Remove unused noqa directive
(RUF100)
538-538: Unused noqa directive (non-enabled: T201)
Remove unused noqa directive
(RUF100)
546-546: Unused noqa directive (non-enabled: T201)
Remove unused noqa directive
(RUF100)
547-547: Unused noqa directive (non-enabled: T201)
Remove unused noqa directive
(RUF100)
555-555: Unused noqa directive (non-enabled: T201)
Remove unused noqa directive
(RUF100)
558-558: Unused noqa directive (non-enabled: T201)
Remove unused noqa directive
(RUF100)
562-562: Unused noqa directive (non-enabled: T201)
Remove unused noqa directive
(RUF100)
⏰ 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 (4)
tests/test_check_schema_purity.py (1)
307-398: Excellent test coverage for naming conventions.The naming convention tests are comprehensive and well-structured, covering file prefixes, class prefixes,
__init__.pyexclusion, and private class handling for both models and enums.scripts/check_schema_purity.py (3)
1-78: Well-structured constants and clear documentation.The module-level documentation clearly explains the script's purpose, limitations, and usage. The use of
frozensetfor immutable collections of forbidden imports and calls is appropriate and prevents accidental modification.
90-272: Robust AST-based purity checking with alias resolution.The
PurityCheckerclass provides comprehensive detection of purity violations through AST traversal. The alias tracking via_imported_namesand the multiple visitor methods (visit_Import,visit_ImportFrom,visit_Call,visit_Attribute,visit_Subscript) ensure thorough coverage of forbidden patterns, including environment access via dictionary subscripts.
508-563: Well-structured main function with clear error reporting.The
mainfunction provides excellent user experience with clear status messages, grouped violation output by category, and appropriate exit codes. The directory existence validation and empty file handling prevent confusing errors.
| return None | ||
|
|
||
|
|
||
| def _read_file_safely( # noqa: PLR0911 |
There was a problem hiding this comment.
Remove unused noqa directive.
The # noqa: PLR0911 directive is unused according to static analysis (RUF100), indicating that the PLR0911 rule (too many return statements) is not enabled in your ruff configuration. Remove the directive to keep the code clean.
🔎 Proposed fix
-def _read_file_safely( # noqa: PLR0911
+def _read_file_safely(
file_path: Path, *, validate_path: bool = True
) -> tuple[str | None, list[Violation]]:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def _read_file_safely( # noqa: PLR0911 | |
| def _read_file_safely( | |
| file_path: Path, *, validate_path: bool = True | |
| ) -> tuple[str | None, list[Violation]]: |
🧰 Tools
🪛 Ruff (0.14.10)
274-274: Unused noqa directive (non-enabled: PLR0911)
Remove unused noqa directive
(RUF100)
🤖 Prompt for AI Agents
In scripts/check_schema_purity.py around line 274, the function definition
includes an unused noqa directive "# noqa: PLR0911"; remove this directive from
the function signature so the line no longer contains the noqa comment. Ensure
no other code relies on that directive and run the linter to confirm no PLR0911
suppression is required; if the PLR0911 rule is actually needed, instead enable
it in the ruff config rather than keeping the inline noqa.
| def print_violation(v: Violation) -> None: | ||
| """Print a violation in a readable format.""" | ||
| relative_path = v.file.relative_to(Path.cwd()) if v.file.is_absolute() else v.file | ||
| print(f" {relative_path}:{v.line}: [{v.category}] {v.message}") # noqa: T201 | ||
|
|
||
|
|
||
| def main() -> int: | ||
| """Run purity and naming checks on schema modules. | ||
|
|
||
| Returns: | ||
| Exit code: 0 if all checks pass, 1 if violations found | ||
|
|
||
| """ | ||
| project_root = Path(__file__).parent.parent | ||
|
|
||
| # Validate that schema directories exist | ||
| missing_dirs: list[str] = [] | ||
| for module_path in SCHEMA_MODULE_PATHS: | ||
| full_path = project_root / module_path | ||
| if not full_path.exists(): | ||
| missing_dirs.append(module_path) | ||
|
|
||
| if missing_dirs: | ||
| print(f"⚠️ Schema directories not found: {', '.join(missing_dirs)}") # noqa: T201 | ||
| print(" This may indicate a configuration error.") # noqa: T201 | ||
| return 1 | ||
|
|
||
| schema_files = find_schema_files(project_root) | ||
|
|
||
| if not schema_files: | ||
| print("⚠️ No schema files found to check") # noqa: T201 | ||
| return 0 | ||
|
|
||
| all_violations: list[Violation] = [] | ||
|
|
||
| print(f"Checking {len(schema_files)} schema files...") # noqa: T201 | ||
| print() # noqa: T201 | ||
|
|
||
| for file_path in schema_files: | ||
| # Check both purity and naming in a single parse | ||
| violations = check_file(file_path) | ||
| all_violations.extend(violations) | ||
|
|
||
| if all_violations: | ||
| print(f"❌ Found {len(all_violations)} violation(s):") # noqa: T201 | ||
| print() # noqa: T201 | ||
|
|
||
| # Group by category | ||
| by_category: dict[str, list[Violation]] = {} | ||
| for v in all_violations: | ||
| by_category.setdefault(v.category, []).append(v) | ||
|
|
||
| for category, violations in sorted(by_category.items()): | ||
| print(f" {category} ({len(violations)}):") # noqa: T201 | ||
| for v in violations: | ||
| print_violation(v) | ||
| print() # noqa: T201 | ||
|
|
||
| return 1 | ||
|
|
||
| print(f"✅ All {len(schema_files)} schema files passed purity and naming checks") # noqa: T201 | ||
| return 0 | ||
|
|
||
|
|
||
| if __name__ == "__main__": | ||
| sys.exit(main()) |
There was a problem hiding this comment.
Remove unused noqa directives for print statements.
Multiple # noqa: T201 directives are unused according to static analysis (RUF100), indicating that the T201 rule (print statements) is not enabled in your ruff configuration. Remove these directives on lines 505, 525, 526, 532, 537, 538, 546, 547, 555, 558, and 562.
🔎 Proposed fix pattern
Apply this pattern to all affected lines:
- print(f"Checking {len(schema_files)} schema files...") # noqa: T201
+ print(f"Checking {len(schema_files)} schema files...")Affected lines: 505, 525, 526, 532, 537, 538, 546, 547, 555, 558, 562
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def print_violation(v: Violation) -> None: | |
| """Print a violation in a readable format.""" | |
| relative_path = v.file.relative_to(Path.cwd()) if v.file.is_absolute() else v.file | |
| print(f" {relative_path}:{v.line}: [{v.category}] {v.message}") # noqa: T201 | |
| def main() -> int: | |
| """Run purity and naming checks on schema modules. | |
| Returns: | |
| Exit code: 0 if all checks pass, 1 if violations found | |
| """ | |
| project_root = Path(__file__).parent.parent | |
| # Validate that schema directories exist | |
| missing_dirs: list[str] = [] | |
| for module_path in SCHEMA_MODULE_PATHS: | |
| full_path = project_root / module_path | |
| if not full_path.exists(): | |
| missing_dirs.append(module_path) | |
| if missing_dirs: | |
| print(f"⚠️ Schema directories not found: {', '.join(missing_dirs)}") # noqa: T201 | |
| print(" This may indicate a configuration error.") # noqa: T201 | |
| return 1 | |
| schema_files = find_schema_files(project_root) | |
| if not schema_files: | |
| print("⚠️ No schema files found to check") # noqa: T201 | |
| return 0 | |
| all_violations: list[Violation] = [] | |
| print(f"Checking {len(schema_files)} schema files...") # noqa: T201 | |
| print() # noqa: T201 | |
| for file_path in schema_files: | |
| # Check both purity and naming in a single parse | |
| violations = check_file(file_path) | |
| all_violations.extend(violations) | |
| if all_violations: | |
| print(f"❌ Found {len(all_violations)} violation(s):") # noqa: T201 | |
| print() # noqa: T201 | |
| # Group by category | |
| by_category: dict[str, list[Violation]] = {} | |
| for v in all_violations: | |
| by_category.setdefault(v.category, []).append(v) | |
| for category, violations in sorted(by_category.items()): | |
| print(f" {category} ({len(violations)}):") # noqa: T201 | |
| for v in violations: | |
| print_violation(v) | |
| print() # noqa: T201 | |
| return 1 | |
| print(f"✅ All {len(schema_files)} schema files passed purity and naming checks") # noqa: T201 | |
| return 0 | |
| if __name__ == "__main__": | |
| sys.exit(main()) | |
| def print_violation(v: Violation) -> None: | |
| """Print a violation in a readable format.""" | |
| relative_path = v.file.relative_to(Path.cwd()) if v.file.is_absolute() else v.file | |
| print(f" {relative_path}:{v.line}: [{v.category}] {v.message}") | |
| def main() -> int: | |
| """Run purity and naming checks on schema modules. | |
| Returns: | |
| Exit code: 0 if all checks pass, 1 if violations found | |
| """ | |
| project_root = Path(__file__).parent.parent | |
| # Validate that schema directories exist | |
| missing_dirs: list[str] = [] | |
| for module_path in SCHEMA_MODULE_PATHS: | |
| full_path = project_root / module_path | |
| if not full_path.exists(): | |
| missing_dirs.append(module_path) | |
| if missing_dirs: | |
| print(f"⚠️ Schema directories not found: {', '.join(missing_dirs)}") | |
| print(" This may indicate a configuration error.") | |
| return 1 | |
| schema_files = find_schema_files(project_root) | |
| if not schema_files: | |
| print("⚠️ No schema files found to check") | |
| return 0 | |
| all_violations: list[Violation] = [] | |
| print(f"Checking {len(schema_files)} schema files...") | |
| print() | |
| for file_path in schema_files: | |
| # Check both purity and naming in a single parse | |
| violations = check_file(file_path) | |
| all_violations.extend(violations) | |
| if all_violations: | |
| print(f"❌ Found {len(all_violations)} violation(s):") | |
| print() | |
| # Group by category | |
| by_category: dict[str, list[Violation]] = {} | |
| for v in all_violations: | |
| by_category.setdefault(v.category, []).append(v) | |
| for category, violations in sorted(by_category.items()): | |
| print(f" {category} ({len(violations)}):") | |
| for v in violations: | |
| print_violation(v) | |
| print() | |
| return 1 | |
| print(f"✅ All {len(schema_files)} schema files passed purity and naming checks") | |
| return 0 | |
| if __name__ == "__main__": | |
| sys.exit(main()) |
🧰 Tools
🪛 Ruff (0.14.10)
505-505: Unused noqa directive (non-enabled: T201)
Remove unused noqa directive
(RUF100)
525-525: Unused noqa directive (non-enabled: T201)
Remove unused noqa directive
(RUF100)
526-526: Unused noqa directive (non-enabled: T201)
Remove unused noqa directive
(RUF100)
532-532: Unused noqa directive (non-enabled: T201)
Remove unused noqa directive
(RUF100)
537-537: Unused noqa directive (non-enabled: T201)
Remove unused noqa directive
(RUF100)
538-538: Unused noqa directive (non-enabled: T201)
Remove unused noqa directive
(RUF100)
546-546: Unused noqa directive (non-enabled: T201)
Remove unused noqa directive
(RUF100)
547-547: Unused noqa directive (non-enabled: T201)
Remove unused noqa directive
(RUF100)
555-555: Unused noqa directive (non-enabled: T201)
Remove unused noqa directive
(RUF100)
558-558: Unused noqa directive (non-enabled: T201)
Remove unused noqa directive
(RUF100)
562-562: Unused noqa directive (non-enabled: T201)
Remove unused noqa directive
(RUF100)
🤖 Prompt for AI Agents
In scripts/check_schema_purity.py around lines 502 to 567 the code contains
multiple unused "# noqa: T201" directives on print statements (lines 505, 525,
526, 532, 537, 538, 546, 547, 555, 558, 562) flagged by static analysis; remove
these redundant noqa comments from those lines so the print statements are
regular code without unnecessary suppression tokens, leaving the prints
themselves unchanged and running the file-level linter again to confirm no
remaining T201 suppressions are present.
- Remove len(violations) >= 2 check in favor of explicit violation category checks - Makes test clearer and more maintainable per review feedback - Addresses PLR2004 linting concern
Code Review for PR #8: CI Hardening - Purity/Naming/Determinism GatesSummaryThis is an excellent implementation of CI hardening for schema modules. The PR successfully implements AST-based purity checking, naming convention enforcement, and removes the previous JSON schema export workflow in favor of direct Pydantic model usage. The code quality is high, test coverage is comprehensive, and the implementation aligns well with the stated goals. ✅ Strengths1. Excellent Code Quality
2. Robust Testing
3. Smart Architectural Decisions
4. Good Documentation
🔍 Issues & SuggestionsHigh Priority1. Incomplete Alias Resolution for Nested Patterns (Minor Issue)Location: scripts/check_schema_purity.py:245-271 Issue: The import datetime as dt
x = dt.datetime.now() # May not be fully detectedCurrent Mitigation: The forbidden import itself is caught (defense-in-depth), and the test at line 232 shows this pattern IS detected via simplified matching (lines 165-181). Suggestion: Consider adding a comment in the code explaining that the simplified pattern matching at lines 169-181 handles most nested cases, and the import check provides additional coverage. Impact: Low - the current implementation catches most cases, and this is explicitly documented as acceptable. 2. Path Validation Logic Could Be SimplifiedLocation: scripts/check_schema_purity.py:428-436 Issue: The path validation logic in is_in_schema_dir = any(
str(resolved_path).startswith(str((project_root / module_path).resolve()))
for module_path in SCHEMA_MODULE_PATHS
)
validate_path = is_in_schema_dirSuggestion: Consider always validating paths in production code, and use a parameter or environment variable to disable validation only in test scenarios. This would simplify the logic and make it clearer. Impact: Low - current implementation works but could be clearer. Medium Priority3. Magic Numbers for Security ConstraintsLocation: scripts/check_schema_purity.py:13 Issue: The constant Suggestion: Extract to a named constant at module level for clarity: # Minimum parts needed for call pattern simplification (module.class.method)
_MIN_CALL_PARTS_FOR_SIMPLIFICATION = 3Impact: Very low - code clarity improvement. 4. Consider Adding a Dry-Run ModeLocation: scripts/check_schema_purity.py:508-563 Suggestion: Consider adding a Implementation: def main() -> int:
import argparse
parser = argparse.ArgumentParser()
parser.add_argument('--warn-only', action='store_true',
help='Report violations without failing')
args = parser.parse_args()
# ... existing code ...
if all_violations:
# ... print violations ...
return 0 if args.warn_only else 1Impact: Low - nice-to-have for adoption but not critical. 5. Test File OrganizationLocation: tests/test_check_schema_purity.py Observation: The test file is 399 lines and very comprehensive. Consider splitting into multiple files if it grows further:
Impact: Very low - current organization is fine, just a maintenance consideration for the future. Low Priority / Enhancements6. Violation Output FormatLocation: scripts/check_schema_purity.py:502-506 Suggestion: Consider adding color-coded output for better readability (using a library like def print_violation(v: Violation) -> None:
relative_path = v.file.relative_to(Path.cwd()) if v.file.is_absolute() else v.file
# Add color for category
category_color = '\033[91m' if v.category.startswith('forbidden') else '\033[93m'
reset_color = '\033[0m'
print(f" {relative_path}:{v.line}: [{category_color}{v.category}{reset_color}] {v.message}")Impact: Very low - cosmetic improvement. 7. Performance ConsiderationLocation: scripts/check_schema_purity.py:484-499 Observation: Files are processed sequentially. For very large codebases, consider parallel processing: from multiprocessing import Pool
def main() -> int:
# ... existing code ...
with Pool() as pool:
violations_per_file = pool.map(check_file, schema_files)
all_violations = [v for violations in violations_per_file for v in violations]Impact: Very low - current codebase is small, so this is premature optimization. 🔒 Security Assessment✅ Strong Security Posture
No Critical Security Issues Found📊 Test Coverage AssessmentExcellent Coverage (30 tests)Integration Tests (4 tests):
Purity Violation Detection (17 tests):
Naming Conventions (9 tests):
Missing Test Coverage (Minor Gaps)
Recommendation: Current test coverage is excellent for the PR scope. The minor gaps are acceptable. 🎯 Performance ConsiderationsCurrent Performance: Good
Future Optimizations (if needed):
Verdict: Performance is appropriate for current scale. 📋 Checklist Against PR Description
🚀 RecommendationsBefore Merging (Optional):
Future Enhancements (Post-Merge):
Breaking Change Impact:This PR removes the JSON schema export workflow and generated files. This is a breaking change for any downstream systems that were consuming the JSON schemas directly. Mitigation: The PR description and README clearly document the new approach (direct Pydantic model usage), which is actually superior to JSON schema consumption. Recommendation: Ensure all downstream systems are aware of this change. 🎉 Final VerdictAPPROVE ✅ This is high-quality work that successfully implements the stated goals with:
The minor issues noted above are truly minor and don't block merging. The team has demonstrated strong engineering practices, and the implementation is production-ready. Key Metrics:
Great work! 🎊 Reviewed by: Claude Code Assistant |
- Add detailed inline comment explaining nested alias pattern detection - Extract min_parts_for_simplification to module-level constant _MIN_PARTS_FOR_CALL_SIMPLIFICATION - Improves code readability and maintainability - Addresses optional pre-merge review suggestions
PR Review: CI Hardening - Purity/Naming/Determinism Gates [OMN-966]Executive Summary✅ Overall Assessment: Excellent work with minor recommendations This PR successfully implements comprehensive CI hardening with AST-based purity enforcement, naming conventions, and removes the JSON schema export workflow in favor of direct Pydantic model usage. The implementation is well-tested, secure, and aligns with project design principles. Code Quality & Best Practices✅ Strengths
|
…N-966] - Move scripts from scripts/ to src/onex_change_control/scripts/ - Update imports in tests to use new package location - Fix path resolution in check_schema_purity.py for module execution - Create reusable setup-poetry.yml workflow to reduce CI duplication - Update CI jobs to use reusable workflow - CLI entrypoints already configured in pyproject.toml Addresses: - Scripts Location (Medium) - Scripts now importable as package modules - Code Duplication in CI (Low) - Reusable workflow eliminates duplication Linear tickets created: - OMN-1041: Add --warn-only flag to check-schema-purity - OMN-1042: Add colored output to check-schema-purity - OMN-1043: Document version mapping policy
Pull Request Review: Schema Purity & Naming Gates [OMN-966]SummaryThis PR implements comprehensive CI hardening through AST-based purity enforcement and naming convention checks for schema modules. Overall, this is high-quality work with excellent test coverage and thoughtful implementation. The code is production-ready with only minor suggestions for improvement. ✅ Strengths1. Excellent Architecture & Design
2. Comprehensive Test Coverage
3. Security Considerations
4. Developer Experience
🟡 Issues & SuggestionsHIGH: GitHub Actions Workflow Composition IssueLocation: Problem: The # ci.yml line 15 - INCORRECT USAGE
- uses: ./.github/workflows/setup-poetry.yml
with:
python-version: "3.12"Why this is wrong:
Fix Option 1 - Composite Action (Recommended for this use case): # .github/actions/setup-poetry/action.yml
name: Setup Poetry
description: Install Python, Poetry, and dependencies
inputs:
python-version:
required: true
type: string
runs:
using: composite
steps:
- uses: actions/setup-python@v5
id: setup-python
with:
python-version: ${{ inputs.python-version }}
- name: Cache Poetry dependencies
uses: actions/cache@v4
with:
path: .venv
key: venv-${{ runner.os }}-${{ inputs.python-version }}-${{ hashFiles('**/poetry.lock') }}
- name: Install Poetry
uses: snok/install-poetry@v1
with:
version: 1.8.0
virtualenvs-create: true
virtualenvs-in-project: true
- name: Install dependencies
shell: bash
run: poetry install --no-interactionThen update - uses: ./.github/actions/setup-poetry
with:
python-version: "3.12"Fix Option 2 - Keep Reusable Workflow (More overhead): Recommendation: Use composite action (Option 1) - it's the standard pattern for reusable steps. MEDIUM: Missing checkout step in workflowLocation: Issue: The reusable workflow includes Fix: Remove checkout from the composite action (it's already in the calling workflow). LOW: Path validation logic could be simplifiedLocation: Current code: project_root = Path(__file__).parent.parent
resolved_path = file_path.resolve()
is_in_schema_dir = any(
str(resolved_path).startswith(str((project_root / module_path).resolve()))
for module_path in SCHEMA_MODULE_PATHS
)
validate_path = is_in_schema_dirSuggestion: Use project_root = Path(__file__).parent.parent
resolved_path = file_path.resolve()
is_in_schema_dir = any(
resolved_path.is_relative_to((project_root / module_path).resolve())
for module_path in SCHEMA_MODULE_PATHS
)
validate_path = is_in_schema_dirThis is more explicit and avoids string conversion. LOW: Magic number in call simplificationLocation: Current: _MIN_PARTS_FOR_CALL_SIMPLIFICATION = 3Suggestion: The constant name is good, but consider adding a docstring explaining why 3 is the magic number: # Minimum parts needed for simplification: module.class.method (e.g., datetime.datetime.now)
# Requires exactly 3 parts to extract meaningful class.method pattern
_MIN_PARTS_FOR_CALL_SIMPLIFICATION = 3The inline comment at lines 180-186 is excellent but could reference this constant. LOW: Test coverage gap - file permissionsLocation: Observation: Tests cover Suggestion: Add a test case: def test_handles_permission_error(self, tmp_path: Path) -> None:
"""Test that permission errors are properly reported."""
models_dir = tmp_path / "models"
models_dir.mkdir()
test_file = models_dir / "model_test.py"
test_file.write_text("class ModelTest: pass\n")
test_file.chmod(0o000) # Remove all permissions
violations = check_file(test_file)
assert any(v.category == "file_error" for v in violations)
assert any("Permission denied" in v.message for v in violations)
test_file.chmod(0o644) # Restore for cleanupDOCUMENTATION: README CLI usage is outdatedLocation: Current: poetry run python scripts/validate_yaml.py contracts/OMN-123.yamlShould be (now that CLI entrypoints are configured): poetry run validate-yaml contracts/OMN-123.yamlSame for 🔒 Security Review✅ Properly Handled:
|
Pull Request Review: CI Hardening for Schema Purity & Naming ConventionsSummaryThis PR successfully implements comprehensive CI hardening with purity enforcement, naming conventions, and determinism gates for schema modules. The implementation is well-architected with strong test coverage and good security practices. ✅ Strengths1. Excellent Code Quality
2. Strong Security Practices
3. Excellent Test Coverage
4. Smart Performance Optimizations
5. CI/CD Improvements
🔍 Issues & RecommendationsCritical IssuesNone found! 🎉 High Priority Recommendations1. Reusable Workflow Pattern Incorrect (.github/workflows/ci.yml:15, 29, 42)Issue: The Problem:
Fix: Either:
# .github/actions/setup-poetry/action.yml
name: Setup Poetry
description: Install Python and Poetry dependencies
inputs:
python-version:
required: true
runs:
using: composite
steps:
- uses: actions/setup-python@v5
# ... rest of steps with 'shell: bash' addedThen use it: - uses: ./.github/actions/setup-poetry
with:
python-version: "3.12"
# In ci.yml
jobs:
setup-poetry:
uses: ./.github/workflows/setup-poetry.yml
with:
python-version: "3.12"
lint:
needs: setup-poetry
runs-on: ubuntu-latest
steps:
# Can't reuse venv across jobs, needs reinstallRecommendation: Go with option 1 (composite action) for better reusability. 2. Test File Path Validation May Be Too PermissiveLocation: src/onex_change_control/scripts/check_schema_purity.py:443-451 Issue: The validation logic allows any file not in schema directories to bypass path validation: is_in_schema_dir = any(
str(resolved_path).startswith(str((project_root / module_path).resolve()))
for module_path in SCHEMA_MODULE_PATHS
)
validate_path = is_in_schema_dirConcern: This means test files in arbitrary locations won't have path traversal protection, which could be exploited if the tool is ever exposed to untrusted input. Fix: Always validate paths, but allow test directories explicitly: # Add to config
ALLOWED_PATHS = SCHEMA_MODULE_PATHS + ["tests/"]
# In _read_file_safely
if validate_path:
allowed_dirs = [
(project_root / path).resolve()
for path in ALLOWED_PATHS
]Medium Priority Recommendations3. Simplified Call Pattern Could Miss Edge CasesLocation: src/onex_change_control/scripts/check_schema_purity.py:178-196 Issue: The simplified pattern matching ( Example Edge Cases:
Recommendation: Document this limitation more explicitly in the code comment, or add a check to ensure the second-to-last part matches expected module names: # Only simplify if the second-to-last part is a known module
SIMPLIFIABLE_MODULES = {"datetime", "date", "os", "Path"}
if len(parts) >= 3 and parts[-2] in SIMPLIFIABLE_MODULES:
simplified = f"{parts[-2]}.{parts[-1]}"4. Missing Tests for os.environ Edge CasesCoverage Gap: The tests don't cover:
Recommendation: Add tests for these edge cases to ensure they're caught (or document why they're acceptable). 5. Documentation Could Be Clearer on Removal of JSON Schema ExportLocation: PR description mentions removing schemas/1.0.0/*.schema.json files Consideration: This is a significant architectural change. The PR description explains it well, but:
Low Priority Suggestions6. Magic Number for Simplification ThresholdLocation: src/onex_change_control/scripts/check_schema_purity.py:85 _MIN_PARTS_FOR_CALL_SIMPLIFICATION = 3Suggestion: This is well-documented but could benefit from a comment explaining why 3 specifically (module.class.method pattern). 7. Error Message ConsistencySome error messages use single quotes, some use double quotes:
Suggestion: Pick one style for consistency (double quotes recommended for Python). 🔒 Security Analysis✅ Good Security Practices
|
- Convert setup-poetry.yml from reusable workflow to composite action - Composite actions can be used as steps, reusable workflows cannot - Move from .github/workflows/ to .github/actions/setup-poetry/action.yml - Update CI jobs to use composite action syntax - Fixes CI failure: 'Can't find action.yml' error
Pull Request Review - PR #8Executive Summary✅ LGTM with minor suggestions - This is a well-architected implementation that adds critical CI hardening for schema purity enforcement. The code quality is excellent with comprehensive test coverage and strong security considerations. Recommendation: Approve with optional enhancements to consider for future iterations. Code Quality Assessment ⭐⭐⭐⭐⭐Strengths
Test Coverage Analysis ⭐⭐⭐⭐⭐Excellent coverage with 17 comprehensive tests for the purity checker: Coverage Highlights
Test Quality
Security Analysis ⭐⭐⭐⭐⭐Security Strengths
No Security Issues DetectedPerformance Considerations ⭐⭐⭐⭐½Optimizations Present
Minor Performance Suggestions (Optional)
Best Practices Adherence ⭐⭐⭐⭐⭐Excellent Adherence
Potential Issues & SuggestionsMinor Issues (Non-Blocking)
Enhancement Opportunities (Future Work)
Architectural Decisions - Excellent Choices1. Removal of JSON Schema Export ✅ ExcellentOld Approach: Generate JSON schemas, commit them, check for drift Benefits:
2. AST-Based Purity Checking ✅ ExcellentUsing Python's
3. Composite GitHub Action ✅ ExcellentReduces duplication and improves maintainability:
Breaking Changes ReviewChanges Requiring Attention
Impact: None - all changes are improvements or replacements of unused functionality Documentation Quality ⭐⭐⭐⭐Strengths
Suggestions
RecommendationsMust Fix (None)All critical functionality is implemented correctly. Should Consider
Nice to Have
Final Assessment
Overall Rating: ⭐⭐⭐⭐⭐ (5/5) ConclusionThis PR successfully implements CI hardening with purity and naming enforcement for schema modules. The implementation is production-ready with excellent code quality, comprehensive testing, and strong security considerations. The architectural decision to replace JSON schema export with direct Pydantic model usage is sound and simplifies the development workflow. The AST-based purity checker is well-designed and maintainable. Recommendation: ✅ Approve and merge The optional enhancements suggested above can be addressed in future iterations as needed. The current implementation provides solid value and meets all acceptance criteria stated in the PR description. Review completed by: Claude Sonnet 4.5 |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/onex_change_control/scripts/check_schema_purity.py (1)
289-289: Remove unused noqa directives.As previously flagged, the
# noqa: PLR0911directive on line 289 and multiple# noqa: T201directives throughout themain()function (lines 520, 543, 544, 550, 555, 556, 564, 565, 573, 576, 580) are unused because these rules are not enabled in your Ruff configuration. Remove them to keep the code clean.
🧹 Nitpick comments (2)
src/onex_change_control/scripts/check_schema_purity.py (1)
337-354: Consider a more precise path traversal check.The current check
if ".." in path_strcould theoretically flag legitimate filenames containing ".." (e.g.,foo..bar.py). While unlikely in practice for schema modules, a more precise check would use path components:🔎 Optional refinement
- path_str = str(file_path) - if ".." in path_str: + # Check for path traversal in path components (more precise than string check) + if ".." in file_path.parts:tests/test_check_schema_purity.py (1)
55-96: Consider a less fragile approach for testing missing directories.This test copies and modifies the script source by replacing a hardcoded string pattern. This approach is fragile because:
- It relies on exact formatting of
SCHEMA_MODULE_PATHSin the source- Changes to whitespace or structure will break the replacement
Consider using monkeypatching or environment-based configuration instead.
🔎 Alternative approach
One option is to refactor the script to accept
SCHEMA_MODULE_PATHSas a parameter or read from an environment variable during testing:# In check_schema_purity.py, allow override for testing import os SCHEMA_MODULE_PATHS = os.environ.get( "SCHEMA_MODULE_PATHS", "src/onex_change_control/models,src/onex_change_control/enums" ).split(",")Then the test can simply set the environment variable instead of modifying source code.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.github/actions/setup-poetry/action.yml.github/workflows/ci.ymlpyproject.tomlsrc/onex_change_control/scripts/__init__.pysrc/onex_change_control/scripts/check_schema_purity.pysrc/onex_change_control/scripts/validate_yaml.pytests/test_check_schema_purity.pytests/test_validate_yaml_cli.py
✅ Files skipped from review due to trivial changes (3)
- tests/test_validate_yaml_cli.py
- .github/actions/setup-poetry/action.yml
- src/onex_change_control/scripts/init.py
🧰 Additional context used
🧬 Code graph analysis (2)
src/onex_change_control/scripts/check_schema_purity.py (1)
src/onex_change_control/scripts/validate_yaml.py (1)
main(221-253)
tests/test_check_schema_purity.py (1)
src/onex_change_control/scripts/check_schema_purity.py (1)
check_file(402-496)
🪛 actionlint (1.7.9)
.github/workflows/ci.yml
15-15: could not parse action metadata in "/home/jailuser/git/.github/actions/setup-poetry": line 5: unexpected key "type" for definition of input "python-version"
(action)
🪛 Ruff (0.14.10)
src/onex_change_control/scripts/check_schema_purity.py
289-289: Unused noqa directive (non-enabled: PLR0911)
Remove unused noqa directive
(RUF100)
520-520: Unused noqa directive (non-enabled: T201)
Remove unused noqa directive
(RUF100)
543-543: Unused noqa directive (non-enabled: T201)
Remove unused noqa directive
(RUF100)
544-544: Unused noqa directive (non-enabled: T201)
Remove unused noqa directive
(RUF100)
550-550: Unused noqa directive (non-enabled: T201)
Remove unused noqa directive
(RUF100)
555-555: Unused noqa directive (non-enabled: T201)
Remove unused noqa directive
(RUF100)
556-556: Unused noqa directive (non-enabled: T201)
Remove unused noqa directive
(RUF100)
564-564: Unused noqa directive (non-enabled: T201)
Remove unused noqa directive
(RUF100)
565-565: Unused noqa directive (non-enabled: T201)
Remove unused noqa directive
(RUF100)
573-573: Unused noqa directive (non-enabled: T201)
Remove unused noqa directive
(RUF100)
576-576: Unused noqa directive (non-enabled: T201)
Remove unused noqa directive
(RUF100)
580-580: Unused noqa directive (non-enabled: T201)
Remove unused noqa directive
(RUF100)
⏰ 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)
.github/workflows/ci.yml (1)
46-56: LGTM!The new
schema-purityjob is well-structured with appropriate fail-fast behavior vianeeds: [test]. The consolidated setup and single purity check step simplify the CI workflow.src/onex_change_control/scripts/check_schema_purity.py (3)
97-207: LGTM!The
PurityCheckerAST visitor is well-implemented with comprehensive coverage for forbidden imports, calls, attribute access, and subscript access. The alias tracking and simplified pattern matching for nested aliases provide good defense-in-depth.
402-496: LGTM!The
check_filefunction is well-designed with single-pass parsing, proper handling of__init__.py, and allowance for private classes. The path validation toggle for test scenarios is pragmatic.
523-581: LGTM!The
mainfunction provides clear output with emoji indicators, proper exit codes, and grouped violation reporting. The directory existence validation at startup is a good fail-fast approach.src/onex_change_control/scripts/validate_yaml.py (1)
8-11: LGTM!The updated usage documentation correctly reflects the new CLI entry point (
poetry run validate-yaml) instead of direct script invocation, aligning with the package-based CLI approach configured inpyproject.toml.pyproject.toml (1)
14-17: LGTM!The CLI entry points are correctly configured, mapping the command names to their respective module main functions. This enables clean invocation via
poetry run validate-yamlandpoetry run check-schema-purity.tests/test_check_schema_purity.py (4)
39-54: LGTM!The integration tests verify the script runs successfully against existing schema files and reports appropriate output. Good coverage of the happy path.
98-248: LGTM!Excellent coverage of purity violation detection including forbidden imports, forbidden calls, aliased imports/calls, nested alias patterns, and allowed patterns. The tests clearly document edge cases and current behavior limitations.
249-314: LGTM!Good coverage of edge cases including
__future__imports, environment dictionary access via subscript, syntax error handling, and allowed Path operations. The explicit violation category checks (replacing magic numbers) improve test clarity.
316-407: LGTM!Comprehensive naming convention tests covering model/enum file prefixes, class prefixes,
__init__.pyskipping, and private class allowance. Well-structured test cases with clear assertions.
| - uses: actions/checkout@v4 | ||
| - uses: actions/setup-python@v5 | ||
| id: setup-python | ||
| - uses: ./.github/actions/setup-poetry | ||
| with: | ||
| python-version: "3.12" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat .github/actions/setup-poetry/action.ymlRepository: OmniNode-ai/onex_change_control
Length of output: 940
🌐 Web query:
GitHub Actions composite action inputs schema documentation
💡 Result:
Key docs and key points
-
Official metadata syntax (action.yml) — defines inputs as YAML mapping with name, description, required (boolean), and default (string). Composite actions use runs.using: "composite" and read inputs via the inputs context (not automatically exported as INPUT_… env vars). [1]
-
Creating a composite action tutorial — example action.yml and how to consume inputs in composite actions (showing default/required usage and how to reference inputs in steps). [2]
-
Limitations: inputs are treated as strings (no native array type); common workarounds use JSON or newline-separated strings parsed in the action. See community answers/discussion. [3]
References
- GitHub Docs — Metadata syntax reference (inputs/outputs for actions). [1]
- GitHub Docs — Creating a composite action (examples showing inputs usage). [2]
- StackOverflow — discussion about array input limitations for custom GitHub Actions. [3]
Remove the unsupported type key from the setup-poetry action's input definition.
The setup-poetry action uses type: string in its input definition, but composite actions do not support the type key for inputs. GitHub Actions composite action inputs only accept description, required, and default fields. Remove line 6 (type: string) from .github/actions/setup-poetry/action.yml to resolve the actionlint warning and ensure the action behaves correctly.
🧰 Tools
🪛 actionlint (1.7.9)
15-15: could not parse action metadata in "/home/jailuser/git/.github/actions/setup-poetry": line 5: unexpected key "type" for definition of input "python-version"
(action)
🤖 Prompt for AI Agents
In .github/workflows/ci.yml around lines 14–17 and in
.github/actions/setup-poetry/action.yml (input definition), the composite action
incorrectly declares an input using the unsupported key `type: string`; remove
the `type: string` line from the input definition in
.github/actions/setup-poetry/action.yml so the input only uses allowed keys
(description, required, default), save the file, and re-run actionlint/CI to
verify the warning is resolved.
- Add trailing newline to .github/actions/setup-poetry/action.yml - Fixes end-of-file-fixer pre-commit hook failure
Pull Request Review: CI Hardening - Purity/Naming/Determinism GatesSummaryThis PR successfully implements comprehensive CI hardening for schema modules per D-008 purity requirements. The implementation is high quality with excellent test coverage, security considerations, and performance optimizations. ✅ Strengths1. Excellent Code Quality
2. Outstanding Test Coverage
3. Smart Architectural Decisions
4. Excellent Documentation
🔍 Areas for Consideration1. Test Assertion Clarity (Minor)Consider adding assert messages for clearer test failures in test_detects_environment_dict_access 2. Alias Resolution (Documented Limitation)The PR acknowledges deeply nested alias patterns may not be fully detected. Current coverage is acceptable with defense-in-depth approach. 3. CI Job DependenciesConsider making schema-purity depend on [pre-commit, type-check, test] for consistency 4. Path Validation LogicPath validation is well-designed. Consider extracting duplicated logic at lines 446-451 to helper function. 🎯 Best Practices Observed
🔒 Security ConsiderationsStrengths:
Recommendation: Consider max file size check for DoS prevention 📊 PerformanceSingle-pass parsing optimization is excellent. Future enhancement: consider parallel processing for large codebases. 📝 Final VerdictAPPROVED ✅ This PR demonstrates:
Suggestions above are optional enhancements, not blocking. Ready to merge. Estimated Impact:
Great work! 🎉 Review by: Claude Code | Date: 2025-12-26 |
…ferred half of OMN-14462) (#4398) * feat(OMN-14776): scripts/merge-proof local-proof env wrapper (F-20 deferred half) Port the F-20 merge-proof local-proof env wrapper (OMN-14462) into onex_change_control. Resolves OMNI_HOME deterministically from the script's own on-disk location (rule #8) and fails fast with the exact export lines when it cannot; strips GIT_DIR/GIT_INDEX_FILE/GIT_WORK_TREE + PYTHONPATH from any delegated child env. Default run mode runs pre-commit run --all-files (OCC has no run_topic_lint.sh). Sentinel sibling = omnibase_infra. Extensionless scripts/merge-proof is not governed by the scripts-canonical-form scanner, so no scripts_exceptions.yaml entry is needed (matches omnimarket#1805 / omnibase_infra#2338 precedent). tests/unit/scripts/test_merge_proof.py (8 tests) drives the real script via subprocess in an isolated tmp location. Closes OMN-14776. * evidence(OMN-14776): OCC ticket contract + passing receipts for merge-proof wrapper Add contracts/OMN-14776.yaml (ModelTicketContract) with 3 above-L0 dod_evidence items and their passing receipts under drift/dod_receipts/OMN-14776/, satisfying OCC's required occ-preflight / eligibility + verify receipt gates for this repo's own PR. Receipts are hash-bound (contract_entry_sha256) and PR-bound (#4398). Evidence-Ticket: OMN-14776
1 participant
Summary
This PR implements CI hardening per D-008 purity requirements, adding gates to enforce:
Model*/model_*,Enum*/enum_*New CI Gates
Schema Purity & Naming Check (new job)
What it checks:
os,time,requests,socket,subprocess,random, etc.datetime.now(),os.getenv(),Path.cwd(),Path.home()model_*.pyfor models,enum_*.pyfor enumsModel*for model classes,Enum*for enum classesManifest Validation (added to schema-determinism)
What it validates:
export_script_versionis present for traceabilityNew Scripts
scripts/check_schema_purity.pyscripts/validate_manifest.pyTest Coverage
Acceptance Criteria
Ticket
Closes OMN-966
Summary by CodeRabbit
New Features
validate-yamlandcheck-schema-puritycommand-line tools for schema validation.Documentation
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.