You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
Implement protocol tests: schema, validator, checker, and skill - #45
test.yaml schema as Pydantic models; the validator applies it to tests/ under every protocol, so content-repository CI checks tests with no workflow change.
scripts/check_test_result.py: --shape prints the result shape (values and nested keys withheld, table headers shown unless withheld) and writes source.yaml; the plain call compares each test with a result subdirectory, writes run.yaml, and exits 0, 1, or 2 as the standard says.
skills/test-protocol/SKILL.md: the executor side.
Fixtures and 18 pytest cases: schema in memory, validator on disk, checker end to end in a throwaway git repository.
The ADR moves to Accepted and the proposed notice leaves the standard. One wording fix in the run.yaml example: result is pass, fail, or not_run.
Next: reformat the eight fixtures in waldronlab/agent-protocols to this schema, then run the skill on them for the first run record.
ADR 0016 is accepted; this is the tooling it describes.
- scripts/models.py: ProtocolTest and FileRef, the test.yaml schema.
- scripts/validate_protocol.py: validates every tests/<name>/ under a
protocol, so CI in content repositories checks tests through the
existing action.
- scripts/check_test_result.py: --shape prints the result shape with
values and nested keys withheld and snapshots the source; the plain
call compares results and writes run.yaml. It is the only thing that
decides pass or fail.
- skills/test-protocol: the executor side of the result contract.
- Fixtures: valid/with-tests, eight invalid/test-* cases, and a worked
example in the template. 18 new pytest cases.
- Docs: the proposed notice is gone, README and DEVELOPMENT.md updated.
Drafted with Claude Code.
The reason will be displayed to describe this comment to others. Learn more.
⚠️Code review skipped — your organization has no extra usage available to pay for this review.
If your organization's extra usage balance is empty, an organization admin can add extra usage credits at claude.ai/admin-settings/usage. If its monthly spend limit was reached, an admin can raise it on the same page. If neither applies, contact Anthropic support.
Once extra usage is available, someone with write access to this repository can comment @claude review on this pull request to trigger a review.
… exit 2 on malformed input
After Copilot review. Also stop treating a section whose parameter is
named path or url as a file slot: file detection starts at each
section's values.
Drafted with Claude Code.
Preserve nested file descriptors in the generated shape
scripts/check_test_result.py:171
The schema accepts file references inside mappings and lists, and comparison recurses into them, but the shape drops their file format and header. For example, tables: {sample: {path: expected.csv}} becomes tables: {'<key>': 'mapping'}. An executor following the shape cannot discover the required CSV header without reading withheld expectations. Preserve nested file descriptors while masking data keys and values, or reject nested file references during validation.
Check forbidden results paths before creating directories
scripts/check_test_result.py:205
This creates the directory before checking whether its location is forbidden. Passing <protocol>/tests/results leaves an empty directory behind after rejection. Subsequent validation and --shape calls then fail because that directory has no test.yaml, even after choosing an external results directory. Check containment before creating anything.
Validate CSV row widths before comparison
scripts/check_test_result.py:244
Rows are never checked against the header width. Two files containing id,x followed by A incorrectly pass, while two containing A,1,2 raise an uncaught IndexError in compare_table and prevent the run record. Validate every row against the header width when reading a table. Treat malformed reference data as a contract error and malformed actual data as a per-test failure.
Preserve numeric precision when comparing CSV values
scripts/check_test_result.py:252
Parsing CSV cells as binary floats can produce false passes at zero tolerance: 9007199254740993 and 9007199254740992 become the same float before comparison. Preserve numeric precision through both parsing and comparison, while retaining tolerance handling and rejecting non-finite matches. Add a regression test for adjacent integers above the exact range of a float.
Record empty actual CSVs as per-test failures
scripts/check_test_result.py:264
A zero-byte executor CSV raises ContractError here, which escapes to main, returns exit 2, and prevents run.yaml from being written for any test. Invalid actual output should instead be recorded as a per-test failure, just like a missing result file. Catch table-format errors from the actual file separately, while keeping invalid reference data a contract error.
Reject result symlinks resolving outside the results directory
scripts/check_test_result.py:301
The lexical path check accepts a result file that is a symlink to an expected CSV outside the results directory. The checker then compares the expected file against itself and reports PASS without an independently produced output. Resolve the file path and require it to remain beneath the test's results subdirectory before reading it.
Require UTC timestamps in run records
scripts/check_test_result.py:410
Timestamp validation accepts date-only and timezone-free strings, and _stamp copies strings unchanged. As a result, both '2026-10-20' and a quoted timestamp with +02:00 can enter run.yaml even though the standard specifies UTC timestamps. Require timezone-aware UTC values for both strings and YAML datetime objects, or normalize timezone-aware strings as well as datetime objects before writing the record.
Verify remote input digests before execution
skills/test-protocol/SKILL.md:64
Remote inputs are passed unchanged in the shape, and the checker's verified downloader only handles expected outputs. These steps never require verifying an input's SHA-256, so execution on changed remote bytes can still receive PASS. The standard requires a digest mismatch to fail the test. Download remote inputs outside the protocol directory, verify them before execution, and record a failed test rather than using mismatched bytes.
File slots are direct values of a section; containment is checked
before the results directory is created; CSV rows must match the header
width, malformed expected tables are a contract error and malformed
result tables a per-test failure; cells compare as Decimals; result
files must resolve inside the result directory; timestamps must carry a
timezone and are written as UTC; --shape downloads and verifies remote
inputs before anything runs.
Drafted with Claude Code.
Malformed protocol frontmatter bypasses the contract-error handling: extract_frontmatter() wraps YAML parsing errors in a plain Exception, which main() does not catch. Both commands then terminate with a traceback and exit status 1, reserved for failed comparisons, instead of status 2. Translate the parsing exception into ContractError here.
Symlinked test directories bypass source change detection
scripts/check_test_result.py:101
Both test loaders accept symlinked test directories, but rglob does not descend into them here. Changing test.yaml inside such a directory leaves the source digest unchanged, so checking can use different expectations from those present at --shape without reporting a source change. Reject symlinked test directories consistently, or include their contents in a cycle-safe snapshot traversal. Add a regression case that edits a file through the directory symlink.
CSV reader accepts unterminated quoted fields
scripts/check_test_result.py:264
The default CSV reader accepts unterminated quoted fields. For example, id,x\n001,"3 parses like the valid table id,x\n001,3\n, allowing a truncated result to receive PASS. Enable strict parsing so malformed result tables fail and malformed expected tables reach the existing contract-error handling.
Mixed integer and float comparison loses precision
scripts/check_test_result.py:385
At tolerance 0, expected 9007199254740993 incorrectly matches actual 9007199254740992.0: mixed integer/float subtraction rounds the expected integer before computing the difference. Convert the operands to Decimal before comparing, using the existing _within helper, so unequal counts cannot receive PASS. Add a regression case for mixed numeric types beyond float precision.
If source.yaml contains a non-empty YAML list or scalar, loading succeeds but source.get() raises an uncaught AttributeError. The CLI then exits 1, which denotes a failed comparison, instead of contract-error status 2. Require a mapping here, as load_executor does.
Symlinked result directories allow stale outputs to pass
scripts/check_test_result.py:560
A test's result directory can be a symlink to a previous run's directory. is_dir() follows it, and the file-level containment check trusts that external directory as its root, allowing old outputs to receive PASS in this run. Check each resolved test directory against results_dir before reading its results.
Malformed frontmatter and a non-mapping source.yaml are contract
errors; symlinks under the protocol directory or in result
subdirectories are refused; CSV parsing is strict; mixed int and float
values compare as Decimals; a downloaded input is handed over as an
absolute path.
Drafted with Claude Code.
The reason will be displayed to describe this comment to others. Learn more.
🔵 Needs a closer look
Unresolved comparison, validation, and provenance issues can produce incorrect results or misleading run records.
No open issues 🎉
6 previously missed issues
In code that hasn't changed since last review
Ignored inputs are incorrectly attributed to a committed revision
scripts/check_test_result.py:132
git status omits ignored files, but compute_digest() includes every file under tests/. If a test uses an ignored, untracked input, the run records uncommitted: false and a commit that does not contain the executed data. Check for ignored files among the hashed sources before assigning a commit, and add a regression test for an ignored input.
Decimal subtraction and abs() use the default 28-digit context, so this can accept a difference outside the tolerance. A CSV value of 1.0000000000000000000000000001e-12 compared with 0 passes with tolerance 1e-12 because the difference rounds down. Use exact arithmetic for finite comparisons and add a boundary regression test.
Symlinked result manifests bypass result validation
scripts/check_test_result.py:419
A real result directory can contain a result.yaml symlink to a previous run. Both is_file() and read_text() follow it, so scalar-only tests or diagnostic refusals can pass using stale results despite the directory-level symlink guard. Reject symlinked result manifests before reading them, and cover this alongside the directory-symlink test.
HTTPS URL validation accepts invalid hostnames and ports
scripts/models.py:187
The prefix check accepts malformed URLs such as https:///input.csv and https://example.org:invalid/input.csv. These pass CI's schema validation but cannot be downloaded when the test runs. Validate the hostname and port as well as the HTTPS scheme.
Splitting on / before removing the query misidentifies valid remote filenames when a query parameter contains a slash. For example, https://example.org/expected.csv?token=a/b gets the name b, so the schema rejects this CSV reference. Extract the filename from the parsed URL path instead, and add coverage for query parameters containing slashes.
Unsupported YAML expectation types are not rejected
scripts/models.py:230
Non-file expectations are not checked for types the checker supports. For example, YAML loads expected_outputs: {day: 2026-10-18} as a date; it passes validation, but compare_value() rejects even an identical result as unsupported. Recursively reject unsupported expectation values for both exact and diagnostic tests, so malformed tests fail CI rather than producing unavoidable execution failures. Authors can quote dates to compare them as strings.
Ignored files under tests/ make a run uncommitted; tolerance
comparisons use exact rational arithmetic; a symlinked result.yaml
fails; URLs are parsed rather than prefix-matched; expectations may
only hold types the checker can compare.
Drafted with Claude Code.
Stray non-directory test entries are silently ignored
scripts/check_test_result.py:65
Non-directory entries are silently ignored here, while validate_tests_dir() rejects them. For example, the new invalid/test-stray-file fixture can pass this loader and produce a passing run record when its expected result is supplied, despite failing CI validation. Reject non-hidden stray files consistently with the validator instead of skipping them.
Symlinked tests root is not rejected
scripts/check_test_result.py:104
The symlink check covers descendants but not tests/ itself. A symlinked root is traversed without any ordinary child being identified as a symlink. If Git tracks that root link, a clean checkout can therefore attribute external test data to a commit that does not contain it. Reject a symlinked tests_dir before traversal, as already done for protocol.md and nested entries.
Invalid download URLs cause uncaught CLI errors
scripts/check_test_result.py:267
A URL such as https://example.org/my input.csv passes FileRef validation but raises http.client.InvalidURL when the request is constructed. An unencoded Unicode path similarly raises UnicodeEncodeError. Neither is caught by main, so the CLI emits a traceback and exits 1, which is reserved for failed comparisons, instead of contract-error status 2. Translate these download errors into ContractError.
It is an HTTPException, not a ValueError. The previous commit shipped
with this test failing because the pipeline hid the pytest exit status.
Drafted with Claude Code.
Reject overlapping expected outputs and absent keys
scripts/models.py:327
An outcome with expected_outputs: {n: 1} and expected_absent: [n] passes schema validation, but the checker fails it whether n is present or absent. Reject overlapping keys here so contradictory test definitions are reported as invalid tests rather than executor failures.
The reason will be displayed to describe this comment to others. Learn more.
🔵 Needs a closer look
Unresolved checker and validation defects affect source attribution, answer withholding, and valid test execution.
No open issues 🎉
4 previously missed issues
In code that hasn't changed since last review
Require the canonical tests directory for digest computation
scripts/check_test_result.py:61
The CLI accepts <protocol>/alternate-tests, but compute_digest() always hashes <protocol>/tests. Changes to the selected tests after --shape therefore go undetected, and run.yaml can attribute modified expectations to the original snapshot. Require the canonical tests/ directory, checking its normalized name so valid . arguments are not rejected.
Reject unterminated CSV headers with a ContractError
scripts/check_test_result.py:180
An unterminated quote in the CSV header makes this non-strict reader absorb subsequent expected data rows into the header. --shape then exposes those answers to the executor instead of rejecting the malformed test. Use the strict table reader and translate its TableError into a ContractError before printing the shape.
Normalize TESTS_DIR before deriving the protocol parent
scripts/check_test_result.py:646
Passing . as TESTS_DIR from inside a protocol's tests/ directory fails: Path('.').parent is still ., so both commands look for protocol.md inside tests/ and exit 2. Normalize the argument before deriving its parent, while retaining the explicit rejection of a symlinked tests root.
Reject symlinks in test roots and descendant files
scripts/validate_protocol.py:68
CI accepts symlinked test directories and local files because these checks follow symlinks. For example, a symlinked tests/basic/input.csv passes validation, but the checker's compute_digest() rejects it, so --shape cannot start that CI-green test. Reject symlinks during validation too, including the tests/ root and all descendants.
…hema
Files are {path: ...}, diagnostic fixtures carry their evidence under
accepted_outcomes, and the ad hoc compare_* and intended_* keys are gone.
Expected values are unchanged. Validates against the test schema in
waldronlab/agent-protocol-standard#45.
Drafted with Claude Code.
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
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.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
The tooling ADR 0016 (#43) describes. Closes #3.
Next: reformat the eight fixtures in waldronlab/agent-protocols to this schema, then run the skill on them for the first run record.
Drafted with Claude Code.