Rewrite validator in Python using Pydantic

[lwaldron]

No description provided.

[lwaldron]

@copilot review

[Copilot]

🟡 Changes recommended

The index action has a critical dependency failure, with additional validation and coverage issues unresolved.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

This pull request rewrites protocol validation and index generation from R to Python using Pydantic and PyYAML, migrating CI, actions, and tests.

Changes:

• Adds Python models, validation, repository helpers, and index generation.
• Migrates actions and CI to Python/pytest.
• Replaces legacy R entry points and tests.

File summaries

File	Summary and final findings
tests/test-repo-utils.R	Removes legacy repository-helper tests.
tests/test-malformed-values.R	Removes malformed-value tests.
tests/test-generator.R	Removes R generator tests.
tests/test_validate_protocol.py	Adds fixture validation tests. Nit (2 votes): Expected diagnostic assertions were dropped.
tests/test_repo_utils.py	Adds URL parsing tests. Nit (2 votes): Repository and ref detection coverage was dropped.
tests/test_models.py	Adds Pydantic model tests.
tests/run-tests.R	Removes the R test runner.
scripts/validate-protocol.R	Removes the R validator.
scripts/validate_protocol.py	Adds the Python validator. Moderate (3 votes): Malformed reviews input can raise TypeError. Moderate (3 votes): The unescaped ... alternative can terminate frontmatter incorrectly. Nit (1 vote): Documentation still references deleted R commands.
scripts/repo-utils.R	Removes R repository helpers.
scripts/repo_utils.py	Adds Python repository and ref helpers.
scripts/models.py	Adds Pydantic schemas. Moderate (3 votes): Explicit null method_origin_citation should be rejected. Moderate (2 votes): Explicit null type should be rejected.
scripts/generate-protocols-yaml.R	Removes the R index generator.
scripts/generate_protocols_yaml.py	Adds the Python index generator. Moderate (2 votes): Generator behavior lacks focused pytest coverage and workflow execution.
actions/validate-protocols/action.yml	Switches validation action to Python.
actions/generate-index/action.yml	Switches index generation action to Python. Critical (3 votes): The action does not install Pydantic, causing import failure.
.github/workflows/validate.yml	Migrates CI to Python and pytest.

Review details

Suppressed comments (2)

scripts/validate_protocol.py:75

• The frontmatter extractor has the same unescaped ... alternative, which matches any three characters rather than only the YAML document terminator. A three-character YAML line can truncate the parsed frontmatter; escape the dots.

    match = re.match(r'^---[\r\n]+(.*?[\r\n]+)(?:---|...)[\r\n]+', content, re.DOTALL)

scripts/validate_protocol.py:1

• This migration removes the R entry points while current user-facing documentation still instructs contributors to run deleted files: README.md:76,89, PROTOCOL_STANDARD.md:278, and template/CONTRIBUTING.md:55 (and to install R packages). After merging, the documented local validation workflow is broken; update those references to the Python commands and dependencies.

#!/usr/bin/env python3

• Files reviewed: 17/17 changed files
• Comments generated: 8
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

@copilot review

Addressed in d51000b: the generate-index action now installs pydantic alongside pyyaml, and I added pytest coverage for generate_protocols_yaml.py covering repository/ref URL generation, date serialization, and refusal-to-write when any protocol is unreadable.

[lwaldron]

@copilot review

[lwaldron]

@copilot review

[lwaldron]

@copilot review

[Copilot]

@copilot review

Addressed in 68637f0: fixed remaining validator/model edge cases (type: ~, method_origin_citation: ~, escaped ... frontmatter terminator, and invalid reviews shape handling), restored focused detect_repository/detect_ref tests, strengthened invalid fixture diagnostic assertions, and added tests/conftest.py so scripts.* imports resolve consistently in CI pytest runs.

[Copilot]

@copilot review

Reviewed current PR state on 68637f0 with no further code changes required. I re-ran python -m pytest tests/ -q locally and it passes (22 passed).

@copilot review

Re-reviewed after the latest updates: 68637f0 remains valid with no additional fixes needed, and python -m pytest tests/ -q passes (22 passed).

[Copilot]

🟡 Changes recommended

Unresolved moderate findings remain in validation, generation, dependencies, portability, documentation, and regression coverage.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (12)

scripts/generate_protocols_yaml.py:21

