Docs adr python - #40
Merged
Merged
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Both reviewed files have unresolved review findings, and the supplied assessments indicate changes are needed.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Documents the migration to Python/Pydantic validation and provides contributor guidance.
Changes:
- Adds ADR 0015 describing the migration decision and trade-offs.
- Adds Python setup, testing, schema, and maintenance guidance to
DEVELOPMENT.md.
File summaries
| File | Summary | Final review findings |
|---|---|---|
docs/adr/0015-migrate-validator-to-python.md |
Records the Python validator migration. | 5 nit comments: correct ADR numbering and index registration (1, 1, and 2 votes); accurately describe fixture-based validation and ProtocolFrontmatter (3 votes); add an Alternatives Considered section (1 vote). |
DEVELOPMENT.md |
Documents development and validation workflows. | 7 nit comments: clarify the Pydantic 2 requirement (3 votes), schema boundaries (3), baseline_dict (3), invalid template path (2), expected error regex (2), test-suite coverage (2), and fixture updates required for schema changes (1). |
Review details
Suppressed comments (6)
DEVELOPMENT.md:56
- This says validation rules can be tested entirely in memory, but document-level validation still reads fixture Markdown and several tests write temporary protocol files. Scope the advice to Pydantic frontmatter rules so contributors do not omit end-to-end coverage.
Because we use Pydantic, testing validation rules does not require writing markdown files to disk or dealing with fragile string manipulation. You test rules entirely in-memory by passing dictionaries to the models.
DEVELOPMENT.md:79
- A new required frontmatter field also invalidates the checked-in valid fixtures, especially
tests/fixtures/valid/basic/protocols, which the workflow validates directly. Updating only the two template protocols leaves CI failing; please document updating valid fixtures and content-repository protocols as part of the schema change.
If your schema change adds a new required field, ensure you also update `template/protocols/example-protocol/protocol.md` and `template/protocols/example-composite/protocol.md` so that future protocols scaffolded from the template do not immediately fail validation.
docs/adr/0015-migrate-validator-to-python.md:1
- This heading uses
ADR 1, but the repository numbers ADRs by the zero-padded record number (for example,# 0014...) and this file is 0015. Use# 0015...so references identify the same record.
# ADR 1: Migrate Protocol Validator to Python and Pydantic
docs/adr/0015-migrate-validator-to-python.md:4
- Unlike the existing ADRs and
docs/adr/template.md, this accepted ADR omits theDecidersmetadata and uses a different metadata format. Add the standard metadata, including the actual decision makers, so this record preserves decision provenance consistently.
**Date:** 2026-09-13
**Status:** Accepted
docs/adr/0015-migrate-validator-to-python.md:1
- The ADR index in
docs/adr/README.mdstops at 0014, so this accepted ADR is not discoverable from the repository's index. Add a 0015 entry alongside this file.
# ADR 1: Migrate Protocol Validator to Python and Pydantic
docs/adr/0015-migrate-validator-to-python.md:16
- Every existing ADR and the shared template includes an
## Alternatives Consideredsection before## Consequences; this record jumps directly to consequences, so it does not document the rejected options and trade-offs. Please add that section with the alternatives considered.
## Consequences
- Files reviewed: 2/2 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.
| ## Technical Stack | ||
| - **Language**: Python 3.10+ | ||
| - **Validation**: [Pydantic](https://docs.pydantic.dev/) for declarative schema definitions. | ||
| - **Testing**: [Pytest](https://docs.pytest.org/) for the in-memory test suite. |
| ## Local Setup | ||
| To run tests locally, install Python and the required dependencies: | ||
| ```bash | ||
| pip install pytest pydantic pyyaml |
|
|
||
| ## Modifying the Schema | ||
|
|
||
| The protocol schema is defined entirely in `scripts/models.py` using Pydantic. |
|
|
||
| def test_composite_requires_dependencies(): | ||
| # 1. Start with a valid baseline dictionary | ||
| bad_dict = {**baseline_dict} |
| bad_dict["protocols_used"] = [] | ||
|
|
||
| # 3. Assert that Pydantic rejects it with the expected error message | ||
| with pytest.raises(ValidationError, match="Composite protocols must declare 'protocols_used'"): |
| ``` | ||
|
|
||
| ## Updating the Template | ||
| If your schema change adds a new required field, ensure you also update `template/protocols/example-protocol/protocol.md` and `template/protocols/example-composite/protocol.md` so that future protocols scaffolded from the template do not immediately fail validation. |
| @@ -0,0 +1,25 @@ | |||
| # ADR 1: Migrate Protocol Validator to Python and Pydantic | |||
|
|
||
| ### Positive | ||
| * **Declarative Simplicity:** Complex validation logic (e.g., cross-field dependencies, type checking) is now handled natively by Pydantic. The core logic was reduced from ~800 lines of procedural R to ~160 lines of declarative Python. | ||
| * **Robust Testing:** Testing is now performed in-memory on Python dictionaries (e.g., `Protocol(**bad_dict)`), completely eliminating the need for fragile string manipulation and disk I/O. |
Member
Author
|
@copilot review |
Co-authored-by: lwaldron <639942+lwaldron@users.noreply.github.com>
Contributor
I applied a final clarification pass in |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ADR and documentation of new Python validation system