ci: add recursive pass validation gates#114
Conversation
|
Workflow file was intentionally placed at Action required at merge: Move If this PR needs to be reverted during merge/review, undo that path placement. |
|
❌ PR Too Large 📋 Best Practices for Large Changes
🚫 This PR is blocked from merging until size limits are met. |
📝 WalkthroughWalkthroughA comprehensive schema validation system for GMP-RSP-001 artifacts is introduced. A Python utility script provides multiple validation modes (YAML sections, completeness, halt codes, schema versions, quality logs, drift detection), while a GitHub Actions workflow orchestrates multi-pass CI gates that validate spec completeness, section presence, and cross-file consistency across recursive-pass YAML files, culminating in pytest and artifact collection. ChangesSchema Validation Framework & CI Orchestration
Sequence DiagramsequenceDiagram
participant GH as GitHub
participant WF as Workflow
participant Validate as validate_schema.py
participant Specs as YAML Specs
participant Tests as pytest
participant Artifacts as Artifacts
GH->>WF: Trigger on push/PR/dispatch
WF->>WF: Preflight: Verify files exist
WF->>Validate: validate_schema check_completeness (pass-0)
Validate->>Specs: Load recursive-pass-0.yaml
Specs-->>Validate: YAML data
Validate->>Validate: Check required sections
Validate-->>WF: PASS or FAIL
WF->>Validate: validate_schema check_completeness (pass-1)
Validate->>Specs: Load recursive-pass-1.yaml
Specs-->>Validate: YAML data
Validate-->>WF: PASS or FAIL
WF->>Validate: validate_schema check_completeness (pass-2)
Validate->>Specs: Load recursive-pass-2.yaml
Specs-->>Validate: YAML data
Validate-->>WF: PASS or FAIL
WF->>Validate: validate_schema check_pass_chain
Validate->>Specs: Load all pass YAMLs
Specs-->>Validate: YAML data
Validate->>Validate: Verify schema_version consistency
Validate-->>WF: Chain integrity result
WF->>Validate: validate_schema drift_check (pass-0 ↔ pass-1)
Validate->>Specs: Load both YAMLs
Specs-->>Validate: YAML data
Validate->>Validate: Compare contract keys
Validate-->>WF: Drift report
WF->>Tests: pytest execution
Tests-->>WF: Test results & exit code
WF->>Artifacts: Upload pytest results
WF->>WF: Collect CI evidence
WF->>Artifacts: Upload ci_evidence_report.json
Artifacts-->>GH: Evidence stored
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 39f41a657e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| @@ -0,0 +1,625 @@ | |||
| # ci/workflows/recursive-pass-ci.yml | |||
There was a problem hiding this comment.
This workflow is added under github/workflows/ instead of .github/workflows/, so GitHub Actions will not discover or run it on push, pull_request, or workflow_dispatch. In that state, all validation gates introduced here are silently bypassed and cannot protect main.
Useful? React with 👍 / 👎.
| "${{ env.CI_DIR }}/required_sections_pass0.txt" \ | ||
| "${{ env.CI_DIR }}/shared_contracts.yaml" \ | ||
| "${{ env.CI_DIR }}/halt_codes_pass0.txt" \ | ||
| "${{ env.SPECS_DIR }}/recursive-pass-0.yaml" \ | ||
| "${{ env.SPECS_DIR }}/recursive-pass-1.yaml" \ |
There was a problem hiding this comment.
The preflight step requires artifacts like ci/required_sections_pass0.txt, specs/recursive-pass-0.yaml, and docs/pass_quality_log.yaml, but those files are not present in this commit; once this workflow is moved to .github/workflows/, preflight will fail immediately on every run and block all PRs. The gate should either include these artifacts in the same change or guard the checks behind an onboarding mode.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (3)
ci/validate_schema.py (1)
374-424: ⚡ Quick winConsider replacing the hand-rolled CLI dispatch with
argparse.SonarCloud flags cognitive complexity at 23 (limit 15) for
main, driven by repeatedlen(args)checks and string-mode comparisons.argparsesubparsers (or even adict[str, Callable]lookup) would also give you--helpfor free, which the workflow's preflight step at line 90 currently tries to exercise but only succeeds because of the trailing|| true.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ci/validate_schema.py` around lines 374 - 424, The main function currently does manual argv parsing and many len(args)/mode checks; replace this with argparse subparsers (or a dict[str, Callable] dispatcher) to reduce cognitive complexity: create an ArgumentParser in main with subparsers for the modes and required positional arguments for each subcommand, map subcommands to the existing handlers (check_section, check_quality_log, check_completeness, check_halt_codes, check_pass_chain, check_schema_version, drift_check), call the appropriate handler with parsed args, and use parser.error or _fail for uniform error exits so you can remove the repeated len(args) checks and manual error strings while preserving exit behavior.github/workflows/recursive-pass-ci.yml (2)
38-46: 💤 Low value
warn_onlyinput is declared but never wired in.The footer at lines 621–624 admits
warn_only"does not suppress gate failures in v2 — it is provided as a documentation signal only". Operators dispatching the workflow withwarn_only: truewill reasonably expect non-blocking behavior and be surprised when gates still fail the run. Either drop the input or actually use it — for example by gating each blocking step onif: inputs.warn_only != 'true', or wrapping withcontinue-on-error: ${{ inputs.warn_only == 'true' }}.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@github/workflows/recursive-pass-ci.yml` around lines 38 - 46, The workflow input warn_only is declared but not used; either remove it or wire it into the blocking steps so dispatching with warn_only: "true" produces non-blocking behavior. Fix by locating the workflow_dispatch inputs block (warn_only) and then for each gate/blocking job or step that should be non-blocking when warned, add one of two options: 1) conditionally skip the blocking behavior with if: ${{ inputs.warn_only != 'true' }} on the blocking step/job (so it only runs when warn_only is false), or 2) allow failures by adding continue-on-error: ${{ inputs.warn_only == 'true' }} to those steps; apply consistently to all gate steps referenced in the workflow so the input actually toggles blocking vs warn-only behavior.
113-347: ⚖️ Poor tradeoffOptional: factor the repeated checkout/setup/pip block into a composite action.
Every gate job repeats the same five-step preamble (
actions/checkout@v4→actions/setup-python@v5→pip install pyyaml). A composite action under.github/actions/setup-validator/action.yml(or a reusable workflow) would reduce ~70 lines of duplication and eliminate the risk that one job drifts (e.g. forgetscache: pipor installs a different dep set). Not blocking — file is functional as-is.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@github/workflows/recursive-pass-ci.yml` around lines 113 - 347, The CI file repeats the same checkout/setup-python/pip install pyyaml preamble across jobs like schema-completeness-pass0, schema-completeness-pass1, schema-completeness-pass2, observability-section-present, principal-hierarchy-present, etc.; create a composite action (e.g. .github/actions/setup-validator/action.yml) that performs actions/checkout@v4, actions/setup-python@v5 with a python-version input and pip cache, and runs pip install pyyaml, then update each job to replace the repeated steps with a single step using your composite action (pass through inputs like python-version and any env vars) so each job calls the composite action instead of duplicating the three steps.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ci/validate_schema.py`:
- Around line 303-311: When validating entries, first ensure each entry is a
mapping before checking required fields: in the loop that iterates over entries
(the variable entries and QUALITY_LOG_FIELDS), add a type guard such as "if not
isinstance(entry, Mapping): errors.append(f'entry[{i}] must be a mapping
(CONTRACT_INTEGRITY_VIOLATION)'); continue" so non-dict entries fail loudly
instead of performing membership checks; otherwise proceed to check missing
fields as before. Also remove the redundant "FAIL:" prefix from the message
passed into _fail (call site currently embeds "FAIL:"), leaving only the
descriptive message so the final output doesn't repeat "FAIL:".
- Around line 128-140: The list-comprehension opens sidecar files without
closing them; replace direct open(...) usage with a safe read pattern — e.g.,
use Path(required_file).read_text().splitlines() (or wrap with with
open(required_file) as f: f.readlines()) when building `required`, and apply the
same fix for `halt_codes_file` in `check_halt_codes` and `contract_keys_file` in
`drift_check` so those file handles are not leaked; update the comprehension
that builds `required` and the analogous comprehensions in the
`check_halt_codes` and `drift_check` code paths to use the chosen safe-read
approach.
- Around line 44-108: Add explicit type annotations to every function signature
in this module: annotate parameters and return types for load_yaml, _fail,
is_non_empty, get_nested (and also for the other functions referenced in the
review: check_section, check_completeness, check_halt_codes, check_pass_chain,
_parse_semver, check_schema_version, check_quality_log, drift_check, main) using
appropriate types (e.g., path: str -> dict | None for loaders, msg: str for
_fail, v: Any -> bool for is_non_empty, data: Mapping[str, Any], keys:
Sequence[str] -> Any | None for get_nested, etc.), and add
Any/Mapping/Sequence/Optional imports as needed; then run ruff auto-fixes (ruff
format / ruff check --fix) to sort/format imports to resolve I001. Ensure
mypy/strict passes after adding the annotations.
- Around line 62-75: The _fail function should be annotated with typing.NoReturn
so callers like load_yaml are inferred as non-returning; add "from typing import
NoReturn" (or update existing typing imports) and change the signature of _fail
to include "-> NoReturn" (keep parameters _fail(msg, prefix="ERROR") ->
NoReturn) so mypy --strict no longer treats exception branches as returning
None.
In `@github/workflows/recursive-pass-ci.yml`:
- Around line 84-91: The CI step currently runs python ${{ env.VALIDATE_SCRIPT
}} --help which is a no-op and masked by the trailing || true; either add a
proper --help handler to validate_schema.py (e.g. use argparse and implement a
--help/--mode docstring around the main entry referenced near
validate_schema.py:374) so the script returns 0 for --help, or remove the flaky
smoke test and replace it with a robust module-import smoke test that executes
the file without masking errors (use importlib.util.spec_from_file_location +
spec.loader.exec_module to load ${{ env.VALIDATE_SCRIPT }}), and ensure you drop
the trailing || true so real import/Syntax errors fail the workflow.
- Around line 1-2: The workflow file named recursive-pass-ci.yml is currently
placed outside GitHub's auto-discovery directory; move the file into the
repository's workflows directory used by GitHub Actions (so Actions will pick up
the workflow), update the top-of-file comment that currently reads "#
ci/workflows/recursive-pass-ci.yml" to reflect the new location, and update the
SETUP reference mentioned around the file's SETUP section (line referenced in
the review) so it points to the new location; after committing the move, verify
the workflow appears and runs in the Actions UI and that branch-protection jobs
(e.g., "Full test suite — tests/test_pass_invariants.py") resolve correctly.
- Around line 51-53: Update the CI environment variable PYTHON_VERSION from
"3.11" to "3.12" in the workflow env block so the jobs run under the project's
Python baseline; keep VALIDATE_SCRIPT unchanged. Locate the env section
containing PYTHON_VERSION and replace its value to "3.12" (or higher if desired)
so mypy/validate_schema.py and other lint/type checks execute with Python 3.12
semantics.
- Around line 514-542: The inline Python step uses datetime.datetime.utcnow()
(deprecated/naive), a bare except, and open(path) without a context manager;
update imports to include timezone (or use datetime.now(tz=timezone.utc)) and
replace datetime.datetime.utcnow().isoformat() with a timezone-aware call (e.g.,
datetime.now(timezone.utc).isoformat() + "Z"); open each spec file with a
context manager (with open(path) as f: data = yaml.safe_load(f)); tighten error
handling by catching specific exceptions (e.g., yaml.YAMLError and OSError)
instead of bare except Exception as e and record their messages into
report["specs"][name].
---
Nitpick comments:
In `@ci/validate_schema.py`:
- Around line 374-424: The main function currently does manual argv parsing and
many len(args)/mode checks; replace this with argparse subparsers (or a
dict[str, Callable] dispatcher) to reduce cognitive complexity: create an
ArgumentParser in main with subparsers for the modes and required positional
arguments for each subcommand, map subcommands to the existing handlers
(check_section, check_quality_log, check_completeness, check_halt_codes,
check_pass_chain, check_schema_version, drift_check), call the appropriate
handler with parsed args, and use parser.error or _fail for uniform error exits
so you can remove the repeated len(args) checks and manual error strings while
preserving exit behavior.
In `@github/workflows/recursive-pass-ci.yml`:
- Around line 38-46: The workflow input warn_only is declared but not used;
either remove it or wire it into the blocking steps so dispatching with
warn_only: "true" produces non-blocking behavior. Fix by locating the
workflow_dispatch inputs block (warn_only) and then for each gate/blocking job
or step that should be non-blocking when warned, add one of two options: 1)
conditionally skip the blocking behavior with if: ${{ inputs.warn_only != 'true'
}} on the blocking step/job (so it only runs when warn_only is false), or 2)
allow failures by adding continue-on-error: ${{ inputs.warn_only == 'true' }} to
those steps; apply consistently to all gate steps referenced in the workflow so
the input actually toggles blocking vs warn-only behavior.
- Around line 113-347: The CI file repeats the same checkout/setup-python/pip
install pyyaml preamble across jobs like schema-completeness-pass0,
schema-completeness-pass1, schema-completeness-pass2,
observability-section-present, principal-hierarchy-present, etc.; create a
composite action (e.g. .github/actions/setup-validator/action.yml) that performs
actions/checkout@v4, actions/setup-python@v5 with a python-version input and pip
cache, and runs pip install pyyaml, then update each job to replace the repeated
steps with a single step using your composite action (pass through inputs like
python-version and any env vars) so each job calls the composite action instead
of duplicating the three steps.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7ee75dac-8508-4b8a-93c9-0473d9be1f86
📒 Files selected for processing (2)
ci/validate_schema.pygithub/workflows/recursive-pass-ci.yml
| import sys | ||
| import re | ||
| import yaml | ||
|
|
||
|
|
||
| # ── Required quality log entry fields ────────────────────────────────────── | ||
| QUALITY_LOG_FIELDS = [ | ||
| "pass_id", | ||
| "timestamp_utc", | ||
| "gap_ids_addressed", | ||
| "gate_results", | ||
| "regression_count", | ||
| "improvement_delta", | ||
| "notes", | ||
| ] | ||
|
|
||
|
|
||
| # ── YAML loading ──────────────────────────────────────────────────────────── | ||
| def load_yaml(path): | ||
| try: | ||
| with open(path) as f: | ||
| return yaml.safe_load(f) | ||
| except FileNotFoundError: | ||
| _fail(f"not found: {path}") | ||
| except yaml.YAMLError as e: | ||
| _fail(f"YAML parse error in {path}: {e}") | ||
|
|
||
|
|
||
| def _fail(msg, prefix="ERROR"): | ||
| print(f"{prefix}: {msg}", file=sys.stderr) | ||
| sys.exit(1) | ||
|
|
||
|
|
||
| # ── Value helpers ─────────────────────────────────────────────────────────── | ||
| def is_non_empty(v): | ||
| """ | ||
| Return True if v is a non-None, non-empty value. | ||
|
|
||
| FIX (v2): Previously, `float` values including 0.0 fell through to the | ||
| final `return True` branch, which is correct — 0.0 IS a defined value. | ||
| The bug was that `isinstance(v, (str, list, dict))` did NOT match float, | ||
| so 0.0 returned True correctly in v1 as well. The actual bug was that | ||
| integer 0 and boolean False also returned True (correct), but improvement_delta | ||
| of exactly 0.0 in the quality log is a semantically meaningful value | ||
| that SHOULD be accepted, not rejected. No change needed for 0.0. | ||
|
|
||
| The real fix: ensure None → False, empty str/list/dict → False, | ||
| and all other defined values (including 0, 0.0, False) → True. | ||
| This is the correct semantics for "field is present and has a value". | ||
| """ | ||
| if v is None: | ||
| return False | ||
| if isinstance(v, (str, list, dict)) and len(v) == 0: | ||
| return False | ||
| return True | ||
|
|
||
|
|
||
| def get_nested(data, keys): | ||
| """Traverse nested dict by key list. Returns None if any key is missing.""" | ||
| node = data | ||
| for k in keys: | ||
| if not isinstance(node, dict) or k not in node: | ||
| return None | ||
| node = node[k] | ||
| return node |
There was a problem hiding this comment.
Add type hints to every function signature (coding-guideline violation).
None of the functions in this file (load_yaml, _fail, is_non_empty, get_nested, check_section, check_completeness, check_halt_codes, check_pass_chain, _parse_semver, check_schema_version, check_quality_log, drift_check, main) have parameter or return-type annotations. Combined with mypy --strict engine/, this will block CI/merge. While here, ruff also flags I001 (import block unsorted) — fix with ruff format/ruff check --fix.
♻️ Suggested signatures (illustrative)
-import sys
-import re
-import yaml
+import re
+import sys
+from typing import Any, NoReturn
+
+import yaml
@@
-def load_yaml(path):
+def load_yaml(path: str) -> Any:
@@
-def _fail(msg, prefix="ERROR"):
+def _fail(msg: str, prefix: str = "ERROR") -> NoReturn:
@@
-def is_non_empty(v):
+def is_non_empty(v: Any) -> bool:
@@
-def get_nested(data, keys):
+def get_nested(data: Any, keys: list[str]) -> Any:
@@
-def check_section(section_path, filepath):
+def check_section(section_path: str, filepath: str) -> None:
@@
-def check_completeness(filepath, required_file):
+def check_completeness(filepath: str, required_file: str) -> None:
@@
-def check_halt_codes(filepath, halt_codes_file):
+def check_halt_codes(filepath: str, halt_codes_file: str) -> None:
@@
-def check_pass_chain(filepaths):
+def check_pass_chain(filepaths: list[str]) -> None:
@@
-def _parse_semver(v):
+def _parse_semver(v: object) -> tuple[int, int, int] | None:
@@
-def check_schema_version(filepath, minimum_version):
+def check_schema_version(filepath: str, minimum_version: str) -> None:
@@
-def check_quality_log(filepath):
+def check_quality_log(filepath: str) -> None:
@@
-def drift_check(file_a, file_b, contract_keys_file):
+def drift_check(file_a: str, file_b: str, contract_keys_file: str) -> None:
@@
-def main():
+def main() -> None:As per coding guidelines: "Type hints on every function signature, class attribute, ambiguous variable" and "Add type hints to all function parameters and return types in Python files".
🧰 Tools
🪛 GitHub Check: Lint & Format (Ruff + MyPy)
[failure] 96-98: ruff (SIM103)
ci/validate_schema.py:96:5: SIM103 Return the negated condition directly
help: Inline condition
[failure] 64-64: ruff (PTH123)
ci/validate_schema.py:64:14: PTH123 open() should be replaced by Path.open()
help: Replace with Path.open()
[failure] 44-46: ruff (I001)
ci/validate_schema.py:44:1: I001 Import block is un-sorted or un-formatted
help: Organize imports
🪛 GitHub Check: Refactoring Validation Gate
[failure] 96-98: ruff (SIM103)
ci/validate_schema.py:96:5: SIM103 Return the negated condition directly
help: Inline condition
[failure] 64-64: ruff (PTH123)
ci/validate_schema.py:64:14: PTH123 open() should be replaced by Path.open()
help: Replace with Path.open()
[failure] 44-46: ruff (I001)
ci/validate_schema.py:44:1: I001 Import block is un-sorted or un-formatted
help: Organize imports
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ci/validate_schema.py` around lines 44 - 108, Add explicit type annotations
to every function signature in this module: annotate parameters and return types
for load_yaml, _fail, is_non_empty, get_nested (and also for the other functions
referenced in the review: check_section, check_completeness, check_halt_codes,
check_pass_chain, _parse_semver, check_schema_version, check_quality_log,
drift_check, main) using appropriate types (e.g., path: str -> dict | None for
loaders, msg: str for _fail, v: Any -> bool for is_non_empty, data: Mapping[str,
Any], keys: Sequence[str] -> Any | None for get_nested, etc.), and add
Any/Mapping/Sequence/Optional imports as needed; then run ruff auto-fixes (ruff
format / ruff check --fix) to sort/format imports to resolve I001. Ensure
mypy/strict passes after adding the annotations.
| def load_yaml(path): | ||
| try: | ||
| with open(path) as f: | ||
| return yaml.safe_load(f) | ||
| except FileNotFoundError: | ||
| _fail(f"not found: {path}") | ||
| except yaml.YAMLError as e: | ||
| _fail(f"YAML parse error in {path}: {e}") | ||
|
|
||
|
|
||
| def _fail(msg, prefix="ERROR"): | ||
| print(f"{prefix}: {msg}", file=sys.stderr) | ||
| sys.exit(1) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Type _fail as NoReturn so callers correctly infer non-return.
_fail always calls sys.exit(1), but without NoReturn, mypy --strict will treat callers like load_yaml as falling through to an implicit None return on the except branches, polluting the inferred return type and propagating Optional everywhere downstream. Annotating _fail removes that noise.
-def _fail(msg, prefix="ERROR"):
+def _fail(msg: str, prefix: str = "ERROR") -> NoReturn:
print(f"{prefix}: {msg}", file=sys.stderr)
sys.exit(1)As per coding guidelines: "Linter: ruff check + mypy --strict engine/".
🧰 Tools
🪛 GitHub Check: Lint & Format (Ruff + MyPy)
[failure] 64-64: ruff (PTH123)
ci/validate_schema.py:64:14: PTH123 open() should be replaced by Path.open()
help: Replace with Path.open()
🪛 GitHub Check: Refactoring Validation Gate
[failure] 64-64: ruff (PTH123)
ci/validate_schema.py:64:14: PTH123 open() should be replaced by Path.open()
help: Replace with Path.open()
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ci/validate_schema.py` around lines 62 - 75, The _fail function should be
annotated with typing.NoReturn so callers like load_yaml are inferred as
non-returning; add "from typing import NoReturn" (or update existing typing
imports) and change the signature of _fail to include "-> NoReturn" (keep
parameters _fail(msg, prefix="ERROR") -> NoReturn) so mypy --strict no longer
treats exception branches as returning None.
| try: | ||
| required = [ | ||
| line.strip() | ||
| for line in open(required_file) | ||
| if line.strip() and not line.startswith("#") | ||
| ] | ||
| except FileNotFoundError: | ||
| _fail(f"required sections file not found: {required_file}") | ||
|
|
||
| missing = [ | ||
| s for s in required | ||
| if not is_non_empty(get_nested(data, s.split("."))) | ||
| ] |
There was a problem hiding this comment.
Use a context manager when reading the sidecar text files (resource leak + ruff failures).
open(required_file) (line 131), open(halt_codes_file) (line 176), and open(contract_keys_file) (line 336) are passed directly to a list comprehension and never explicitly closed. CPython will close them eventually via refcount, but PyPy/long-running tests will leak FDs, and ruff is currently failing with SIM115 and PTH123 on all three sites. Switch to Path(...).read_text().splitlines() (or a with block) — same fix in check_halt_codes and drift_check.
♻️ Proposed pattern
+from pathlib import Path
@@
- try:
- required = [
- line.strip()
- for line in open(required_file)
- if line.strip() and not line.startswith("#")
- ]
- except FileNotFoundError:
- _fail(f"required sections file not found: {required_file}")
+ try:
+ lines = Path(required_file).read_text(encoding="utf-8").splitlines()
+ except FileNotFoundError:
+ _fail(f"required sections file not found: {required_file}")
+ required = [s.strip() for s in lines if s.strip() and not s.lstrip().startswith("#")]🧰 Tools
🪛 GitHub Check: Lint & Format (Ruff + MyPy)
[failure] 131-131: ruff (PTH123)
ci/validate_schema.py:131:25: PTH123 open() should be replaced by Path.open()
help: Replace with Path.open()
[failure] 131-131: ruff (SIM115)
ci/validate_schema.py:131:25: SIM115 Use a context manager for opening files
🪛 GitHub Check: Refactoring Validation Gate
[failure] 131-131: ruff (PTH123)
ci/validate_schema.py:131:25: PTH123 open() should be replaced by Path.open()
help: Replace with Path.open()
[failure] 131-131: ruff (SIM115)
ci/validate_schema.py:131:25: SIM115 Use a context manager for opening files
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ci/validate_schema.py` around lines 128 - 140, The list-comprehension opens
sidecar files without closing them; replace direct open(...) usage with a safe
read pattern — e.g., use Path(required_file).read_text().splitlines() (or wrap
with with open(required_file) as f: f.readlines()) when building `required`, and
apply the same fix for `halt_codes_file` in `check_halt_codes` and
`contract_keys_file` in `drift_check` so those file handles are not leaked;
update the comprehension that builds `required` and the analogous comprehensions
in the `check_halt_codes` and `drift_check` code paths to use the chosen
safe-read approach.
| entries = data.get("entries") | ||
| if not isinstance(entries, list): | ||
| errors.append("entries must be a list") | ||
| else: | ||
| for i, entry in enumerate(entries): | ||
| for field in QUALITY_LOG_FIELDS: | ||
| if field not in entry: | ||
| errors.append(f"entry[{i}] missing required field {field!r}") | ||
|
|
There was a problem hiding this comment.
Validate that each entry is a mapping before checking required fields.
If a quality-log entry is malformed (e.g. a string or list instead of a dict), field not in entry silently does substring/membership matching against the wrong type and the validator reports PASS. Add a type guard so non-dict entries fail loudly with CONTRACT_INTEGRITY_VIOLATION-style errors instead of being accepted.
entries = data.get("entries")
if not isinstance(entries, list):
errors.append("entries must be a list")
else:
for i, entry in enumerate(entries):
+ if not isinstance(entry, dict):
+ errors.append(f"entry[{i}] must be a mapping, got {type(entry).__name__}")
+ continue
for field in QUALITY_LOG_FIELDS:
if field not in entry:
errors.append(f"entry[{i}] missing required field {field!r}")Side note: line 296 calls _fail("FAIL: quality log root must be a mapping", prefix="FAIL"), which prints FAIL: FAIL: .... Drop the embedded FAIL: from the message.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ci/validate_schema.py` around lines 303 - 311, When validating entries, first
ensure each entry is a mapping before checking required fields: in the loop that
iterates over entries (the variable entries and QUALITY_LOG_FIELDS), add a type
guard such as "if not isinstance(entry, Mapping): errors.append(f'entry[{i}]
must be a mapping (CONTRACT_INTEGRITY_VIOLATION)'); continue" so non-dict
entries fail loudly instead of performing membership checks; otherwise proceed
to check missing fields as before. Also remove the redundant "FAIL:" prefix from
the message passed into _fail (call site currently embeds "FAIL:"), leaving only
the descriptive message so the final output doesn't repeat "FAIL:".
| # ci/workflows/recursive-pass-ci.yml | ||
| # GMP-RSP-001 | L9 GMP v3.2.0 |
There was a problem hiding this comment.
Blocker: workflow must live at .github/workflows/recursive-pass-ci.yml, not github/workflows/.
GitHub Actions only auto-discovers workflow files under the dotfile directory .github/workflows/. Merging this file at github/workflows/recursive-pass-ci.yml will silently ship dead YAML — none of the gates (preflight, completeness, halt-codes, drift, unit-tests, evidence) will ever run, and branch-protection rules referring to the job name Full test suite — tests/test_pass_invariants.py (per the SETUP step 6) will never resolve. The PR description acknowledges this and proposes renaming on merge; please ensure the rename actually lands and is verified in the Actions tab on main post-merge, otherwise the entire CI gate is a no-op. Also update the leading comment on line 1 (# ci/workflows/recursive-pass-ci.yml) and SETUP line 559 once moved.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@github/workflows/recursive-pass-ci.yml` around lines 1 - 2, The workflow file
named recursive-pass-ci.yml is currently placed outside GitHub's auto-discovery
directory; move the file into the repository's workflows directory used by
GitHub Actions (so Actions will pick up the workflow), update the top-of-file
comment that currently reads "# ci/workflows/recursive-pass-ci.yml" to reflect
the new location, and update the SETUP reference mentioned around the file's
SETUP section (line referenced in the review) so it points to the new location;
after committing the move, verify the workflow appears and runs in the Actions
UI and that branch-protection jobs (e.g., "Full test suite —
tests/test_pass_invariants.py") resolve correctly.
| env: | ||
| PYTHON_VERSION: "3.11" # single override point; update here to change all jobs | ||
| VALIDATE_SCRIPT: ci/validate_schema.py |
There was a problem hiding this comment.
Bump PYTHON_VERSION to 3.12 (or higher) to match project language baseline.
The validation script will be linted/typed under the project's mypy --strict and may use 3.12-only typing constructs (e.g. type aliases, PEP 695 generics) over time; keeping CI on 3.11 risks drift between local and CI behavior, and contradicts the stated language target.
- PYTHON_VERSION: "3.11" # single override point; update here to change all jobs
+ PYTHON_VERSION: "3.12" # single override point; update here to change all jobsAs per coding guidelines: "Language: Python 3.12+" and "Python 3.12+ with async/await for all I/O operations".
📝 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.
| env: | |
| PYTHON_VERSION: "3.11" # single override point; update here to change all jobs | |
| VALIDATE_SCRIPT: ci/validate_schema.py | |
| env: | |
| PYTHON_VERSION: "3.12" # single override point; update here to change all jobs | |
| VALIDATE_SCRIPT: ci/validate_schema.py |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@github/workflows/recursive-pass-ci.yml` around lines 51 - 53, Update the CI
environment variable PYTHON_VERSION from "3.11" to "3.12" in the workflow env
block so the jobs run under the project's Python baseline; keep VALIDATE_SCRIPT
unchanged. Locate the env section containing PYTHON_VERSION and replace its
value to "3.12" (or higher if desired) so mypy/validate_schema.py and other
lint/type checks execute with Python 3.12 semantics.
| - name: Verify validate_schema.py exists | ||
| run: | | ||
| if [ ! -f "${{ env.VALIDATE_SCRIPT }}" ]; then | ||
| echo "❌ Missing: ${{ env.VALIDATE_SCRIPT }}" >&2 | ||
| exit 1 | ||
| fi | ||
| python ${{ env.VALIDATE_SCRIPT }} --help 2>/dev/null || true | ||
| echo "✅ validate_schema.py present" |
There was a problem hiding this comment.
Preflight --help invocation is effectively a no-op.
validate_schema.py has no --help handler — passing it falls into the else branch and exits 1 with unknown mode: '--help'. The trailing || true then masks this and would also mask a real SyntaxError / ImportError, so the only thing this step actually verifies is file existence (already covered by the [ ! -f ... ] check). Either implement --help in the script (recommended; pairs naturally with the argparse refactor noted on validate_schema.py:374) or replace the smoke test with something that actually exits non-zero on a broken script, e.g. python -c "import importlib.util, sys; spec=importlib.util.spec_from_file_location('vs','${{ env.VALIDATE_SCRIPT }}'); m=importlib.util.module_from_spec(spec); spec.loader.exec_module(m)".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@github/workflows/recursive-pass-ci.yml` around lines 84 - 91, The CI step
currently runs python ${{ env.VALIDATE_SCRIPT }} --help which is a no-op and
masked by the trailing || true; either add a proper --help handler to
validate_schema.py (e.g. use argparse and implement a --help/--mode docstring
around the main entry referenced near validate_schema.py:374) so the script
returns 0 for --help, or remove the flaky smoke test and replace it with a
robust module-import smoke test that executes the file without masking errors
(use importlib.util.spec_from_file_location + spec.loader.exec_module to load
${{ env.VALIDATE_SCRIPT }}), and ensure you drop the trailing || true so real
import/Syntax errors fail the workflow.
| python - << 'EOF' | ||
| import yaml, json, datetime, pathlib | ||
|
|
||
| specs = { | ||
| "pass-0": "specs/recursive-pass-0.yaml", | ||
| "pass-1": "specs/recursive-pass-1.yaml", | ||
| "pass-2": "specs/recursive-pass-2.yaml", | ||
| } | ||
| report = { | ||
| "generated_utc": datetime.datetime.utcnow().isoformat() + "Z", | ||
| "run_id": "${{ github.run_id }}", | ||
| "commit": "${{ github.sha }}", | ||
| "specs": {}, | ||
| } | ||
| for name, path in specs.items(): | ||
| try: | ||
| data = yaml.safe_load(open(path)) | ||
| report["specs"][name] = { | ||
| "schema_version": data.get("schema_version", "MISSING"), | ||
| "yaml_valid": True, | ||
| } | ||
| except Exception as e: | ||
| report["specs"][name] = {"yaml_valid": False, "error": str(e)} | ||
|
|
||
| pathlib.Path("ci_evidence_report.json").write_text( | ||
| json.dumps(report, indent=2) | ||
| ) | ||
| print("Evidence report written.") | ||
| EOF |
There was a problem hiding this comment.
Replace datetime.utcnow() — deprecated in 3.12 and violates project datetime guideline.
datetime.datetime.utcnow() returns a naive datetime and is deprecated in Python 3.12; once PYTHON_VERSION is bumped this will emit DeprecationWarning and (eventually) be removed. Use timezone-aware UTC instead. While here, the bare except Exception and the open(path) without a context manager are also worth tightening since the same script is generating an evidence artifact that downstream consumers rely on.
- import yaml, json, datetime, pathlib
+ import json
+ import pathlib
+ from datetime import UTC, datetime
+
+ import yaml
@@
- report = {
- "generated_utc": datetime.datetime.utcnow().isoformat() + "Z",
+ report = {
+ "generated_utc": datetime.now(tz=UTC).isoformat(),
"run_id": "${{ github.run_id }}",
"commit": "${{ github.sha }}",
"specs": {},
}
for name, path in specs.items():
try:
- data = yaml.safe_load(open(path))
+ with pathlib.Path(path).open(encoding="utf-8") as f:
+ data = yaml.safe_load(f)
report["specs"][name] = {
"schema_version": data.get("schema_version", "MISSING"),
"yaml_valid": True,
}
- except Exception as e:
+ except (OSError, yaml.YAMLError) as e:
report["specs"][name] = {"yaml_valid": False, "error": str(e)}As per coding guidelines: "Always use datetime.now(tz=UTC) for timezone-aware datetime operations".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@github/workflows/recursive-pass-ci.yml` around lines 514 - 542, The inline
Python step uses datetime.datetime.utcnow() (deprecated/naive), a bare except,
and open(path) without a context manager; update imports to include timezone (or
use datetime.now(tz=timezone.utc)) and replace
datetime.datetime.utcnow().isoformat() with a timezone-aware call (e.g.,
datetime.now(timezone.utc).isoformat() + "Z"); open each spec file with a
context manager (with open(path) as f: data = yaml.safe_load(f)); tighten error
handling by catching specific exceptions (e.g., yaml.YAMLError and OSError)
instead of bare except Exception as e and record their messages into
report["specs"][name].
|
Workflow file was intentionally moved to github/workflows/recursive-pass-ci.yml without the leading dot. If this PR needs to be reverted during merge/review, undo that path placement. When ready to activate GitHub Actions, rename it to .github/workflows/recursive-pass-ci.yml. |
1 participant
Summary
Adds recursive pass CI validation gates and schema validation support.
Files
ci/validate_schema.pygithub/workflows/recursive-pass-ci.ymlNotes
The workflow file is currently placed at
github/workflows/recursive-pass-ci.yml(without the leading dot) due to GitHub App token permission restrictions on.github/workflows/.Fix when merging: Rename
github/workflows/recursive-pass-ci.yml→.github/workflows/recursive-pass-ci.ymlso GitHub Actions can discover it.If this PR needs to be undone during merge/review, revert the workflow placement.
Verification
Summary by CodeRabbit