• Path.rglob() yields paths in no guaranteed order, so a repository with multiple protocols can produce a differently ordered PROTOCOLS.yaml across checkouts/runs, causing noisy commits and an unstable federation index. The previous R generator enumerated files in sorted order; sort this list before iterating.

    protocol_files = list(Path(protocols_dir).rglob("protocol.md"))

scripts/generate_protocols_yaml.py:46

• file_path is a Path, whose string form uses backslashes on Windows. Interpolating it directly creates protocol_url values such as .../protocols\\example\\protocol.md, which are not valid raw GitHub paths; serialize the path with POSIX separators.

        frontmatter['protocol_url'] = f"https://raw.githubusercontent.com/{repository_name}/{repository_ref}/{file_path}"

scripts/generate_protocols_yaml.py:23

• The Python replacement adds explicit safety branches for a missing protocols directory and for a directory containing no protocol.md, but the new test file never exercises either branch; the deleted R suite covered both cases. Please add tests asserting failure and no index output for each, otherwise a regression could publish an empty index unnoticed.

    if not os.path.isdir(protocols_dir):
        sys.exit(f"No '{protocols_dir}' directory found. Pass the protocols directory as the first argument.")
        
    protocol_files = list(Path(protocols_dir).rglob("protocol.md"))
    if not protocol_files:
        sys.exit(f"No 'protocol.md' files found under '{protocols_dir}'.")

scripts/generate_protocols_yaml.py:27

• The generator's repository-detection refusal is also new safety behavior, but the replacement tests always set GITHUB_REPOSITORY and never cover the no-repository case; the deleted R suite asserted that this path exits without writing an index. Add a test with no environment value and no usable git remote so this guard cannot regress.

    repository_name = detect_repository()
    if not repository_name:
        sys.exit("Could not determine which repository these protocols belong to. Set GITHUB_REPOSITORY to 'owner/name', or run this script inside a git checkout whose 'origin' remote points at the repository hosting them.")

scripts/models.py:1

• The implementation uses Pydantic 2-only APIs (field_validator, model_validator, and model_dump), but the action/workflow installs an unconstrained pydantic. If a runner already has Pydantic 1 installed, pip install pydantic can leave it in place and the validator fails at import time; declare the required major version consistently at every install site.

from pydantic import BaseModel, Field, field_validator, model_validator

scripts/validate_protocol.py:75

• This regex requires the closing ---/... marker to be followed by a newline, so valid frontmatter whose terminator is the file's final line (no trailing newline) is returned as None and rejected. The protocol format does not require a final newline; allow either a line ending or EOF after the terminator.

    match = re.match(r'^---[\r\n]+(.*?[\r\n]+)(?:---|\.\.\.)[\r\n]+', content, re.DOTALL)

scripts/validate_protocol.py:93

• This parser scans the raw file, while the structural checks above strip fenced code. A fenced example containing a ## History & Reviews heading and valid-looking entries can therefore satisfy the required history even when the protocol has no real history section. Parse the history from the same prose-with-fences-removed view, or explicitly ignore headings inside fences.

    with open(file_path, 'r', encoding='utf-8') as f:
        lines = [line.rstrip('\n') for line in f]
        
    heading_idx = [i for i, line in enumerate(lines) if re.match(r'^##[ \t]+History & Reviews[ \t]*$', line)]

scripts/validate_protocol.py:373

• When Pydantic rejects any frontmatter field, fm_model is set to None and this passes an empty list to validate_history. For fixtures such as invalid-status, the raw reviews array is otherwise valid, but every markdown review is then falsely reported as missing from frontmatter, adding misleading errors to the real validation failure. Preserve/validate the raw reviews for history comparison, or skip only that comparison when model parsing fails.

    fm_reviews = fm_model.reviews if fm_model else []
        
    history_ok = validate_history(file_path, raw_frontmatter, fm_reviews)

scripts/validate_protocol.py:91

• Both this read and the body read later in validate_protocol strip only \n, leaving \r on CRLF files. The heading and fence regexes then fail to recognize valid ---, ## Materials, ## Steps, and ## History & Reviews lines, so otherwise valid protocols fail on Windows/CRLF checkouts; normalize both reads to strip \r\n.

        lines = [line.rstrip('\n') for line in f]

scripts/validate_protocol.py:1

• This new entry point replaces the deleted scripts/validate-protocol.R, but the repository still documents that R command in README.md:76,89 and template/CONTRIBUTING.md:55 (and the standard/ADRs name it too). Those documented local instructions will fail after this merge; update the user-facing docs/templates alongside the rewrite or retain a compatibility wrapper.

#!/usr/bin/env python3

