From 6a045750d6fc1116a76f074ae8438f7e390b5be5 Mon Sep 17 00:00:00 2001 From: Levi Waldron Date: Fri, 23 Oct 2026 09:26:42 -0400 Subject: [PATCH 1/3] Conditions on materials: placement rule, must/should tiers, expected_warning ADR 0017 (Proposed). Materials is the single home for conditions on inputs; must stops before Step 1, should warns and proceeds. Diagnostic outcomes may carry expected_warning; result.yaml may carry warnings. Runner and test-protocol skills check Materials before Step 1. Closes #46 Drafted with Claude Code. Co-Authored-By: Claude Fable 5.1 --- PROTOCOL_STANDARD.md | 35 ++++++- docs/adr/0017-conditions-on-materials.md | 91 +++++++++++++++++++ docs/adr/README.md | 1 + scripts/check_test_result.py | 16 +++- scripts/models.py | 9 +- skills/protocol-runner/SKILL.md | 14 ++- skills/test-protocol/SKILL.md | 22 +++-- .../protocols/example-protocol/protocol.md | 6 ++ .../tests/orientation/test.yaml | 1 + tests/test_protocol_tests.py | 50 ++++++++++ 10 files changed, 225 insertions(+), 20 deletions(-) create mode 100644 docs/adr/0017-conditions-on-materials.md diff --git a/PROTOCOL_STANDARD.md b/PROTOCOL_STANDARD.md index 0e925c8..f9ebedb 100644 --- a/PROTOCOL_STANDARD.md +++ b/PROTOCOL_STANDARD.md @@ -158,6 +158,23 @@ The markdown body must contain a `## Materials` section and a `## Steps` section `### Step` heading, and `## History & Reviews` must be the last section. A document with metadata but no materials and no steps is not a protocol, however complete its frontmatter is, and CI rejects it. +`## Materials` lists every input the protocol consumes (data, parameters, software, reference data) +and every condition an input must or should satisfy. Steps consume materials; they do not introduce +new conditions on inputs. Statements about what the protocol does or does not do belong in the +overview or `## Notes`, not in Materials. + +A condition on a material is written with **must** or **should**, in the sense of RFC 2119, and an +executor checks every condition before Step 1: + +* A condition written with **must** is a requirement. An executor that finds it violated stops, + reports which condition failed, and produces none of the protocol's outputs. +* A condition written with **should** is an advisory. The executor proceeds, warns the user naming + the condition and its consequence, and records the warning with the execution. + +This applies to conditions written in a protocol's Materials section. The standard's own prose uses +these words in their ordinary sense. A check that can only be made during computation, such as a +sample whose total is zero, belongs in the step where it arises. + The remaining structure is a recommendation rather than a requirement, and follows it for compatibility with future export tools such as protocols.io integration: @@ -168,6 +185,11 @@ Brief overview (1-2 sentences). ## Materials +- **Input data:** + - `input table` — what it is, and its conditions: columns must be ..., values should be ...; + if they are not, warn that ... +- **Parameters:** + - `theta` — what it controls, its default, and the values it must take. - **Software & Repositories:** - `ToolName` ([Repository URL]) — Description/version. - **Databases & Reference Data:** @@ -350,9 +372,12 @@ least one of: an executor that refused did not produce the protocol's outputs. An outcome with neither, or with both empty, is invalid, because a label alone is the executor's -word. The keys -`outcome` and `observation` are reserved for the executor's report and may appear in neither -`expected_outputs` nor `expected_absent`; the validator rejects a test that uses them. +word. An outcome may add `expected_warning`, a phrase that must appear, case-insensitively, in +one of the executor's `warnings`. This is how a **should** condition in Materials is checked: the +executor proceeded, produced the outputs, and said what was wrong with the input. A warning is +also the executor's word, so it accompanies evidence rather than replacing it. The keys +`outcome`, `observation`, and `warnings` are reserved for the executor's report and may appear in +neither `expected_outputs` nor `expected_absent`; the validator rejects a test that uses them. ```yaml evaluation: @@ -362,6 +387,7 @@ evaluation: expected_outputs: removed_features: [A, B] retained_features: [C, D, E] + expected_warning: orientation - outcome: refused_for_orientation expected_absent: [removed_features, retained_features, retained_matrix] ``` @@ -396,6 +422,9 @@ both. The executor does not read `expected_outputs` itself. * In `diagnostic` mode, `result.yaml` has `outcome`, one of the accepted outcomes, and `observation`, one sentence saying what was found. If that outcome carries `expected_outputs`, their keys follow. +* In either mode, `result.yaml` may have `warnings`, a list of strings, one per warning the + executor gave because a **should** condition in Materials was not met. It is required when an + outcome carries `expected_warning`. `scripts/check_test_result.py ` compares every test that has a result subdirectory, or one with `--test `, and is the only thing that decides pass or fail. A diff --git a/docs/adr/0017-conditions-on-materials.md b/docs/adr/0017-conditions-on-materials.md new file mode 100644 index 0000000..cf24725 --- /dev/null +++ b/docs/adr/0017-conditions-on-materials.md @@ -0,0 +1,91 @@ +# 0017. Conditions on Materials: Where They Live and What a Violation Means + +- **Status:** Proposed +- **Date:** 2026-10-23 +- **Deciders:** Levi Waldron (User), AI Agent + +## Context and Problem Statement + +Reviewing a Shannon diversity protocol (waldronlab/agent-protocols#53), the rule about rarefaction +appeared in three places: a Materials bullet, a sentence in Step 2, and a Notes paragraph. None of +them said what an executor should do with a count table that had not been rarefied. The author had +no rule to follow: the standard required a `## Materials` heading and recommended two bullet +groups, software and reference data, and said nothing about where conditions on inputs belong. + +Existing protocols had converged on a convention the standard never stated: conditions written as +prose under Materials, then a "Step 1: Validate" restating them. Steps could contain anything. +Diagnostic tests referred to "a requirement the protocol states" without saying where, and could +check only a refusal, through `expected_absent`. There was no way for a protocol to say "proceed, +but warn", which is what the rarefaction case needs: the index is computable, and the user needs to +know the values are not comparable across samples. + +Issue #46 records the discussion. + +## Decision + +**Materials is the single home for conditions on inputs.** It lists every input the protocol +consumes (data, parameters, software, reference data) and every condition an input must or should +satisfy. Steps consume materials and do not introduce new conditions on them. Statements about what +the protocol does or does not do belong in the overview or Notes. + +**Two severity tiers, written with RFC 2119 words.** A condition written with *must* is a +requirement: an executor that finds it violated stops, reports which condition failed, and produces +none of the protocol's outputs. A condition written with *should* is an advisory: the executor +proceeds, warns the user naming the condition and its consequence, and records the warning with the +execution. + +**The check happens before Step 1.** Every condition in Materials is checked against the inputs +before the first step runs, so a protocol no longer needs a validation step for anything Materials +already states. A check that can only be made during computation, such as a sample whose total is +zero, stays in the step where it arises. + +**Scope.** The RFC 2119 reading applies to conditions written in a protocol's Materials section. +The standard's own prose, which already uses *must* and *should* in their ordinary sense throughout, +is not reinterpreted. + +**The test contract gains the warning tier.** A diagnostic outcome may carry `expected_warning`, a +phrase that must appear in the executor's `warnings`. It accompanies evidence (`expected_outputs` +or `expected_absent`) rather than replacing it, since a warning is the executor's word as much as +the outcome label is. `result.yaml` may carry `warnings`, a list of strings, in either mode; +`warnings` joins `outcome` and `observation` as a reserved key. The result shape shows that +`warnings` exists and withholds the phrase, so an executor warns because the protocol said to, not +because the shape did. + +The protocol-runner skill checks Materials before Step 1 and lists warnings with the departures. +The test-protocol skill writes them into `result.yaml`. + +## Alternatives Considered + +- **One tier: every condition is a requirement.** Rejected. The rarefaction case is the common + one in microbiome work: the computation is well defined on the input, and the problem is with + what the user does with the output. Refusing would push authors to write the condition as a note, + which is where it was. +- **A required "Step 1: Validate".** Rejected. It restates Materials, drifts from it, and puts the + answer to "what are the inputs" in two places. Existing protocols that have one remain valid; the + step is now redundant rather than wrong. +- **A structured schema for materials** (one YAML block per input with typed constraints). Rejected + for now. Conditions like "samples should share a sequencing depth" are prose, and a schema that + cannot express them would be worked around. Prose with two reserved words is checkable by an + agent and reviewable by a person. A schema waits for a validator that would use it. +- **Adopt RFC 2119 for the standard document itself.** Rejected. It would give about twenty existing + uses of *must* and *should* a meaning nobody intended, each of which would need auditing, for no + benefit to protocol authors. +- **Make `expected_warning` evidence on its own.** Rejected. An outcome checked only by a warning + string is checked only by what the executor said about itself, which is the rule ADR 0016 set + against labels. + +## Consequences + +Protocol authors have one place to write conditions and two words to write them with. Reviewers +have a rule to apply when a condition appears in a step or a note. Executors have a defined action +for each tier, and the test contract can check both. + +The validator is unchanged: it cannot check prose semantics, and the rule is enforceable in review. +The `expected_warning` and `warnings` keys are validated and checked by the existing tooling. + +Existing protocols with a validation step remain valid. Moving their conditions into Materials is a +content change for each repository, made as protocols are next revised. + +The runner's before-Step-1 check is part of its execution contract, which issue #15 proposes to +move into `PROTOCOL_STANDARD.md`; this ADR puts the condition rules there directly and leaves the +rest of #15 open. diff --git a/docs/adr/README.md b/docs/adr/README.md index 11885f8..86ac1a9 100644 --- a/docs/adr/README.md +++ b/docs/adr/README.md @@ -20,5 +20,6 @@ This directory contains Architecture Decision Records (ADRs) for the `agent-prot - [0014. Ask Two Citation Questions, and Require the Answerable One](0014-two-citation-questions.md) — amends 0009 and 0012 - [0015. Migrate Protocol Validator to Python and Pydantic](0015-migrate-validator-to-python.md) - [0016. Protocol Tests: Fixtures, a Result Contract, and a Review Status](0016-protocol-tests.md) +- [0017. Conditions on Materials: Where They Live and What a Violation Means](0017-conditions-on-materials.md) — proposed; extends 0012 and 0016 diff --git a/scripts/check_test_result.py b/scripts/check_test_result.py index 24ffa1a..4c87ddc 100644 --- a/scripts/check_test_result.py +++ b/scripts/check_test_result.py @@ -35,7 +35,7 @@ from repo_utils import parse_repository_url # noqa: E402 CHECKER_NAME = "check_test_result.py" -CHECKER_VERSION = "0.1.0" +CHECKER_VERSION = "0.2.0" RUN_RECORD_VERSION = 1 SOURCE_FILE = "source.yaml" EXECUTOR_FILE = "executor.yaml" @@ -233,7 +233,8 @@ def shape_of(case: ProtocolTest, test_dir: Path, results_dir: Path) -> dict: if o.expected_absent: entry["expected_absent"] = list(o.expected_absent) outcomes.append(entry) - shape["result"] = {"outcome": "one of the labels below", "observation": "string"} + shape["result"] = {"outcome": "one of the labels below", "observation": "string", + "warnings": "list of strings, when the protocol said to warn"} shape["accepted_outcomes"] = outcomes return shape @@ -454,6 +455,11 @@ def check_one(case: ProtocolTest, test_dir: Path, result_dir: Path, results_dir: return entry tol = case.evaluation.numeric_tolerance + warnings = result.get("warnings") + if warnings is not None and not (isinstance(warnings, list) and all(isinstance(w, str) for w in warnings)): + entry["result"] = "fail" + entry["failures"] = ["warnings: must be a list of strings"] + return entry if case.evaluation.mode == "exact": failures = compare_top(case.expected_outputs, result, tol, test_dir, result_dir, results_dir) else: @@ -472,6 +478,12 @@ def check_one(case: ProtocolTest, test_dir: Path, result_dir: Path, results_dir: for k in outcome.expected_absent or []: if k in result: failures.append(f"{k}: present in result.yaml, but outcome '{label}' requires it absent") + if outcome.expected_warning: + needle = outcome.expected_warning.casefold() + if not any(needle in w.casefold() for w in warnings or []): + failures.append( + f"warnings: no entry contains {outcome.expected_warning!r}, which outcome '{label}' requires" + ) entry["result"] = "pass" if not failures else "fail" if failures: entry["failures"] = failures diff --git a/scripts/models.py b/scripts/models.py index 338112f..2d4e5c5 100644 --- a/scripts/models.py +++ b/scripts/models.py @@ -153,8 +153,9 @@ def validate_type_and_deps(self): TEST_NAME_PATTERN = r"^[a-z0-9]+([-_][a-z0-9]+)*$" SHA256_PATTERN = r"^[0-9a-f]{64}$" -# Keys the executor's own report uses in diagnostic mode; a test may not expect them. -RESERVED_RESULT_KEYS = ("outcome", "observation") +# Keys the executor's own report uses; a test may not expect them. `outcome` and `observation` +# belong to diagnostic mode, `warnings` to either mode. +RESERVED_RESULT_KEYS = ("outcome", "observation", "warnings") # Keys the checker writes into run.yaml itself; executor.yaml may not supply them. RESERVED_RUN_RECORD_KEYS = ( "run_record", "protocol", "version", "repository", "protocol_commit", @@ -306,6 +307,7 @@ class AcceptedOutcome(BaseModel): outcome: str expected_outputs: Optional[Dict[str, Any]] = None expected_absent: Optional[List[str]] = None + expected_warning: Optional[str] = None @model_validator(mode='after') def has_evidence(self): @@ -313,10 +315,13 @@ def has_evidence(self): raise ValueError(f"outcome label must be lower-case words joined by - or _, found '{self.outcome}'") has_outputs = bool(self.expected_outputs) has_absent = bool(self.expected_absent) + # A warning is the executor's word, like the label; it is checked alongside evidence, not instead of it. if not has_outputs and not has_absent: raise ValueError( f"outcome '{self.outcome}' needs evidence: a non-empty 'expected_outputs' or 'expected_absent'" ) + if self.expected_warning is not None and not self.expected_warning.strip(): + raise ValueError(f"outcome '{self.outcome}' 'expected_warning' must not be blank") if has_outputs: _check_no_reserved(self.expected_outputs, f"outcome '{self.outcome}' expected_outputs") _check_file_refs(self.expected_outputs, f"outcome '{self.outcome}' expected_outputs", expected=True) diff --git a/skills/protocol-runner/SKILL.md b/skills/protocol-runner/SKILL.md index 9b7e465..a001bce 100644 --- a/skills/protocol-runner/SKILL.md +++ b/skills/protocol-runner/SKILL.md @@ -1,7 +1,7 @@ --- name: protocol-runner description: Search, retrieve, evaluate trust, and execute citable workflows from federated protocol repositories -version: 2.0.0 +version: 2.1.0 category: protocols author: waldronlab tags: [workflow, protocol, pipeline, method_origin_citation, provenance] @@ -96,15 +96,19 @@ silent substitution this skill is meant to prevent: ### 6. Execute Protocol -1. Follow the steps in the fetched protocol content in order. -2. **Resource Discovery**: Agents can discover and download pre-computed reference data and upstream tools using the `database_urls` and `upstream_repositories` YAML fields provided in the index. -3. Adapt the provided code to the user's specific environment, file paths, parameters, and organisms as necessary. -4. If a step cannot be followed exactly as written, or requires a different package version than specified, note this departure inline. +1. **Check the materials before Step 1.** Every condition in `## Materials` is written with *must* or *should* (PROTOCOL_STANDARD.md, "Markdown Content Structure"). Check each against the inputs the user supplied: + - A *must* condition that fails: stop, report which condition failed and on which input, and produce none of the protocol's outputs. + - A *should* condition that fails: warn the user, naming the condition and the consequence the protocol gives for it, then continue. Record the warning for step 7. +2. Follow the steps in the fetched protocol content in order. +3. **Resource Discovery**: Agents can discover and download pre-computed reference data and upstream tools using the `database_urls` and `upstream_repositories` YAML fields provided in the index. +4. Adapt the provided code to the user's specific environment, file paths, parameters, and organisms as necessary. +5. If a step cannot be followed exactly as written, or requires a different package version than specified, note this departure inline. ### 7. Record Departures 1. After execution completes, emit a final "Departures from protocol" section. 2. List any deviations made during execution (e.g., using a different parameter value, skipping a step, or substituting a package). This is a normal part of adapting a protocol; recording it is what matters for provenance. +3. List every warning given in step 6.1, with the condition that was not met. A warning given and not recorded is lost to the reader of the Methods section, who is the person it was for. ### 8. Generate Draft Methods Section diff --git a/skills/test-protocol/SKILL.md b/skills/test-protocol/SKILL.md index 31624e3..1d40574 100644 --- a/skills/test-protocol/SKILL.md +++ b/skills/test-protocol/SKILL.md @@ -1,7 +1,7 @@ --- name: test-protocol description: Execute a protocol's tests from its tests/ directory, write results in the standard's result contract, and check them with the deterministic checker -version: 0.1.0 +version: 0.2.0 category: protocols author: waldronlab tags: [protocol, tests, benchmark, verification, provenance] @@ -62,17 +62,23 @@ For each test in the shape, in any language: 1. Read `protocol.md` in full. Read the test's `inputs`; a file slot `{path: ...}` is relative to the test's directory, which the shape names. -2. Perform the protocol on those inputs exactly as written. Where the protocol says to record a +2. Check every condition in `## Materials` against the inputs before Step 1. A *must* condition + that fails is a refusal. A *should* condition that fails is a warning: proceed, and keep the + warning text for `result.yaml`. +3. Perform the protocol on those inputs exactly as written. Where the protocol says to record a quantity, that quantity is what a top-level key in the shape asks for. -3. Write `//result.yaml`: +4. Write `//result.yaml`: - **exact mode**: one key per top-level key in the shape. A file slot is written as `{path: }` and the file goes in the same subdirectory as a CSV with the header the shape gave, or the header the protocol prescribes when the shape says `withheld`. - **diagnostic mode**: `outcome`, one of the accepted labels; `observation`, one sentence saying what you found; and whatever keys that outcome expects present. If you refused - because the input violates a requirement the protocol states, say so in `observation` and - write nothing the protocol would have produced. -4. Record more than the shape asks for if it helps a reader; extra top-level keys are ignored. + because the input violates a *must* condition, say so in `observation` and write nothing + the protocol would have produced. + - **either mode**: `warnings`, a list of strings, when a *should* condition was not met. Each + entry names the condition and the consequence the protocol gives for it. Omit the key when + there was nothing to warn about. +5. Record more than the shape asks for if it helps a reader; extra top-level keys are ignored. Inside a nested mapping, record exactly the entries the protocol produces; an extra entry is a wrong answer. @@ -93,7 +99,7 @@ executor: language: R invocation: kind: skill - skill: test-protocol 0.1.0 + skill: test-protocol 0.2.0 prompt: | Run the tests for independent-filtering-variance. ``` @@ -129,7 +135,7 @@ during the run. Start again from step 2 in a fresh results directory. out afterwards. 6. A closing line naming this skill and its version: - > 🛠️ Skill executed: `test-protocol` v`0.1.0` (`waldronlab/agent-protocol-standard`) + > 🛠️ Skill executed: `test-protocol` v`0.2.0` (`waldronlab/agent-protocol-standard`) ## Notes diff --git a/template/protocols/example-protocol/protocol.md b/template/protocols/example-protocol/protocol.md index 7155380..fbacdae 100644 --- a/template/protocols/example-protocol/protocol.md +++ b/template/protocols/example-protocol/protocol.md @@ -43,6 +43,12 @@ One or two sentences of overview: what this produces, and what it is for. ## Materials +- **Input data:** + - `counts` — a CSV table, one row per feature and one column per sample. The first column must + hold unique feature identifiers; every other cell must be a non-negative number. Samples should + share a sequencing depth; if they do not, warn that values are not comparable across samples. +- **Parameters:** + - `threshold` — the minimum proportion retained, default `0.9`; must be between 0 and 1. - **Software & Repositories:** - `tool-name` (https://example.org/tool-name) — what it is, and which version this protocol assumes. - **Databases & Reference Data:** diff --git a/tests/fixtures/valid/with-tests/protocols/example-protocol/tests/orientation/test.yaml b/tests/fixtures/valid/with-tests/protocols/example-protocol/tests/orientation/test.yaml index 5430083..6c3b3eb 100644 --- a/tests/fixtures/valid/with-tests/protocols/example-protocol/tests/orientation/test.yaml +++ b/tests/fixtures/valid/with-tests/protocols/example-protocol/tests/orientation/test.yaml @@ -12,5 +12,6 @@ evaluation: removed_features: [A, B] retained_features: [C, D] retained_matrix: {path: retained.csv, header: withheld} + expected_warning: orientation - outcome: refused_for_orientation expected_absent: [removed_features, retained_features, retained_matrix] diff --git a/tests/test_protocol_tests.py b/tests/test_protocol_tests.py index a0226bc..77c641a 100644 --- a/tests/test_protocol_tests.py +++ b/tests/test_protocol_tests.py @@ -79,6 +79,22 @@ def test_diagnostic_requires_evidence_and_no_top_level_expectations(): {"outcome": "r", "expected_outputs": {"n": 1}, "expected_absent": ["n"]}]}}) +def test_expected_warning_accompanies_evidence(): + base = {"name": "diag", "description": "x", "inputs": {}} + # a warning alone is not evidence + with pytest.raises(ValidationError, match="needs evidence"): + ProtocolTest(**{**base, "evaluation": {"mode": "diagnostic", "accepted_outcomes": [ + {"outcome": "warned", "expected_warning": "depth"}]}}) + with pytest.raises(ValidationError, match="must not be blank"): + ProtocolTest(**{**base, "evaluation": {"mode": "diagnostic", "accepted_outcomes": [ + {"outcome": "warned", "expected_outputs": {"n": 1}, "expected_warning": " "}]}}) + ProtocolTest(**{**base, "evaluation": {"mode": "diagnostic", "accepted_outcomes": [ + {"outcome": "warned", "expected_outputs": {"n": 1}, "expected_warning": "depth"}]}}) + # `warnings` is the executor's key in either mode + with pytest.raises(ValidationError, match="reserved key"): + ProtocolTest(**exact_case(expected_outputs={"warnings": ["x"]})) + + def test_negative_or_non_finite_tolerance_rejected(): for bad in (-1, float("nan"), float("inf")): with pytest.raises(ValidationError, match="finite number >= 0"): @@ -147,6 +163,7 @@ def passing_results(results: Path): (results / "basic" / "retained.csv").write_text("feature,sample_1,sample_2\nC,5,8.0\nD,10,10\n") write_yaml(results / "orientation" / "result.yaml", { "outcome": "orientation_corrected", "observation": "Transposed the input.", + "warnings": ["Features were in columns; the orientation was corrected before filtering."], "removed_features": ["A", "B"], "retained_features": ["C", "D"], "retained_matrix": {"path": "kept.csv"}, }) @@ -256,6 +273,39 @@ def test_check_diagnostic_rules(repo, tmp_path, capsys): assert "not one of the accepted outcomes" in out and "observation: missing" in out +def test_check_expected_warning(repo, tmp_path, capsys): + results = tmp_path / "results" + tests = repo / "protocols/example-protocol/tests" + _, out = shape(repo, results, capsys) + orient = next(t for t in out["tests"] if t["name"] == "orientation") + assert "warnings" in orient["result"] + assert all("expected_warning" not in o for o in orient["accepted_outcomes"]) # the phrase is withheld + executor(results) + passing_results(results) + corrected = yaml.safe_load((results / "orientation/result.yaml").read_text()) + # the outputs are right but nobody was warned + write_yaml(results / "orientation/result.yaml", {k: v for k, v in corrected.items() if k != "warnings"}) + assert checker.main(["--test", "orientation", str(tests), str(results)]) == 1 + assert "no entry contains 'orientation'" in capsys.readouterr().out + # a warning about something else does not count + write_yaml(results / "orientation/result.yaml", {**corrected, "warnings": ["Low depth in s2."]}) + assert checker.main(["--test", "orientation", str(tests), str(results)]) == 1 + # the match is case-insensitive + write_yaml(results / "orientation/result.yaml", {**corrected, "warnings": ["ORIENTATION corrected."]}) + assert checker.main(["--test", "orientation", str(tests), str(results)]) == 0 + # warnings must be a list of strings, in either mode + write_yaml(results / "orientation/result.yaml", {**corrected, "warnings": "orientation"}) + assert checker.main(["--test", "orientation", str(tests), str(results)]) == 1 + assert "must be a list of strings" in capsys.readouterr().out + basic = yaml.safe_load((results / "basic/result.yaml").read_text()) + write_yaml(results / "basic/result.yaml", {**basic, "warnings": [1]}) + assert checker.main(["--test", "basic", str(tests), str(results)]) == 1 + # a refusal need not warn + write_yaml(results / "orientation/result.yaml", + {"outcome": "refused_for_orientation", "observation": "Features are in columns."}) + assert checker.main(["--test", "orientation", str(tests), str(results)]) == 0 + + def test_check_not_run_is_neither(repo, tmp_path, capsys): results = tmp_path / "results" tests = repo / "protocols/example-protocol/tests" From 9f2541f65cd5e313439ba51f8f1637b26cf6c32b Mon Sep 17 00:00:00 2001 From: Levi Waldron Date: Fri, 23 Oct 2026 09:44:24 -0400 Subject: [PATCH 2/3] Address review: fixture states its should condition, stale versions, no early return The with-tests fixture's Materials now states the orientation condition its test expects a warning for. Diagnostic mode is defined as failing a condition in Materials. Run-record example at 0.2.0. Malformed warnings is reported alongside the other failures rather than short-circuiting. expected_warning is stripped in the model. Blank-warning invalid fixture. Drafted with Claude Code. Co-Authored-By: Claude Fable 5.1 --- PROTOCOL_STANDARD.md | 6 +- docs/adr/0017-conditions-on-materials.md | 4 +- scripts/check_test_result.py | 11 +-- scripts/models.py | 6 +- skills/protocol-runner/SKILL.md | 2 +- .../test-blank-expected-warning/expected.txt | 1 + .../protocols/example-protocol/protocol.md | 75 +++++++++++++++++++ .../tests/blank-warning/test.yaml | 9 +++ .../protocols/example-protocol/protocol.md | 3 + tests/test_protocol_tests.py | 1 + 10 files changed, 103 insertions(+), 15 deletions(-) create mode 100644 tests/fixtures/invalid/test-blank-expected-warning/expected.txt create mode 100644 tests/fixtures/invalid/test-blank-expected-warning/protocols/example-protocol/protocol.md create mode 100644 tests/fixtures/invalid/test-blank-expected-warning/protocols/example-protocol/tests/blank-warning/test.yaml diff --git a/PROTOCOL_STANDARD.md b/PROTOCOL_STANDARD.md index f9ebedb..dc5583b 100644 --- a/PROTOCOL_STANDARD.md +++ b/PROTOCOL_STANDARD.md @@ -362,7 +362,7 @@ else. The default is to show the header in the result shape. **`exact`**: every entry in `expected_outputs` must be present in the result and match. `expected_outputs` must have at least one entry; a test that checks nothing is invalid. -**`diagnostic`**: the input violates a requirement the protocol states, and the test checks what the +**`diagnostic`**: the input fails a condition in Materials, and the test checks what the executor does about it. `evaluation.accepted_outcomes` lists the outcomes that count as correct. The top-level `expected_outputs` is omitted; each outcome carries its own evidence instead, at least one of: @@ -469,10 +469,10 @@ executor: language: R # what the execution was written in, if known invocation: # how the protocol reached the agent kind: skill # skill | in-context | unguided - skill: test-protocol 0.1.0 # name and version, for kind: skill + skill: test-protocol 0.2.0 # name and version, for kind: skill prompt: | # verbatim text that started the run Run the tests for independent-filtering-variance. -checker: check_test_result.py 0.1.0 +checker: check_test_result.py 0.2.0 tests: - name: variance_rows result: pass # pass | fail | not_run (no result subdirectory) diff --git a/docs/adr/0017-conditions-on-materials.md b/docs/adr/0017-conditions-on-materials.md index cf24725..28b2f0d 100644 --- a/docs/adr/0017-conditions-on-materials.md +++ b/docs/adr/0017-conditions-on-materials.md @@ -47,8 +47,8 @@ is not reinterpreted. phrase that must appear in the executor's `warnings`. It accompanies evidence (`expected_outputs` or `expected_absent`) rather than replacing it, since a warning is the executor's word as much as the outcome label is. `result.yaml` may carry `warnings`, a list of strings, in either mode; -`warnings` joins `outcome` and `observation` as a reserved key. The result shape shows that -`warnings` exists and withholds the phrase, so an executor warns because the protocol said to, not +`warnings` joins `outcome` and `observation` as a reserved key. The diagnostic result shape names +`warnings` and withholds the phrase, so an executor warns because the protocol said to, not because the shape did. The protocol-runner skill checks Materials before Step 1 and lists warnings with the departures. diff --git a/scripts/check_test_result.py b/scripts/check_test_result.py index 4c87ddc..424d078 100644 --- a/scripts/check_test_result.py +++ b/scripts/check_test_result.py @@ -456,14 +456,11 @@ def check_one(case: ProtocolTest, test_dir: Path, result_dir: Path, results_dir: tol = case.evaluation.numeric_tolerance warnings = result.get("warnings") - if warnings is not None and not (isinstance(warnings, list) and all(isinstance(w, str) for w in warnings)): - entry["result"] = "fail" - entry["failures"] = ["warnings: must be a list of strings"] - return entry + warnings_ok = warnings is None or (isinstance(warnings, list) and all(isinstance(w, str) for w in warnings)) + failures = [] if warnings_ok else ["warnings: must be a list of strings"] if case.evaluation.mode == "exact": - failures = compare_top(case.expected_outputs, result, tol, test_dir, result_dir, results_dir) + failures += compare_top(case.expected_outputs, result, tol, test_dir, result_dir, results_dir) else: - failures = [] label = result.get("outcome") outcome = case.outcome(label) if isinstance(label, str) else None if outcome is None: @@ -480,7 +477,7 @@ def check_one(case: ProtocolTest, test_dir: Path, result_dir: Path, results_dir: failures.append(f"{k}: present in result.yaml, but outcome '{label}' requires it absent") if outcome.expected_warning: needle = outcome.expected_warning.casefold() - if not any(needle in w.casefold() for w in warnings or []): + if not warnings_ok or not any(needle in w.casefold() for w in warnings or []): failures.append( f"warnings: no entry contains {outcome.expected_warning!r}, which outcome '{label}' requires" ) diff --git a/scripts/models.py b/scripts/models.py index 2d4e5c5..179f6f0 100644 --- a/scripts/models.py +++ b/scripts/models.py @@ -320,8 +320,10 @@ def has_evidence(self): raise ValueError( f"outcome '{self.outcome}' needs evidence: a non-empty 'expected_outputs' or 'expected_absent'" ) - if self.expected_warning is not None and not self.expected_warning.strip(): - raise ValueError(f"outcome '{self.outcome}' 'expected_warning' must not be blank") + if self.expected_warning is not None: + self.expected_warning = self.expected_warning.strip() + if not self.expected_warning: + raise ValueError(f"outcome '{self.outcome}' 'expected_warning' must not be blank") if has_outputs: _check_no_reserved(self.expected_outputs, f"outcome '{self.outcome}' expected_outputs") _check_file_refs(self.expected_outputs, f"outcome '{self.outcome}' expected_outputs", expected=True) diff --git a/skills/protocol-runner/SKILL.md b/skills/protocol-runner/SKILL.md index a001bce..714e7a2 100644 --- a/skills/protocol-runner/SKILL.md +++ b/skills/protocol-runner/SKILL.md @@ -108,7 +108,7 @@ silent substitution this skill is meant to prevent: 1. After execution completes, emit a final "Departures from protocol" section. 2. List any deviations made during execution (e.g., using a different parameter value, skipping a step, or substituting a package). This is a normal part of adapting a protocol; recording it is what matters for provenance. -3. List every warning given in step 6.1, with the condition that was not met. A warning given and not recorded is lost to the reader of the Methods section, who is the person it was for. +3. List every warning given in step 6.1, with the condition that was not met. ### 8. Generate Draft Methods Section diff --git a/tests/fixtures/invalid/test-blank-expected-warning/expected.txt b/tests/fixtures/invalid/test-blank-expected-warning/expected.txt new file mode 100644 index 0000000..5ed8328 --- /dev/null +++ b/tests/fixtures/invalid/test-blank-expected-warning/expected.txt @@ -0,0 +1 @@ +'expected_warning' must not be blank diff --git a/tests/fixtures/invalid/test-blank-expected-warning/protocols/example-protocol/protocol.md b/tests/fixtures/invalid/test-blank-expected-warning/protocols/example-protocol/protocol.md new file mode 100644 index 0000000..3eba6af --- /dev/null +++ b/tests/fixtures/invalid/test-blank-expected-warning/protocols/example-protocol/protocol.md @@ -0,0 +1,75 @@ +--- +name: example-protocol +description: A minimal conforming protocol used as a fixture for the validator test suite. +version: 1.1.0 +authors: + - name: Ada Lovelace + orcid: 0000-0002-1825-0097 +reviews: + - name: Grace Hopper + orcid: 0000-0001-5109-3700 + date: 2026-02-01 + protocol_version: 1.0.0 + status: verified-with-benchmark + - name: Alan Turing + date: 2026-01-20 + protocol_version: 1.0.0 + status: approved +date: 2026-03-01 +status: draft +license: CC-BY-4.0 +type: atomic +method_origin_citation: "10.1000/example" +protocol_citation: "10.1000/example-procedure" +protocols_used: [] +key_packages: [] +category: example +tags: [example, fixture] +--- + +# Example Protocol + +A minimal conforming protocol, exercising two releases, a reviewed older version, an unreviewed +current version, and a reviewer with and without an ORCID. + +## Materials + +- **Software & Repositories:** + - `example-tool` (https://example.org/example-tool) — version 1.0. + +## Steps + +### Step 1: Run the example tool +Run `example-tool --input reads.fastq --output counts.tsv`. + +## Notes + +This protocol exists only to exercise `scripts/validate-protocol.R`. It is not a real method. + +## History & Reviews + + +### Version 1.1.0 (2026-03-01) + +#### Changes +- Made the output path of Step 1 explicit. + +#### Reviews +*No reviews yet.* + +### Version 1.0.0 (2026-01-15) + +#### Changes +- Initial protocol creation. + +#### Reviews + +**Review by Grace Hopper ([0000-0001-5109-3700](https://orcid.org/0000-0001-5109-3700))** +- **Date:** 2026-02-01 +- **Status:** `verified-with-benchmark` +- **Notes:** Executed against the example dataset; the output counts matched the expected values. + +**Review by Alan Turing** +- **Date:** 2026-01-20 +- **Status:** `approved` +- **Notes:** Read for scientific soundness. The parameters in Step 1 are appropriate. diff --git a/tests/fixtures/invalid/test-blank-expected-warning/protocols/example-protocol/tests/blank-warning/test.yaml b/tests/fixtures/invalid/test-blank-expected-warning/protocols/example-protocol/tests/blank-warning/test.yaml new file mode 100644 index 0000000..8a992bd --- /dev/null +++ b/tests/fixtures/invalid/test-blank-expected-warning/protocols/example-protocol/tests/blank-warning/test.yaml @@ -0,0 +1,9 @@ +name: blank-warning +description: A diagnostic outcome whose expected_warning is blank. +inputs: {theta: 0.5} +evaluation: + mode: diagnostic + accepted_outcomes: + - outcome: warned + expected_outputs: {n: 1} + expected_warning: " " diff --git a/tests/fixtures/valid/with-tests/protocols/example-protocol/protocol.md b/tests/fixtures/valid/with-tests/protocols/example-protocol/protocol.md index 3eba6af..7936194 100644 --- a/tests/fixtures/valid/with-tests/protocols/example-protocol/protocol.md +++ b/tests/fixtures/valid/with-tests/protocols/example-protocol/protocol.md @@ -34,6 +34,9 @@ current version, and a reviewer with and without an ORCID. ## Materials +- **Input data:** + - `data` — a CSV table. Features should be in rows and samples in columns; if they are in + columns, correct the orientation and warn that the orientation was corrected, or refuse. - **Software & Repositories:** - `example-tool` (https://example.org/example-tool) — version 1.0. diff --git a/tests/test_protocol_tests.py b/tests/test_protocol_tests.py index 77c641a..e86505b 100644 --- a/tests/test_protocol_tests.py +++ b/tests/test_protocol_tests.py @@ -163,6 +163,7 @@ def passing_results(results: Path): (results / "basic" / "retained.csv").write_text("feature,sample_1,sample_2\nC,5,8.0\nD,10,10\n") write_yaml(results / "orientation" / "result.yaml", { "outcome": "orientation_corrected", "observation": "Transposed the input.", + # the orientation test's corrected outcome carries expected_warning, so this is required "warnings": ["Features were in columns; the orientation was corrected before filtering."], "removed_features": ["A", "B"], "retained_features": ["C", "D"], "retained_matrix": {"path": "kept.csv"}, From 9f44eb6b665cd778047e9c5d79d3f00444e558c5 Mon Sep 17 00:00:00 2001 From: Levi Waldron Date: Fri, 23 Oct 2026 09:58:22 -0400 Subject: [PATCH 3/3] accept adr 0017 --- docs/adr/0017-conditions-on-materials.md | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/docs/adr/0017-conditions-on-materials.md b/docs/adr/0017-conditions-on-materials.md index 28b2f0d..e2c793f 100644 --- a/docs/adr/0017-conditions-on-materials.md +++ b/docs/adr/0017-conditions-on-materials.md @@ -1,8 +1,8 @@ # 0017. Conditions on Materials: Where They Live and What a Violation Means -- **Status:** Proposed +- **Status:** Accepted - **Date:** 2026-10-23 -- **Deciders:** Levi Waldron (User), AI Agent +- **Deciders:** Levi Waldron ## Context and Problem Statement @@ -56,10 +56,9 @@ The test-protocol skill writes them into `result.yaml`. ## Alternatives Considered -- **One tier: every condition is a requirement.** Rejected. The rarefaction case is the common - one in microbiome work: the computation is well defined on the input, and the problem is with - what the user does with the output. Refusing would push authors to write the condition as a note, - which is where it was. +- **One tier: every condition is a requirement.** Rejected. In the rarefaction case, the computation + is well defined on the input, and the problem is with how the user may interpret the + output. Refusing would push authors to write the condition as a note, which is where it was. - **A required "Step 1: Validate".** Rejected. It restates Materials, drifts from it, and puts the answer to "what are the inputs" in two places. Existing protocols that have one remain valid; the step is now redundant rather than wrong.