tests/test_validate_protocol.py:24

• The final fallback accepts any two four-or-more-character words from the expected text, so an unrelated diagnostic can satisfy an invalid fixture (for example, a message containing only name and string passes non-string-name). This weakens the fixture suite's guarantee that each rule is still enforced; use field-specific or normalized accepted messages rather than arbitrary word overlap.

    expected_words = [w for w in re.findall(r"[A-Za-z][A-Za-z0-9_-]*", expected.lower()) if len(w) >= 4]
    output_lower = output.lower()
    matches = sum(1 for w in expected_words if w in output_lower)
    return matches >= min(2, len(expected_words))

tests/test_validate_protocol.py:64

• test_template now checks only that validation fails, so the canonical starter can fail for a different reason and this test still passes. The removed R suite asserted the placeholder-citation diagnostic and that no other errors or crashes occurred; preserve that assertion here so template drift remains detectable.

            assert not validate_protocol(str(p), str(template_dir))

• Files reviewed: 19/30 changed files
• Comments generated: 4
• Review effort level: Lite

[lwaldron]

Yes this should be fixed

[lwaldron]

@copilot review

[lwaldron]

@copilot review

[Copilot]

@copilot review

Addressed in cc0c654: updated review-status diagnostics to include the reviewer name, fixed the ref-detection test to set GITHUB_EVENT_NAME=push, tightened fixture diagnostic matching to explicit normalizations, and added parametrized malformed-shape regression tests for scalar/container frontmatter fields.

@copilot review

Addressed in cc0c654: updated review-status diagnostics to include the reviewer name, fixed the ref-detection test to set GITHUB_EVENT_NAME=push, tightened fixture diagnostic matching to explicit normalizations, and added parametrized malformed-shape regression tests for scalar/container frontmatter fields.

[lwaldron]

Don't maintain backwards compatibility - this is pre-release

[lwaldron]

Don't maintain backwards compatibility - this is pre-release

[lwaldron]

Yes this should be fixed

[Copilot]

@copilot review

Addressed in 07ed516: removed deprecated r-version backward-compatibility inputs from both composite actions and updated README action-input docs to keep only the supported python-version input.

[lwaldron]

@copilot review

[Copilot]

@copilot review

Re-reviewed current PR state after 07ed516: no further changes are needed from my side. The README/test coverage section reflects the Python test modules, deprecated r-version compatibility was removed, and python -m pytest tests/ -q passes (77 passed).

[lwaldron]

@copilot review

[Copilot]

🟡 Changes recommended

Exact date validation, frontmatter delimiter handling, and stale ADR references remain unresolved.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (3)

PROTOCOL_STANDARD.md:278

• This updates the canonical standard reference, but the migration leaves dead entry points in docs/adr/0001-protocol-standard.md, docs/adr/0005-protocol-history-and-reviews.md, and docs/adr/0006-separate-standard-from-protocol-content.md, which still refer to the deleted R validator/generator. Please update those ADR references (and any remaining repository references) so the documentation does not direct readers to scripts that no longer exist.

These rules are enforced by `scripts/validate_protocol.py`, which runs on every pull request.

scripts/models.py:84

• The protocol date validator has the same canonical-format hole: date.fromisoformat() accepts compact/week-date strings under Python 3.11, and the later str(date) comparison normalizes them so they can be accepted despite the standard requiring YYYY-MM-DD. Apply an explicit full-match check before parsing (and cover it with a regression case).

        if isinstance(v, str):
            try:
                return date.fromisoformat(v)
            except ValueError as exc:
                raise ValueError("Must be YYYY-MM-DD date") from exc

scripts/validate_protocol.py:75

• The closing-delimiter alternative is not anchored to the beginning of a line, so any --- or ... text after a newline can terminate the frontmatter. For example, an indented --- inside a valid YAML block-scalar description is treated as the end marker, truncating the mapping and causing valid protocols to be rejected (or incomplete metadata to be generated); anchor the delimiter with multiline matching.

    match = re.match(r'^---[ \t]*[\r\n]+(.*?[\r\n]+)(?:---|\.\.\.)[ \t]*(?:[\r\n]|$)', content, re.DOTALL)

• Files reviewed: 22/33 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

@copilot review

Addressed in e1ff4da: switched generated index timestamps to timezone-aware UTC (datetime.now(UTC)) to eliminate the remaining Python compatibility warning. Re-ran python -m pytest tests/ -q locally (77 passed) and the follow-up validation review found no issues.