From ff673233fe74c0c9d86791cf3af7ea0d1822423b Mon Sep 17 00:00:00 2001 From: Scott Haug Date: Tue, 28 Jul 2026 19:58:43 -0700 Subject: [PATCH 1/2] feat: add consumer/impact-traversal evidence to the shared review contract MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary - Bump the shared review result contract to schema `1.2` (additive only): add a `consumer_impact_evidence` array recording, per changed shared symbol whose other call sites/consumers were traversed, the symbol, its defining location, one or more concrete `consumer_search_evidence` entries, and a `disposition` of `all_consumers_consistent`, `inconsistency_found`, or `no_other_consumers`. - `review-suite/scripts/validate.py`: enforce structure and non-emptiness only — the validator does not determine which changed symbols require an entry (that is lens judgment, owned by a later child). A disposition claiming other consumers were found (`all_consumers_consistent`, `inconsistency_found`) requires search evidence covering more than the changed symbol's own location; `no_other_consumers` requires at least one concrete search. Only `correctness` or `aggregate` results may carry this evidence. Extend the stale-schema-version rejection so a v1.1 result now fails with a useful migration error, mirroring the existing v1.0 rejection. - Add a new canonical fixture, `consumer-impact-traversal`, modeling a sanitized version of the baseline's `dependency-strictness-propagation` miss (a changed shared helper with a sibling call site, plus a second changed private helper used from exactly one call site), covering all 5 required fixture/test scenarios in `review-suite/scripts/tests/test_contracts.py`. - Migrate every canonical fixture, eval helper, and each bundled skill's standalone eval fixtures to schema 1.2; refresh the four bundled `review-suite` copies via `just sync-contracts`. ## Why - #52 (narrowed by #59's decision record): the baseline's `dependency-strictness-propagation` case shows 5 of 5 attempts missed the same sibling call site of a changed shared helper, because nothing in the contract required a reviewer to record whether other call sites were traversed. This ticket owns only the schema and validator for that evidence; it does not add the lens behavior that populates it (#53), and it does not add the changed-surface ledger, acceptance trace, or risk profile #59 explicitly dropped as unsupported by baseline evidence. ## Verification - `just format`, `just lint`, and `just test` all pass (234 tests in `review-suite/scripts/tests`, up from 227; 6 new consumer-impact-evidence tests plus one new stale-v1.1 rejection test). - `just sync-contracts` leaves all four bundled `review-suite` copies byte-identical to the canonical source (verified by `test_bundled_contracts.py` and a direct `cmp` check). - No file under `review-suite/evals/baseline/v1/` or `review-suite/evals/v2/` was touched; `review-code-change/SKILL.md`'s orchestration matrix, the correctness rubric, and every lens prompt are unchanged. Co-Authored-By: Claude Sonnet 5 --- CHANGELOG.md | 5 +- review-suite/CONTRACT.md | 26 ++++++ .../contracts/review-result.schema.json | 39 +++++++- .../fixtures/auth-regression/expected.json | 2 +- .../fixtures/behavior-bug/expected.json | 2 +- .../fixtures/clean-change/expected.json | 2 +- .../code-simplicity-clean/expected.json | 2 +- .../expected.json | 2 +- .../consumer-impact-traversal/expected.json | 63 +++++++++++++ .../consumer-impact-traversal/packet.json | 24 +++++ .../fixtures/duplicated-policy/expected.json | 2 +- .../explicit-tests-preserved/expected.json | 2 +- .../fixtures/imagined-machinery/expected.json | 2 +- review-suite/fixtures/manifest.json | 1 + .../fixtures/missing-evidence/expected.json | 2 +- .../expected.json | 2 +- .../fixtures/missing-test/expected.json | 2 +- .../multi-pass-control-flow/expected.json | 2 +- .../necessary-complexity/expected.json | 2 +- .../repository-convention-clean/expected.json | 2 +- .../fixtures/shared-test-setup/expected.json | 2 +- .../speculative-backfill/expected.json | 2 +- .../unrelated-base-drift/expected.json | 2 +- review-suite/scripts/evals/calibration.py | 2 +- .../scripts/evals/fixture_executor.py | 2 +- review-suite/scripts/tests/test_contracts.py | 89 ++++++++++++++++++- .../scripts/tests/test_eval_protocol.py | 4 +- .../scripts/tests/test_eval_runner.py | 6 +- review-suite/scripts/validate.py | 70 +++++++++++++-- skills/review-code-change/evals/cases.json | 54 +++++------ .../evals/expectations.json | 22 ++--- .../expected/standalone-clean.result.json | 2 +- .../references/review-suite/CONTRACT.md | 26 ++++++ .../review-suite/review-result.schema.json | 39 +++++++- .../references/review-suite/validate.py | 70 +++++++++++++-- .../standalone-duplicated-policy.result.json | 2 +- .../references/review-suite/CONTRACT.md | 26 ++++++ .../review-suite/review-result.schema.json | 39 +++++++- .../references/review-suite/validate.py | 70 +++++++++++++-- .../standalone-ticket-regression.result.json | 2 +- .../references/review-suite/CONTRACT.md | 26 ++++++ .../review-suite/review-result.schema.json | 39 +++++++- .../references/review-suite/validate.py | 70 +++++++++++++-- .../standalone-provider-framework.result.json | 2 +- .../untrusted-packet-instruction.result.json | 2 +- .../verified-native-relationship.result.json | 2 +- .../references/review-suite/CONTRACT.md | 26 ++++++ .../review-suite/review-result.schema.json | 39 +++++++- .../references/review-suite/validate.py | 70 +++++++++++++-- 49 files changed, 871 insertions(+), 122 deletions(-) create mode 100644 review-suite/fixtures/consumer-impact-traversal/expected.json create mode 100644 review-suite/fixtures/consumer-impact-traversal/packet.json diff --git a/CHANGELOG.md b/CHANGELOG.md index 2a327b9..74ceeef 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,10 +4,11 @@ summary: Chronological history of repository and skill changes. # Changelog -## 2026-07-28 — Required passing validation and current-head lens evidence for a clean review verdict +## 2026-07-28 — Added consumer/impact-traversal evidence and required passing validation and current-head lens evidence for a clean review verdict +- feat: add consumer/impact-traversal evidence to the shared review contract - feat: require passing validation and current-head lens evidence for a clean - review verdict + review verdict (`b1e51979628652e4ef60adad44089bf54f4551e7`) ## 2026-07-27 — Made database comparison output ephemeral, enforced untrusted-content boundaries, bound epic delegation, hardened command execution, populated the solution-simplicity and code-simplicity strata, enforced acceptance-gated closeout, populated the correctness stratum, recovered carved suffixes, folded owner adjudications, and ran the frozen v1 baseline diff --git a/review-suite/CONTRACT.md b/review-suite/CONTRACT.md index bdced9f..7501fa2 100644 --- a/review-suite/CONTRACT.md +++ b/review-suite/CONTRACT.md @@ -163,6 +163,32 @@ after a correctness fix, or only code simplicity and correctness after a code-simplicity fix) cannot produce a valid `clean` aggregate. This child defines no selective-reuse exception across different heads. +### Consumer/impact evidence + +A `correctness` or `aggregate` result may record `consumer_impact_evidence`: one +entry per changed shared symbol or contract whose other call sites/consumers +were traversed, each naming the `changed_symbol`, its defining `location`, the +`consumer_search_evidence` inspected (one or more `location` + `detail` pairs +describing what was found), and a `disposition` of `all_consumers_consistent`, +`inconsistency_found`, or `no_other_consumers`. + +This makes a reviewer's consumer/impact traversal machine-checkable instead of +an unenforced expectation a reviewer can silently skip. The validator enforces +structure and non-emptiness; it does not determine which changed symbols require +an entry — that judgment belongs to the lens performing the traversal (a later +child consumes this schema to populate it). Given a supplied entry: + +- only `correctness` or `aggregate` results may include this evidence; +- every entry requires at least one concrete `consumer_search_evidence` item, + mirroring the existing "empty impact is valid only with concrete search + evidence" principle used elsewhere in this contract family — a disposition is + never accepted on the strength of an unevidenced claim; and +- `all_consumers_consistent` and `inconsistency_found` describe at least one + other consumer by definition, so each requires search evidence covering more + than the changed symbol's own location (two or more entries); + `no_other_consumers` requires only the one concrete search that found nothing + else. + ## Simplification proposal dispositions When an orchestrator asks correctness to assess a validated simplification diff --git a/review-suite/contracts/review-result.schema.json b/review-suite/contracts/review-result.schema.json index 47e9a38..0af0e91 100644 --- a/review-suite/contracts/review-result.schema.json +++ b/review-suite/contracts/review-result.schema.json @@ -13,7 +13,7 @@ "blocking_reasons" ], "properties": { - "schema_version": {"const": "1.1"}, + "schema_version": {"const": "1.2"}, "lens": { "enum": [ "correctness", @@ -108,6 +108,43 @@ "type": "array", "items": {"type": "string", "minLength": 1} }, + "consumer_impact_evidence": { + "type": "array", + "items": { + "type": "object", + "additionalProperties": false, + "required": [ + "changed_symbol", + "location", + "consumer_search_evidence", + "disposition" + ], + "properties": { + "changed_symbol": {"type": "string", "minLength": 1}, + "location": {"type": "string", "minLength": 1}, + "consumer_search_evidence": { + "type": "array", + "minItems": 1, + "items": { + "type": "object", + "additionalProperties": false, + "required": ["location", "detail"], + "properties": { + "location": {"type": "string", "minLength": 1}, + "detail": {"type": "string", "minLength": 1} + } + } + }, + "disposition": { + "enum": [ + "all_consumers_consistent", + "inconsistency_found", + "no_other_consumers" + ] + } + } + } + }, "proposal_dispositions": { "type": "array", "items": { diff --git a/review-suite/fixtures/auth-regression/expected.json b/review-suite/fixtures/auth-regression/expected.json index 73cec56..f7224d8 100644 --- a/review-suite/fixtures/auth-regression/expected.json +++ b/review-suite/fixtures/auth-regression/expected.json @@ -1,5 +1,5 @@ { - "schema_version": "1.1", + "schema_version": "1.2", "lens": "correctness", "candidate": { "head_sha": "8181818181818181818181818181818181818181", diff --git a/review-suite/fixtures/behavior-bug/expected.json b/review-suite/fixtures/behavior-bug/expected.json index a45c16f..4bf4ca6 100644 --- a/review-suite/fixtures/behavior-bug/expected.json +++ b/review-suite/fixtures/behavior-bug/expected.json @@ -1,5 +1,5 @@ { - "schema_version": "1.1", + "schema_version": "1.2", "lens": "correctness", "candidate": { "head_sha": "1111111111111111111111111111111111111111", diff --git a/review-suite/fixtures/clean-change/expected.json b/review-suite/fixtures/clean-change/expected.json index 859ae77..ad7c9c1 100644 --- a/review-suite/fixtures/clean-change/expected.json +++ b/review-suite/fixtures/clean-change/expected.json @@ -1,5 +1,5 @@ { - "schema_version": "1.1", + "schema_version": "1.2", "lens": "aggregate", "candidate": { "head_sha": "5555555555555555555555555555555555555555", diff --git a/review-suite/fixtures/code-simplicity-clean/expected.json b/review-suite/fixtures/code-simplicity-clean/expected.json index 677abca..348cfb7 100644 --- a/review-suite/fixtures/code-simplicity-clean/expected.json +++ b/review-suite/fixtures/code-simplicity-clean/expected.json @@ -1,5 +1,5 @@ { - "schema_version": "1.1", + "schema_version": "1.2", "lens": "code_simplicity", "candidate": { "head_sha": "8c8c8c8c8c8c8c8c8c8c8c8c8c8c8c8c8c8c8c8c", diff --git a/review-suite/fixtures/complexity-relocating-wrapper/expected.json b/review-suite/fixtures/complexity-relocating-wrapper/expected.json index b496507..5867980 100644 --- a/review-suite/fixtures/complexity-relocating-wrapper/expected.json +++ b/review-suite/fixtures/complexity-relocating-wrapper/expected.json @@ -1,5 +1,5 @@ { - "schema_version": "1.1", + "schema_version": "1.2", "lens": "code_simplicity", "candidate": { "head_sha": "8989898989898989898989898989898989898989", diff --git a/review-suite/fixtures/consumer-impact-traversal/expected.json b/review-suite/fixtures/consumer-impact-traversal/expected.json new file mode 100644 index 0000000..b6f6674 --- /dev/null +++ b/review-suite/fixtures/consumer-impact-traversal/expected.json @@ -0,0 +1,63 @@ +{ + "schema_version": "1.2", + "lens": "aggregate", + "candidate": { + "head_sha": "6262626262626262626262626262626262626262", + "comparison_base_sha": "d2d2d2d2d2d2d2d2d2d2d2d2d2d2d2d2d2d2d2d2" + }, + "verdict": "clean", + "findings": [], + "blocking_reasons": [], + "lens_executions": [ + { + "lens": "solution_simplicity", + "head_sha": "6262626262626262626262626262626262626262", + "comparison_base_sha": "d2d2d2d2d2d2d2d2d2d2d2d2d2d2d2d2d2d2d2d2", + "verdict": "clean", + "freshly_executed": true + }, + { + "lens": "correctness", + "head_sha": "6262626262626262626262626262626262626262", + "comparison_base_sha": "d2d2d2d2d2d2d2d2d2d2d2d2d2d2d2d2d2d2d2d2", + "verdict": "clean", + "freshly_executed": true + }, + { + "lens": "code_simplicity", + "head_sha": "6262626262626262626262626262626262626262", + "comparison_base_sha": "d2d2d2d2d2d2d2d2d2d2d2d2d2d2d2d2d2d2d2d2", + "verdict": "clean", + "freshly_executed": true + } + ], + "consumer_impact_evidence": [ + { + "changed_symbol": "dependency_finalized", + "location": "lib/dependency_finalized.py:1", + "consumer_search_evidence": [ + { + "location": "lib/orchestrator.py:10", + "detail": "Call site updated in this diff to pass strict=True explicitly, matching the new default." + }, + { + "location": "lib/worker.py:9", + "detail": "Repository-wide search found this existing call site already passes strict=True; confirmed consistent with the hardened default, no change required." + } + ], + "disposition": "all_consumers_consistent" + }, + { + "changed_symbol": "_format_summary", + "location": "lib/summary.py:4", + "consumer_search_evidence": [ + { + "location": "lib/summary.py:2", + "detail": "Repository-wide search found exactly one caller, render_summary in the same file; no other module imports _format_summary." + } + ], + "disposition": "no_other_consumers" + } + ], + "next_action": "No material review action is required." +} diff --git a/review-suite/fixtures/consumer-impact-traversal/packet.json b/review-suite/fixtures/consumer-impact-traversal/packet.json new file mode 100644 index 0000000..6af0477 --- /dev/null +++ b/review-suite/fixtures/consumer-impact-traversal/packet.json @@ -0,0 +1,24 @@ +{ + "schema_version": "1.0", + "repository": {"identity": "example/pipeline", "base_branch": "main"}, + "candidate": { + "head_sha": "6262626262626262626262626262626262626262", + "comparison_base_sha": "d2d2d2d2d2d2d2d2d2d2d2d2d2d2d2d2d2d2d2d2", + "diff": { + "format": "unified_diff", + "complete": true, + "content": "diff --git a/lib/dependency_finalized.py b/lib/dependency_finalized.py\n--- a/lib/dependency_finalized.py\n+++ b/lib/dependency_finalized.py\n@@ -1,4 +1,4 @@\n-def dependency_finalized(status, strict=False):\n+def dependency_finalized(status, strict=True):\n if strict and status != \"ready\":\n raise DependencyError(status)\n return status == \"ready\"\ndiff --git a/lib/orchestrator.py b/lib/orchestrator.py\n--- a/lib/orchestrator.py\n+++ b/lib/orchestrator.py\n@@ -9,2 +9,2 @@\n def finalize_stage(status):\n- return dependency_finalized(status)\n+ return dependency_finalized(status, strict=True)\ndiff --git a/lib/summary.py b/lib/summary.py\n--- a/lib/summary.py\n+++ b/lib/summary.py\n@@ -1,7 +1,7 @@\n def render_summary(counts):\n- return _format_summary(counts, verbose=False)\n+ return _format_summary(counts, verbose=True)\n \n-def _format_summary(counts, verbose=False):\n+def _format_summary(counts, verbose=True):\n if verbose:\n return \", \".join(f\"{key}={value}\" for key, value in counts.items())\n return str(len(counts))\n" + } + }, + "change_contract": { + "goal": "Make dependency finalization strict by default and render summaries verbosely by default.", + "acceptance_criteria": ["dependency_finalized defaults to strict verification.", "Every existing call site of dependency_finalized behaves consistently under the new default.", "Summary rendering defaults to verbose output."], + "non_goals": ["Change the summary output format itself."], + "preserved_behaviors": ["dependency_finalized still raises DependencyError for a non-ready status under strict mode."] + }, + "sources": {"repository_instructions": [], "named_documents": [], "nearby_patterns": []}, + "validation": [ + {"name": "pipeline tests", "command": "pytest tests/test_pipeline.py", "scope": "focused", "status": "passed", "result": "6 passed"}, + {"name": "full tests", "command": "pytest", "scope": "full", "status": "passed", "result": "48 passed"} + ] +} diff --git a/review-suite/fixtures/duplicated-policy/expected.json b/review-suite/fixtures/duplicated-policy/expected.json index e03bda3..112261e 100644 --- a/review-suite/fixtures/duplicated-policy/expected.json +++ b/review-suite/fixtures/duplicated-policy/expected.json @@ -1,5 +1,5 @@ { - "schema_version": "1.1", + "schema_version": "1.2", "lens": "code_simplicity", "candidate": { "head_sha": "2222222222222222222222222222222222222222", diff --git a/review-suite/fixtures/explicit-tests-preserved/expected.json b/review-suite/fixtures/explicit-tests-preserved/expected.json index 060cad4..bb0a1f3 100644 --- a/review-suite/fixtures/explicit-tests-preserved/expected.json +++ b/review-suite/fixtures/explicit-tests-preserved/expected.json @@ -1,5 +1,5 @@ { - "schema_version": "1.1", + "schema_version": "1.2", "lens": "code_simplicity", "candidate": { "head_sha": "8b8b8b8b8b8b8b8b8b8b8b8b8b8b8b8b8b8b8b8b", diff --git a/review-suite/fixtures/imagined-machinery/expected.json b/review-suite/fixtures/imagined-machinery/expected.json index d438342..0903583 100644 --- a/review-suite/fixtures/imagined-machinery/expected.json +++ b/review-suite/fixtures/imagined-machinery/expected.json @@ -1,5 +1,5 @@ { - "schema_version": "1.1", + "schema_version": "1.2", "lens": "solution_simplicity", "candidate": { "head_sha": "3333333333333333333333333333333333333333", diff --git a/review-suite/fixtures/manifest.json b/review-suite/fixtures/manifest.json index 7205686..d4e27ff 100644 --- a/review-suite/fixtures/manifest.json +++ b/review-suite/fixtures/manifest.json @@ -14,6 +14,7 @@ {"name": "speculative-backfill", "packet_valid": true}, {"name": "missing-simplification-requirements", "packet_valid": true}, {"name": "clean-change", "packet_valid": true}, + {"name": "consumer-impact-traversal", "packet_valid": true}, {"name": "missing-evidence", "packet_valid": false}, {"name": "unrelated-base-drift", "packet_valid": true} ] diff --git a/review-suite/fixtures/missing-evidence/expected.json b/review-suite/fixtures/missing-evidence/expected.json index 42a35ba..0dbeecf 100644 --- a/review-suite/fixtures/missing-evidence/expected.json +++ b/review-suite/fixtures/missing-evidence/expected.json @@ -1,5 +1,5 @@ { - "schema_version": "1.1", + "schema_version": "1.2", "lens": "aggregate", "candidate": { "head_sha": "6666666666666666666666666666666666666666", diff --git a/review-suite/fixtures/missing-simplification-requirements/expected.json b/review-suite/fixtures/missing-simplification-requirements/expected.json index b4a9d19..88e5436 100644 --- a/review-suite/fixtures/missing-simplification-requirements/expected.json +++ b/review-suite/fixtures/missing-simplification-requirements/expected.json @@ -1,5 +1,5 @@ { - "schema_version": "1.1", + "schema_version": "1.2", "lens": "solution_simplicity", "candidate": { "head_sha": "8686868686868686868686868686868686868686", diff --git a/review-suite/fixtures/missing-test/expected.json b/review-suite/fixtures/missing-test/expected.json index d392f15..c37956b 100644 --- a/review-suite/fixtures/missing-test/expected.json +++ b/review-suite/fixtures/missing-test/expected.json @@ -1,5 +1,5 @@ { - "schema_version": "1.1", + "schema_version": "1.2", "lens": "correctness", "candidate": { "head_sha": "8282828282828282828282828282828282828282", diff --git a/review-suite/fixtures/multi-pass-control-flow/expected.json b/review-suite/fixtures/multi-pass-control-flow/expected.json index 59eba66..783d2b2 100644 --- a/review-suite/fixtures/multi-pass-control-flow/expected.json +++ b/review-suite/fixtures/multi-pass-control-flow/expected.json @@ -1,5 +1,5 @@ { - "schema_version": "1.1", + "schema_version": "1.2", "lens": "code_simplicity", "candidate": { "head_sha": "8888888888888888888888888888888888888888", diff --git a/review-suite/fixtures/necessary-complexity/expected.json b/review-suite/fixtures/necessary-complexity/expected.json index b8fee7b..b8fc73e 100644 --- a/review-suite/fixtures/necessary-complexity/expected.json +++ b/review-suite/fixtures/necessary-complexity/expected.json @@ -1,5 +1,5 @@ { - "schema_version": "1.1", + "schema_version": "1.2", "lens": "solution_simplicity", "candidate": { "head_sha": "4444444444444444444444444444444444444444", diff --git a/review-suite/fixtures/repository-convention-clean/expected.json b/review-suite/fixtures/repository-convention-clean/expected.json index aaea4c2..336d1bf 100644 --- a/review-suite/fixtures/repository-convention-clean/expected.json +++ b/review-suite/fixtures/repository-convention-clean/expected.json @@ -1,5 +1,5 @@ { - "schema_version": "1.1", + "schema_version": "1.2", "lens": "correctness", "candidate": { "head_sha": "8383838383838383838383838383838383838383", diff --git a/review-suite/fixtures/shared-test-setup/expected.json b/review-suite/fixtures/shared-test-setup/expected.json index f9ecb40..1ffb9d8 100644 --- a/review-suite/fixtures/shared-test-setup/expected.json +++ b/review-suite/fixtures/shared-test-setup/expected.json @@ -1,5 +1,5 @@ { - "schema_version": "1.1", + "schema_version": "1.2", "lens": "code_simplicity", "candidate": { "head_sha": "8a8a8a8a8a8a8a8a8a8a8a8a8a8a8a8a8a8a8a8a", diff --git a/review-suite/fixtures/speculative-backfill/expected.json b/review-suite/fixtures/speculative-backfill/expected.json index 1a335b3..c717a9b 100644 --- a/review-suite/fixtures/speculative-backfill/expected.json +++ b/review-suite/fixtures/speculative-backfill/expected.json @@ -1,5 +1,5 @@ { - "schema_version": "1.1", + "schema_version": "1.2", "lens": "solution_simplicity", "candidate": { "head_sha": "8585858585858585858585858585858585858585", diff --git a/review-suite/fixtures/unrelated-base-drift/expected.json b/review-suite/fixtures/unrelated-base-drift/expected.json index 4b9f523..95fabfa 100644 --- a/review-suite/fixtures/unrelated-base-drift/expected.json +++ b/review-suite/fixtures/unrelated-base-drift/expected.json @@ -1,5 +1,5 @@ { - "schema_version": "1.1", + "schema_version": "1.2", "lens": "aggregate", "candidate": { "head_sha": "7777777777777777777777777777777777777777", diff --git a/review-suite/scripts/evals/calibration.py b/review-suite/scripts/evals/calibration.py index 3a39b95..0fb2a3b 100644 --- a/review-suite/scripts/evals/calibration.py +++ b/review-suite/scripts/evals/calibration.py @@ -88,7 +88,7 @@ def kinds(self) -> frozenset[str]: def probe_result(probe: dict[str, Any], candidate: dict[str, Any]) -> dict[str, Any]: """Render one probe as a review result bound to the case's candidate.""" return { - "schema_version": "1.1", + "schema_version": "1.2", "lens": "aggregate", "candidate": candidate, "verdict": probe["verdict"], diff --git a/review-suite/scripts/evals/fixture_executor.py b/review-suite/scripts/evals/fixture_executor.py index ade073b..15a30b3 100644 --- a/review-suite/scripts/evals/fixture_executor.py +++ b/review-suite/scripts/evals/fixture_executor.py @@ -88,7 +88,7 @@ def _clean_lens_executions(candidate: dict[str, Any]) -> list[dict[str, Any]]: def _review(candidate: dict[str, Any], verdict: str, findings: list[dict[str, Any]]): result = { - "schema_version": "1.1", + "schema_version": "1.2", "lens": "aggregate", "candidate": dict(candidate), "verdict": verdict, diff --git a/review-suite/scripts/tests/test_contracts.py b/review-suite/scripts/tests/test_contracts.py index 2e80c1c..61b6e84 100644 --- a/review-suite/scripts/tests/test_contracts.py +++ b/review-suite/scripts/tests/test_contracts.py @@ -414,8 +414,9 @@ def test_unrelated_base_drift_remains_a_valid_clean_pair_at_v1_1(self): class StaleSchemaVersionTests(unittest.TestCase): - """#51 item 11: a stale v1.0 result is rejected with a useful error rather - than silently reinterpreted as v1.1 evidence.""" + """#51 item 11 and #52 item 5: a stale result is rejected with a useful + error rather than silently reinterpreted as newer evidence. Extended by + #52 to also reject a stale v1.1 aggregate now that schema 1.2 exists.""" def test_stale_v1_0_aggregate_result_is_rejected_with_a_useful_error(self): result = load(ROOT / "fixtures" / "clean-change" / "expected.json") @@ -425,6 +426,90 @@ def test_stale_v1_0_aggregate_result_is_rejected_with_a_useful_error(self): self.assertTrue(any("stale v1.0" in error for error in errors)) self.assertTrue(any("1.1" in error for error in errors)) + def test_stale_v1_1_aggregate_result_is_rejected_with_a_useful_error(self): + result = load(ROOT / "fixtures" / "clean-change" / "expected.json") + result["schema_version"] = "1.1" + errors = VALIDATOR.validate_result(result) + self.assertTrue(errors) + self.assertTrue(any("stale v1.1" in error for error in errors)) + self.assertTrue(any("1.2" in error for error in errors)) + + +class ConsumerImpactEvidenceTests(unittest.TestCase): + """#52: `consumer_impact_evidence` records a reviewer's traversal to other + call sites/consumers of a changed shared symbol, making that traversal + machine-checkable instead of an unenforced expectation a reviewer can + silently skip. The validator enforces structure and non-emptiness; it does + not determine which changed symbols require an entry (that is lens + judgment, owned by a later child).""" + + def setUp(self): + self.packet = load( + ROOT / "fixtures" / "consumer-impact-traversal" / "packet.json" + ) + self.result = load( + ROOT / "fixtures" / "consumer-impact-traversal" / "expected.json" + ) + + def test_sibling_call_site_correctly_identified_and_inspected_is_valid_clean( + self, + ): + # Required fixture 1: a changed shared symbol with a sibling call + # site, correctly identified and inspected in + # `consumer_impact_evidence`. + self.assertEqual([], VALIDATOR.validate_pair(self.packet, self.result)) + + def test_disposition_naming_only_the_hardened_call_site_is_rejected(self): + # Required fixture 2: the same fixture, but with the evidence entry + # naming only the hardened call site while still claiming the + # consumers are consistent. `all_consumers_consistent` describes at + # least one other consumer by definition, so it requires evidence + # covering more than the changed symbol's own location. + entry = self.result["consumer_impact_evidence"][0] + entry["consumer_search_evidence"] = entry["consumer_search_evidence"][:1] + errors = VALIDATOR.validate_result(self.result) + self.assertTrue(errors) + self.assertTrue( + any("claims other consumers were found" in error for error in errors) + ) + + def test_single_consumer_with_concrete_search_evidence_is_valid_clean(self): + # Required fixture 3: a changed symbol genuinely used from exactly one + # call site, with concrete search evidence showing no other + # consumers, disposition `no_other_consumers`. + self.result["consumer_impact_evidence"] = [ + self.result["consumer_impact_evidence"][1] + ] + self.assertEqual([], VALIDATOR.validate_result(self.result)) + + def test_no_other_consumers_without_search_evidence_is_rejected(self): + # Required fixture 4: a changed symbol with disposition + # `no_other_consumers` but no search evidence. + self.result["consumer_impact_evidence"][1]["consumer_search_evidence"] = [] + errors = VALIDATOR.validate_result(self.result) + self.assertTrue(errors) + self.assertTrue(any("expected at least 1 item" in error for error in errors)) + + def test_omitted_evidence_array_does_not_itself_fail_schema_validation(self): + # The validator cannot determine, from the result alone, which + # changed symbols require an entry; that judgment belongs to the lens + # performing the traversal (a later child). An aggregate clean result + # that omits `consumer_impact_evidence` entirely remains + # schema-valid; completeness of the traversal is a lens-judgment and + # forward-testing concern, not a mechanical validator check. + del self.result["consumer_impact_evidence"] + self.assertEqual([], VALIDATOR.validate_result(self.result)) + + def test_only_correctness_or_aggregate_results_may_include_the_evidence(self): + self.result["lens"] = "code_simplicity" + for finding in self.result.get("findings", []): + finding["lens"] = "code_simplicity" + errors = VALIDATOR.validate_result(self.result) + self.assertTrue(errors) + self.assertTrue( + any("only correctness or aggregate results" in error for error in errors) + ) + if __name__ == "__main__": unittest.main() diff --git a/review-suite/scripts/tests/test_eval_protocol.py b/review-suite/scripts/tests/test_eval_protocol.py index 5570408..3bbd314 100644 --- a/review-suite/scripts/tests/test_eval_protocol.py +++ b/review-suite/scripts/tests/test_eval_protocol.py @@ -370,7 +370,7 @@ def _valid_response(self, verdict="changes_required"): "expected_effect": "The requirement is met.", } result = { - "schema_version": "1.1", + "schema_version": "1.2", "lens": "aggregate", "candidate": candidate, "verdict": verdict, @@ -459,7 +459,7 @@ def test_valid_blocked_result_is_its_own_status(self): "simulation": False, "executor": {"name": "test"}, "result": { - "schema_version": "1.1", + "schema_version": "1.2", "lens": "aggregate", "candidate": { "head_sha": blocked_case.packet["candidate"]["head_sha"], diff --git a/review-suite/scripts/tests/test_eval_runner.py b/review-suite/scripts/tests/test_eval_runner.py index 0e99ed9..e2683e8 100644 --- a/review-suite/scripts/tests/test_eval_runner.py +++ b/review-suite/scripts/tests/test_eval_runner.py @@ -223,7 +223,7 @@ def test_a_merge_verdict_on_an_incomplete_packet_is_graded_not_failed(self): ' "outcome": "review_result", "simulation": False,\n' ' "executor": {"name": "always-gating"},\n' ' "result": {\n' - ' "schema_version": "1.1", "lens": "aggregate",\n' + ' "schema_version": "1.2", "lens": "aggregate",\n' ' "candidate": request["run"]["candidate"],\n' ' "verdict": "changes_required", "findings": [finding],\n' ' "blocking_reasons": [],\n' @@ -556,7 +556,7 @@ def test_the_adapter_completes_an_evaluation_through_the_protocol(self): ' for lens in ("solution_simplicity", "correctness", "code_simplicity")\n' "]\n" "result = {\n" - ' "schema_version": "1.1", "lens": "aggregate",\n' + ' "schema_version": "1.2", "lens": "aggregate",\n' ' "candidate": candidate, "verdict": "clean",\n' ' "findings": [], "blocking_reasons": [],\n' ' "lens_executions": lens_executions,\n' @@ -619,7 +619,7 @@ def _envelope_stub(self, envelope_body: str) -> Path: ' for lens in ("solution_simplicity", "correctness", "code_simplicity")\n' "]\n" "result = {\n" - ' "schema_version": "1.1", "lens": "aggregate",\n' + ' "schema_version": "1.2", "lens": "aggregate",\n' ' "candidate": candidate, "verdict": "clean",\n' ' "findings": [], "blocking_reasons": [],\n' ' "lens_executions": lens_executions,\n' diff --git a/review-suite/scripts/validate.py b/review-suite/scripts/validate.py index 033b586..8152448 100644 --- a/review-suite/scripts/validate.py +++ b/review-suite/scripts/validate.py @@ -35,7 +35,16 @@ def _schema_file(name: str) -> Path: REQUIRED_AGGREGATE_LENSES = ("solution_simplicity", "correctness", "code_simplicity") -STALE_RESULT_SCHEMA_VERSIONS = {"1.0"} +CONSUMER_IMPACT_DISPOSITIONS_IMPLYING_OTHER_CONSUMERS = ( + "all_consumers_consistent", + "inconsistency_found", +) + +# Maps each stale result schema version to the current version it must be +# migrated to. Extend this mapping, never overwrite it, on the next additive +# schema bump so every prior stale version keeps failing with its own useful +# migration error. +STALE_RESULT_SCHEMA_VERSIONS = {"1.0": "1.1", "1.1": "1.2"} BLOCKABLE_PACKET_ERROR_PATTERNS = ( re.compile( @@ -165,14 +174,15 @@ def validate_packet(packet: dict[str, Any]) -> list[str]: def validate_result(result: dict[str, Any]) -> list[str]: - if ( - isinstance(result, dict) - and result.get("schema_version") in STALE_RESULT_SCHEMA_VERSIONS - ): - return [ - "$.schema_version: stale v1.0 result rejected; v1.0 results are not " - "accepted as v1.1 evidence, rebuild review evidence at schema 1.1" - ] + if isinstance(result, dict): + stale_version = result.get("schema_version") + current_version = STALE_RESULT_SCHEMA_VERSIONS.get(stale_version) + if current_version is not None: + return [ + f"$.schema_version: stale v{stale_version} result rejected; " + f"v{stale_version} results are not accepted as v{current_version} " + f"evidence, rebuild review evidence at schema {current_version}" + ] schema = json.loads(SCHEMAS["result"].read_text()) errors = validate_schema(result, schema) if errors: @@ -230,11 +240,53 @@ def validate_result(result: dict[str, Any]) -> list[str]: + ", ".join(duplicate_dispositions) ) + errors.extend(_check_consumer_impact_evidence(result)) + if result.get("lens") == "aggregate" and verdict == "clean": errors.extend(_check_aggregate_clean_lens_executions(result)) return errors +def _check_consumer_impact_evidence(result: dict[str, Any]) -> list[str]: + """Check consumer/impact evidence structure and disposition consistency. + + #52: `consumer_impact_evidence` records a reviewer's traversal to other + call sites/consumers of a changed shared symbol, so that traversal is + machine-checkable instead of an unenforced expectation. The validator does + not determine which changed symbols require an entry — that judgment + belongs to the correctness lens's own traversal pass (a later child). It + only enforces that whatever is supplied is structurally trustworthy: a + disposition that claims other consumers exist must be backed by evidence + covering more than the changed symbol's own location, and every entry + (including `no_other_consumers`) must cite at least one concrete search. + """ + errors: list[str] = [] + entries = result.get("consumer_impact_evidence") + if not isinstance(entries, list): + return errors + if entries and result.get("lens") not in {"correctness", "aggregate"}: + errors.append( + "$.consumer_impact_evidence: only correctness or aggregate results " + "may include consumer/impact evidence" + ) + for index, entry in enumerate(entries): + if not isinstance(entry, dict): + continue + disposition = entry.get("disposition") + evidence = entry.get("consumer_search_evidence") + evidence_count = len(evidence) if isinstance(evidence, list) else 0 + if ( + disposition in CONSUMER_IMPACT_DISPOSITIONS_IMPLYING_OTHER_CONSUMERS + and evidence_count < 2 + ): + errors.append( + f"$.consumer_impact_evidence[{index}]: disposition {disposition!r} " + "claims other consumers were found and requires search evidence " + "covering more than the changed symbol's own location" + ) + return errors + + def _check_aggregate_clean_lens_executions(result: dict[str, Any]) -> list[str]: """Require one fresh current-head/current-base clean execution per lens. diff --git a/skills/review-code-change/evals/cases.json b/skills/review-code-change/evals/cases.json index 5d30aed..d876c8e 100644 --- a/skills/review-code-change/evals/cases.json +++ b/skills/review-code-change/evals/cases.json @@ -5,9 +5,9 @@ "available_dependencies": ["review-solution-simplicity", "review-correctness", "review-code-simplicity"], "evidence": {"complete": true}, "cycle": 1, "phase": "full", "harness_outcomes": { - "solution_simplicity": {"schema_version": "1.1", "lens": "solution_simplicity", "candidate": {"head_sha": "1010101010101010101010101010101010101010", "comparison_base_sha": "a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0"}, "verdict": "clean", "findings": [], "blocking_reasons": []}, - "correctness": {"schema_version": "1.1", "lens": "correctness", "candidate": {"head_sha": "1010101010101010101010101010101010101010", "comparison_base_sha": "a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0"}, "verdict": "clean", "findings": [], "blocking_reasons": []}, - "code_simplicity": {"schema_version": "1.1", "lens": "code_simplicity", "candidate": {"head_sha": "1010101010101010101010101010101010101010", "comparison_base_sha": "a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0"}, "verdict": "clean", "findings": [], "blocking_reasons": []} + "solution_simplicity": {"schema_version": "1.2", "lens": "solution_simplicity", "candidate": {"head_sha": "1010101010101010101010101010101010101010", "comparison_base_sha": "a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0"}, "verdict": "clean", "findings": [], "blocking_reasons": []}, + "correctness": {"schema_version": "1.2", "lens": "correctness", "candidate": {"head_sha": "1010101010101010101010101010101010101010", "comparison_base_sha": "a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0"}, "verdict": "clean", "findings": [], "blocking_reasons": []}, + "code_simplicity": {"schema_version": "1.2", "lens": "code_simplicity", "candidate": {"head_sha": "1010101010101010101010101010101010101010", "comparison_base_sha": "a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0"}, "verdict": "clean", "findings": [], "blocking_reasons": []} } }, { @@ -16,9 +16,9 @@ "available_dependencies": ["review-solution-simplicity", "review-correctness", "review-code-simplicity"], "evidence": {"complete": true}, "cycle": 1, "phase": "full", "harness_outcomes": { - "solution_simplicity": {"schema_version": "1.1", "lens": "solution_simplicity", "candidate": {"head_sha": "2020202020202020202020202020202020202020", "comparison_base_sha": "b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0"}, "verdict": "changes_required", "findings": [{"id": "solution-simplicity.remove-provider-platform", "lens": "solution_simplicity", "severity": "blocking", "confidence": "high", "rule": "Remote providers are out of scope.", "evidence": [{"location": "candidate diff", "detail": "A registry, provider API, configuration, and dispatch exist for one local write."}], "concern": "The candidate implements an out-of-scope provider platform.", "impact": "It adds configuration, dispatch, and provider failure modes.", "proposed_change": "Replace the provider platform with one direct local write.", "expected_effect": "Remove the platform, configuration, and provider failure modes."}], "blocking_reasons": [], "next_action": "Redesign the implementation and restart the full review on a new head."}, - "correctness": {"schema_version": "1.1", "lens": "correctness", "candidate": {"head_sha": "2020202020202020202020202020202020202020", "comparison_base_sha": "b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0"}, "verdict": "clean", "findings": [], "blocking_reasons": []}, - "code_simplicity": {"schema_version": "1.1", "lens": "code_simplicity", "candidate": {"head_sha": "2020202020202020202020202020202020202020", "comparison_base_sha": "b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0"}, "verdict": "clean", "findings": [], "blocking_reasons": []} + "solution_simplicity": {"schema_version": "1.2", "lens": "solution_simplicity", "candidate": {"head_sha": "2020202020202020202020202020202020202020", "comparison_base_sha": "b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0"}, "verdict": "changes_required", "findings": [{"id": "solution-simplicity.remove-provider-platform", "lens": "solution_simplicity", "severity": "blocking", "confidence": "high", "rule": "Remote providers are out of scope.", "evidence": [{"location": "candidate diff", "detail": "A registry, provider API, configuration, and dispatch exist for one local write."}], "concern": "The candidate implements an out-of-scope provider platform.", "impact": "It adds configuration, dispatch, and provider failure modes.", "proposed_change": "Replace the provider platform with one direct local write.", "expected_effect": "Remove the platform, configuration, and provider failure modes."}], "blocking_reasons": [], "next_action": "Redesign the implementation and restart the full review on a new head."}, + "correctness": {"schema_version": "1.2", "lens": "correctness", "candidate": {"head_sha": "2020202020202020202020202020202020202020", "comparison_base_sha": "b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0"}, "verdict": "clean", "findings": [], "blocking_reasons": []}, + "code_simplicity": {"schema_version": "1.2", "lens": "code_simplicity", "candidate": {"head_sha": "2020202020202020202020202020202020202020", "comparison_base_sha": "b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0"}, "verdict": "clean", "findings": [], "blocking_reasons": []} } }, { @@ -27,9 +27,9 @@ "available_dependencies": ["review-solution-simplicity", "review-correctness", "review-code-simplicity"], "evidence": {"complete": true}, "cycle": 1, "phase": "full", "harness_outcomes": { - "solution_simplicity": {"schema_version": "1.1", "lens": "solution_simplicity", "candidate": {"head_sha": "3030303030303030303030303030303030303030", "comparison_base_sha": "c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0"}, "verdict": "changes_required", "findings": [{"id": "solution-simplicity.remove-conditional-write", "lens": "solution_simplicity", "severity": "strong_recommendation", "confidence": "medium", "rule": "Prefer a direct status update when extra predicates are unnecessary.", "evidence": [{"location": "candidate update", "detail": "The conditional update has more predicates than a direct update."}], "concern": "The predicates may be unnecessary local machinery.", "impact": "They add conditions and a failure path.", "proposed_change": "Remove claim-token and running-state predicates.", "expected_effect": "Fewer predicates and one less failure path."}], "blocking_reasons": [], "next_action": "Continue to correctness before accepting this in-strategy simplification."}, - "correctness": {"schema_version": "1.1", "lens": "correctness", "candidate": {"head_sha": "3030303030303030303030303030303030303030", "comparison_base_sha": "c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0"}, "verdict": "clean", "findings": [], "blocking_reasons": [], "proposal_dispositions": [{"finding_id": "solution-simplicity.remove-conditional-write", "source_lens": "solution_simplicity", "disposition": "unsafe", "reason": "The proposed removal would discard the required atomic claim fence; the current candidate already preserves it.", "evidence": [{"location": "candidate conditional update", "detail": "The claim-token and running-state predicates prevent an expired worker from completing another worker's claim."}]}], "next_action": "Reject the unsafe simplification and continue reviewing the unchanged candidate."}, - "code_simplicity": {"schema_version": "1.1", "lens": "code_simplicity", "candidate": {"head_sha": "3030303030303030303030303030303030303030", "comparison_base_sha": "c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0"}, "verdict": "clean", "findings": [], "blocking_reasons": []} + "solution_simplicity": {"schema_version": "1.2", "lens": "solution_simplicity", "candidate": {"head_sha": "3030303030303030303030303030303030303030", "comparison_base_sha": "c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0"}, "verdict": "changes_required", "findings": [{"id": "solution-simplicity.remove-conditional-write", "lens": "solution_simplicity", "severity": "strong_recommendation", "confidence": "medium", "rule": "Prefer a direct status update when extra predicates are unnecessary.", "evidence": [{"location": "candidate update", "detail": "The conditional update has more predicates than a direct update."}], "concern": "The predicates may be unnecessary local machinery.", "impact": "They add conditions and a failure path.", "proposed_change": "Remove claim-token and running-state predicates.", "expected_effect": "Fewer predicates and one less failure path."}], "blocking_reasons": [], "next_action": "Continue to correctness before accepting this in-strategy simplification."}, + "correctness": {"schema_version": "1.2", "lens": "correctness", "candidate": {"head_sha": "3030303030303030303030303030303030303030", "comparison_base_sha": "c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0"}, "verdict": "clean", "findings": [], "blocking_reasons": [], "proposal_dispositions": [{"finding_id": "solution-simplicity.remove-conditional-write", "source_lens": "solution_simplicity", "disposition": "unsafe", "reason": "The proposed removal would discard the required atomic claim fence; the current candidate already preserves it.", "evidence": [{"location": "candidate conditional update", "detail": "The claim-token and running-state predicates prevent an expired worker from completing another worker's claim."}]}], "next_action": "Reject the unsafe simplification and continue reviewing the unchanged candidate."}, + "code_simplicity": {"schema_version": "1.2", "lens": "code_simplicity", "candidate": {"head_sha": "3030303030303030303030303030303030303030", "comparison_base_sha": "c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0"}, "verdict": "clean", "findings": [], "blocking_reasons": []} } }, { @@ -38,9 +38,9 @@ "available_dependencies": ["review-solution-simplicity", "review-correctness", "review-code-simplicity"], "evidence": {"complete": true}, "cycle": 1, "phase": "full", "harness_outcomes": { - "solution_simplicity": {"schema_version": "1.1", "lens": "solution_simplicity", "candidate": {"head_sha": "4040404040404040404040404040404040404040", "comparison_base_sha": "d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0"}, "verdict": "changes_required", "findings": [{"id": "solution-simplicity.remove-local-policy-service", "lens": "solution_simplicity", "severity": "strong_recommendation", "confidence": "high", "rule": "Use the existing authorization boundary.", "evidence": [{"location": "policy service", "detail": "A new service duplicates the active-admin rule."}], "concern": "The service duplicates repository policy.", "impact": "It adds a service and another policy owner.", "proposed_change": "Call require_active_admin directly.", "expected_effect": "Remove one service and one duplicate policy."}], "blocking_reasons": [], "next_action": "Continue through correctness because this is a bounded in-strategy removal."}, - "correctness": {"schema_version": "1.1", "lens": "correctness", "candidate": {"head_sha": "4040404040404040404040404040404040404040", "comparison_base_sha": "d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0"}, "verdict": "clean", "findings": [], "blocking_reasons": [], "proposal_dispositions": [{"finding_id": "solution-simplicity.remove-local-policy-service", "source_lens": "solution_simplicity", "disposition": "compatible", "reason": "Reusing the existing active-admin helper preserves the repository's authorization boundary.", "evidence": [{"location": "repository policy helper", "detail": "require_active_admin implements the same required authorization check."}]}]}, - "code_simplicity": {"schema_version": "1.1", "lens": "code_simplicity", "candidate": {"head_sha": "4040404040404040404040404040404040404040", "comparison_base_sha": "d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0"}, "verdict": "changes_required", "findings": [{"id": "code-simplicity.reuse-active-admin", "lens": "code_simplicity", "severity": "strong_recommendation", "confidence": "high", "rule": "Reuse repository policy helpers.", "evidence": [{"location": "new actions", "detail": "Two actions repeat the active-admin condition."}], "concern": "The actions duplicate the same repository policy.", "impact": "Three policy definitions can drift.", "proposed_change": "Call require_active_admin directly.", "expected_effect": "Reduce three policy definitions to one."}], "blocking_reasons": []} + "solution_simplicity": {"schema_version": "1.2", "lens": "solution_simplicity", "candidate": {"head_sha": "4040404040404040404040404040404040404040", "comparison_base_sha": "d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0"}, "verdict": "changes_required", "findings": [{"id": "solution-simplicity.remove-local-policy-service", "lens": "solution_simplicity", "severity": "strong_recommendation", "confidence": "high", "rule": "Use the existing authorization boundary.", "evidence": [{"location": "policy service", "detail": "A new service duplicates the active-admin rule."}], "concern": "The service duplicates repository policy.", "impact": "It adds a service and another policy owner.", "proposed_change": "Call require_active_admin directly.", "expected_effect": "Remove one service and one duplicate policy."}], "blocking_reasons": [], "next_action": "Continue through correctness because this is a bounded in-strategy removal."}, + "correctness": {"schema_version": "1.2", "lens": "correctness", "candidate": {"head_sha": "4040404040404040404040404040404040404040", "comparison_base_sha": "d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0"}, "verdict": "clean", "findings": [], "blocking_reasons": [], "proposal_dispositions": [{"finding_id": "solution-simplicity.remove-local-policy-service", "source_lens": "solution_simplicity", "disposition": "compatible", "reason": "Reusing the existing active-admin helper preserves the repository's authorization boundary.", "evidence": [{"location": "repository policy helper", "detail": "require_active_admin implements the same required authorization check."}]}]}, + "code_simplicity": {"schema_version": "1.2", "lens": "code_simplicity", "candidate": {"head_sha": "4040404040404040404040404040404040404040", "comparison_base_sha": "d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0"}, "verdict": "changes_required", "findings": [{"id": "code-simplicity.reuse-active-admin", "lens": "code_simplicity", "severity": "strong_recommendation", "confidence": "high", "rule": "Reuse repository policy helpers.", "evidence": [{"location": "new actions", "detail": "Two actions repeat the active-admin condition."}], "concern": "The actions duplicate the same repository policy.", "impact": "Three policy definitions can drift.", "proposed_change": "Call require_active_admin directly.", "expected_effect": "Reduce three policy definitions to one."}], "blocking_reasons": []} } }, { @@ -49,9 +49,9 @@ "available_dependencies": ["review-solution-simplicity", "review-correctness", "review-code-simplicity"], "evidence": {"complete": true, "new_packet_after_code_simplification": true}, "cycle": 2, "phase": "post_code_simplification_full_restart", "harness_outcomes": { - "solution_simplicity": {"schema_version": "1.1", "lens": "solution_simplicity", "candidate": {"head_sha": "5050505050505050505050505050505050505050", "comparison_base_sha": "e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0"}, "verdict": "clean", "findings": [], "blocking_reasons": []}, - "correctness": {"schema_version": "1.1", "lens": "correctness", "candidate": {"head_sha": "5050505050505050505050505050505050505050", "comparison_base_sha": "e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0"}, "verdict": "clean", "findings": [], "blocking_reasons": []}, - "code_simplicity": {"schema_version": "1.1", "lens": "code_simplicity", "candidate": {"head_sha": "5050505050505050505050505050505050505050", "comparison_base_sha": "e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0"}, "verdict": "clean", "findings": [], "blocking_reasons": []} + "solution_simplicity": {"schema_version": "1.2", "lens": "solution_simplicity", "candidate": {"head_sha": "5050505050505050505050505050505050505050", "comparison_base_sha": "e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0"}, "verdict": "clean", "findings": [], "blocking_reasons": []}, + "correctness": {"schema_version": "1.2", "lens": "correctness", "candidate": {"head_sha": "5050505050505050505050505050505050505050", "comparison_base_sha": "e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0"}, "verdict": "clean", "findings": [], "blocking_reasons": []}, + "code_simplicity": {"schema_version": "1.2", "lens": "code_simplicity", "candidate": {"head_sha": "5050505050505050505050505050505050505050", "comparison_base_sha": "e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0"}, "verdict": "clean", "findings": [], "blocking_reasons": []} } }, { @@ -60,9 +60,9 @@ "available_dependencies": ["review-solution-simplicity", "review-correctness", "review-code-simplicity"], "evidence": {"complete": true, "new_packet_after_correctness_fix": true}, "cycle": 2, "phase": "post_correctness_fix_full_restart", "harness_outcomes": { - "solution_simplicity": {"schema_version": "1.1", "lens": "solution_simplicity", "candidate": {"head_sha": "5151515151515151515151515151515151515151", "comparison_base_sha": "e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1"}, "verdict": "clean", "findings": [], "blocking_reasons": []}, - "correctness": {"schema_version": "1.1", "lens": "correctness", "candidate": {"head_sha": "5151515151515151515151515151515151515151", "comparison_base_sha": "e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1"}, "verdict": "clean", "findings": [], "blocking_reasons": []}, - "code_simplicity": {"schema_version": "1.1", "lens": "code_simplicity", "candidate": {"head_sha": "5151515151515151515151515151515151515151", "comparison_base_sha": "e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1"}, "verdict": "clean", "findings": [], "blocking_reasons": []} + "solution_simplicity": {"schema_version": "1.2", "lens": "solution_simplicity", "candidate": {"head_sha": "5151515151515151515151515151515151515151", "comparison_base_sha": "e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1"}, "verdict": "clean", "findings": [], "blocking_reasons": []}, + "correctness": {"schema_version": "1.2", "lens": "correctness", "candidate": {"head_sha": "5151515151515151515151515151515151515151", "comparison_base_sha": "e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1"}, "verdict": "clean", "findings": [], "blocking_reasons": []}, + "code_simplicity": {"schema_version": "1.2", "lens": "code_simplicity", "candidate": {"head_sha": "5151515151515151515151515151515151515151", "comparison_base_sha": "e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1"}, "verdict": "clean", "findings": [], "blocking_reasons": []} } }, { @@ -84,9 +84,9 @@ "available_dependencies": ["review-solution-simplicity", "review-correctness", "review-code-simplicity"], "evidence": {"complete": true}, "cycle": 1, "phase": "full", "harness_outcomes": { - "solution_simplicity": {"schema_version": "1.1", "lens": "solution_simplicity", "candidate": {"head_sha": "8080808080808080808080808080808080808080", "comparison_base_sha": "b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1"}, "verdict": "clean", "findings": [{"id": "solution-simplicity.future-cleanup", "lens": "solution_simplicity", "severity": "defer", "confidence": "high", "rule": "A real legacy path remains outside this ticket.", "evidence": [{"location": "ticket", "detail": "Migration cleanup is explicitly deferred to issue #90."}], "concern": "A compatibility path remains outside active scope.", "impact": "The branch remains until callers migrate.", "proposed_change": "Remove the path in issue #90.", "expected_effect": "Retire one compatibility branch after migration."}], "blocking_reasons": []}, - "correctness": {"schema_version": "1.1", "lens": "correctness", "candidate": {"head_sha": "8080808080808080808080808080808080808080", "comparison_base_sha": "b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1"}, "verdict": "clean", "findings": [], "blocking_reasons": []}, - "code_simplicity": {"schema_version": "1.1", "lens": "code_simplicity", "candidate": {"head_sha": "8080808080808080808080808080808080808080", "comparison_base_sha": "b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1"}, "verdict": "clean", "findings": [], "blocking_reasons": []} + "solution_simplicity": {"schema_version": "1.2", "lens": "solution_simplicity", "candidate": {"head_sha": "8080808080808080808080808080808080808080", "comparison_base_sha": "b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1"}, "verdict": "clean", "findings": [{"id": "solution-simplicity.future-cleanup", "lens": "solution_simplicity", "severity": "defer", "confidence": "high", "rule": "A real legacy path remains outside this ticket.", "evidence": [{"location": "ticket", "detail": "Migration cleanup is explicitly deferred to issue #90."}], "concern": "A compatibility path remains outside active scope.", "impact": "The branch remains until callers migrate.", "proposed_change": "Remove the path in issue #90.", "expected_effect": "Retire one compatibility branch after migration."}], "blocking_reasons": []}, + "correctness": {"schema_version": "1.2", "lens": "correctness", "candidate": {"head_sha": "8080808080808080808080808080808080808080", "comparison_base_sha": "b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1"}, "verdict": "clean", "findings": [], "blocking_reasons": []}, + "code_simplicity": {"schema_version": "1.2", "lens": "code_simplicity", "candidate": {"head_sha": "8080808080808080808080808080808080808080", "comparison_base_sha": "b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1"}, "verdict": "clean", "findings": [], "blocking_reasons": []} } }, { @@ -95,9 +95,9 @@ "available_dependencies": ["review-solution-simplicity", "review-correctness", "review-code-simplicity"], "evidence": {"complete": true}, "cycle": 3, "phase": "full", "harness_outcomes": { - "solution_simplicity": {"schema_version": "1.1", "lens": "solution_simplicity", "candidate": {"head_sha": "9090909090909090909090909090909090909090", "comparison_base_sha": "c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1"}, "verdict": "clean", "findings": [], "blocking_reasons": []}, - "correctness": {"schema_version": "1.1", "lens": "correctness", "candidate": {"head_sha": "9090909090909090909090909090909090909090", "comparison_base_sha": "c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1"}, "verdict": "changes_required", "findings": [{"id": "correctness.retry-still-unbounded", "lens": "correctness", "severity": "blocking", "confidence": "high", "rule": "Retries must stop at the configured maximum.", "evidence": [{"location": "candidate diff", "detail": "The third candidate still schedules after the maximum."}], "concern": "Retry scheduling remains unbounded.", "impact": "The required retry limit is violated.", "proposed_change": "Stop scheduling at max_retries.", "expected_effect": "Bound repeated work at the required limit."}], "blocking_reasons": []}, - "code_simplicity": {"schema_version": "1.1", "lens": "code_simplicity", "candidate": {"head_sha": "9090909090909090909090909090909090909090", "comparison_base_sha": "c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1"}, "verdict": "clean", "findings": [], "blocking_reasons": []} + "solution_simplicity": {"schema_version": "1.2", "lens": "solution_simplicity", "candidate": {"head_sha": "9090909090909090909090909090909090909090", "comparison_base_sha": "c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1"}, "verdict": "clean", "findings": [], "blocking_reasons": []}, + "correctness": {"schema_version": "1.2", "lens": "correctness", "candidate": {"head_sha": "9090909090909090909090909090909090909090", "comparison_base_sha": "c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1"}, "verdict": "changes_required", "findings": [{"id": "correctness.retry-still-unbounded", "lens": "correctness", "severity": "blocking", "confidence": "high", "rule": "Retries must stop at the configured maximum.", "evidence": [{"location": "candidate diff", "detail": "The third candidate still schedules after the maximum."}], "concern": "Retry scheduling remains unbounded.", "impact": "The required retry limit is violated.", "proposed_change": "Stop scheduling at max_retries.", "expected_effect": "Bound repeated work at the required limit."}], "blocking_reasons": []}, + "code_simplicity": {"schema_version": "1.2", "lens": "code_simplicity", "candidate": {"head_sha": "9090909090909090909090909090909090909090", "comparison_base_sha": "c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1"}, "verdict": "clean", "findings": [], "blocking_reasons": []} } }, { @@ -106,9 +106,9 @@ "available_dependencies": ["review-solution-simplicity", "review-correctness", "review-code-simplicity"], "evidence": {"complete": true}, "cycle": 1, "phase": "full", "harness_outcomes": { - "solution_simplicity": {"schema_version": "1.1", "lens": "correctness", "candidate": {"head_sha": "deadbeefdeadbeefdeadbeefdeadbeefdeadbeef", "comparison_base_sha": "b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2"}, "verdict": "clean", "findings": [], "blocking_reasons": []}, - "correctness": {"schema_version": "1.1", "lens": "correctness", "candidate": {"head_sha": "a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2", "comparison_base_sha": "b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2"}, "verdict": "clean", "findings": [], "blocking_reasons": []}, - "code_simplicity": {"schema_version": "1.1", "lens": "code_simplicity", "candidate": {"head_sha": "a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2", "comparison_base_sha": "b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2"}, "verdict": "clean", "findings": [], "blocking_reasons": []} + "solution_simplicity": {"schema_version": "1.2", "lens": "correctness", "candidate": {"head_sha": "deadbeefdeadbeefdeadbeefdeadbeefdeadbeef", "comparison_base_sha": "b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2"}, "verdict": "clean", "findings": [], "blocking_reasons": []}, + "correctness": {"schema_version": "1.2", "lens": "correctness", "candidate": {"head_sha": "a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2", "comparison_base_sha": "b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2"}, "verdict": "clean", "findings": [], "blocking_reasons": []}, + "code_simplicity": {"schema_version": "1.2", "lens": "code_simplicity", "candidate": {"head_sha": "a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2", "comparison_base_sha": "b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2"}, "verdict": "clean", "findings": [], "blocking_reasons": []} } } ] diff --git a/skills/review-code-change/evals/expectations.json b/skills/review-code-change/evals/expectations.json index 54a91e1..17d917e 100644 --- a/skills/review-code-change/evals/expectations.json +++ b/skills/review-code-change/evals/expectations.json @@ -8,7 +8,7 @@ ], "stopped_reason": "all_required_lenses_clean", "result": { - "schema_version": "1.1", + "schema_version": "1.2", "lens": "aggregate", "candidate": { "head_sha": "1010101010101010101010101010101010101010", @@ -50,7 +50,7 @@ ], "stopped_reason": "solution_strategy_replacement_required", "result": { - "schema_version": "1.1", + "schema_version": "1.2", "lens": "aggregate", "candidate": { "head_sha": "2020202020202020202020202020202020202020", @@ -89,7 +89,7 @@ ], "stopped_reason": "unsafe_simplification_rejected_and_remaining_lenses_clean", "result": { - "schema_version": "1.1", + "schema_version": "1.2", "lens": "aggregate", "candidate": { "head_sha": "3030303030303030303030303030303030303030", @@ -147,7 +147,7 @@ ], "stopped_reason": "code_simplicity_gating_finding", "result": { - "schema_version": "1.1", + "schema_version": "1.2", "lens": "aggregate", "candidate": { "head_sha": "4040404040404040404040404040404040404040", @@ -207,7 +207,7 @@ ], "stopped_reason": "all_required_results_clean_after_full_restart", "result": { - "schema_version": "1.1", + "schema_version": "1.2", "lens": "aggregate", "candidate": { "head_sha": "5050505050505050505050505050505050505050", @@ -247,7 +247,7 @@ "observed_sequence": [], "stopped_reason": "missing_required_dependency", "result": { - "schema_version": "1.1", + "schema_version": "1.2", "lens": "aggregate", "candidate": { "head_sha": "6060606060606060606060606060606060606060", @@ -266,7 +266,7 @@ "observed_sequence": [], "stopped_reason": "evidence_packet_incomplete", "result": { - "schema_version": "1.1", + "schema_version": "1.2", "lens": "aggregate", "candidate": { "head_sha": "7070707070707070707070707070707070707070", @@ -293,7 +293,7 @@ ], "stopped_reason": "all_required_lenses_clean_with_deferred_finding", "result": { - "schema_version": "1.1", + "schema_version": "1.2", "lens": "aggregate", "candidate": { "head_sha": "8080808080808080808080808080808080808080", @@ -354,7 +354,7 @@ ], "stopped_reason": "cycle_budget_exhausted_with_correctness_finding", "result": { - "schema_version": "1.1", + "schema_version": "1.2", "lens": "aggregate", "candidate": { "head_sha": "9090909090909090909090909090909090909090", @@ -391,7 +391,7 @@ ], "stopped_reason": "untrustworthy_lens_result", "result": { - "schema_version": "1.1", + "schema_version": "1.2", "lens": "aggregate", "candidate": { "head_sha": "a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2", @@ -416,7 +416,7 @@ ], "stopped_reason": "all_required_results_clean_after_full_restart", "result": { - "schema_version": "1.1", + "schema_version": "1.2", "lens": "aggregate", "candidate": { "head_sha": "5151515151515151515151515151515151515151", diff --git a/skills/review-code-change/evals/expected/standalone-clean.result.json b/skills/review-code-change/evals/expected/standalone-clean.result.json index 4eed3be..7265a8d 100644 --- a/skills/review-code-change/evals/expected/standalone-clean.result.json +++ b/skills/review-code-change/evals/expected/standalone-clean.result.json @@ -5,7 +5,7 @@ "code_simplicity" ], "result": { - "schema_version": "1.1", + "schema_version": "1.2", "lens": "aggregate", "candidate": { "head_sha": "1212121212121212121212121212121212121212", diff --git a/skills/review-code-change/references/review-suite/CONTRACT.md b/skills/review-code-change/references/review-suite/CONTRACT.md index bdced9f..7501fa2 100644 --- a/skills/review-code-change/references/review-suite/CONTRACT.md +++ b/skills/review-code-change/references/review-suite/CONTRACT.md @@ -163,6 +163,32 @@ after a correctness fix, or only code simplicity and correctness after a code-simplicity fix) cannot produce a valid `clean` aggregate. This child defines no selective-reuse exception across different heads. +### Consumer/impact evidence + +A `correctness` or `aggregate` result may record `consumer_impact_evidence`: one +entry per changed shared symbol or contract whose other call sites/consumers +were traversed, each naming the `changed_symbol`, its defining `location`, the +`consumer_search_evidence` inspected (one or more `location` + `detail` pairs +describing what was found), and a `disposition` of `all_consumers_consistent`, +`inconsistency_found`, or `no_other_consumers`. + +This makes a reviewer's consumer/impact traversal machine-checkable instead of +an unenforced expectation a reviewer can silently skip. The validator enforces +structure and non-emptiness; it does not determine which changed symbols require +an entry — that judgment belongs to the lens performing the traversal (a later +child consumes this schema to populate it). Given a supplied entry: + +- only `correctness` or `aggregate` results may include this evidence; +- every entry requires at least one concrete `consumer_search_evidence` item, + mirroring the existing "empty impact is valid only with concrete search + evidence" principle used elsewhere in this contract family — a disposition is + never accepted on the strength of an unevidenced claim; and +- `all_consumers_consistent` and `inconsistency_found` describe at least one + other consumer by definition, so each requires search evidence covering more + than the changed symbol's own location (two or more entries); + `no_other_consumers` requires only the one concrete search that found nothing + else. + ## Simplification proposal dispositions When an orchestrator asks correctness to assess a validated simplification diff --git a/skills/review-code-change/references/review-suite/review-result.schema.json b/skills/review-code-change/references/review-suite/review-result.schema.json index 47e9a38..0af0e91 100644 --- a/skills/review-code-change/references/review-suite/review-result.schema.json +++ b/skills/review-code-change/references/review-suite/review-result.schema.json @@ -13,7 +13,7 @@ "blocking_reasons" ], "properties": { - "schema_version": {"const": "1.1"}, + "schema_version": {"const": "1.2"}, "lens": { "enum": [ "correctness", @@ -108,6 +108,43 @@ "type": "array", "items": {"type": "string", "minLength": 1} }, + "consumer_impact_evidence": { + "type": "array", + "items": { + "type": "object", + "additionalProperties": false, + "required": [ + "changed_symbol", + "location", + "consumer_search_evidence", + "disposition" + ], + "properties": { + "changed_symbol": {"type": "string", "minLength": 1}, + "location": {"type": "string", "minLength": 1}, + "consumer_search_evidence": { + "type": "array", + "minItems": 1, + "items": { + "type": "object", + "additionalProperties": false, + "required": ["location", "detail"], + "properties": { + "location": {"type": "string", "minLength": 1}, + "detail": {"type": "string", "minLength": 1} + } + } + }, + "disposition": { + "enum": [ + "all_consumers_consistent", + "inconsistency_found", + "no_other_consumers" + ] + } + } + } + }, "proposal_dispositions": { "type": "array", "items": { diff --git a/skills/review-code-change/references/review-suite/validate.py b/skills/review-code-change/references/review-suite/validate.py index 033b586..8152448 100644 --- a/skills/review-code-change/references/review-suite/validate.py +++ b/skills/review-code-change/references/review-suite/validate.py @@ -35,7 +35,16 @@ def _schema_file(name: str) -> Path: REQUIRED_AGGREGATE_LENSES = ("solution_simplicity", "correctness", "code_simplicity") -STALE_RESULT_SCHEMA_VERSIONS = {"1.0"} +CONSUMER_IMPACT_DISPOSITIONS_IMPLYING_OTHER_CONSUMERS = ( + "all_consumers_consistent", + "inconsistency_found", +) + +# Maps each stale result schema version to the current version it must be +# migrated to. Extend this mapping, never overwrite it, on the next additive +# schema bump so every prior stale version keeps failing with its own useful +# migration error. +STALE_RESULT_SCHEMA_VERSIONS = {"1.0": "1.1", "1.1": "1.2"} BLOCKABLE_PACKET_ERROR_PATTERNS = ( re.compile( @@ -165,14 +174,15 @@ def validate_packet(packet: dict[str, Any]) -> list[str]: def validate_result(result: dict[str, Any]) -> list[str]: - if ( - isinstance(result, dict) - and result.get("schema_version") in STALE_RESULT_SCHEMA_VERSIONS - ): - return [ - "$.schema_version: stale v1.0 result rejected; v1.0 results are not " - "accepted as v1.1 evidence, rebuild review evidence at schema 1.1" - ] + if isinstance(result, dict): + stale_version = result.get("schema_version") + current_version = STALE_RESULT_SCHEMA_VERSIONS.get(stale_version) + if current_version is not None: + return [ + f"$.schema_version: stale v{stale_version} result rejected; " + f"v{stale_version} results are not accepted as v{current_version} " + f"evidence, rebuild review evidence at schema {current_version}" + ] schema = json.loads(SCHEMAS["result"].read_text()) errors = validate_schema(result, schema) if errors: @@ -230,11 +240,53 @@ def validate_result(result: dict[str, Any]) -> list[str]: + ", ".join(duplicate_dispositions) ) + errors.extend(_check_consumer_impact_evidence(result)) + if result.get("lens") == "aggregate" and verdict == "clean": errors.extend(_check_aggregate_clean_lens_executions(result)) return errors +def _check_consumer_impact_evidence(result: dict[str, Any]) -> list[str]: + """Check consumer/impact evidence structure and disposition consistency. + + #52: `consumer_impact_evidence` records a reviewer's traversal to other + call sites/consumers of a changed shared symbol, so that traversal is + machine-checkable instead of an unenforced expectation. The validator does + not determine which changed symbols require an entry — that judgment + belongs to the correctness lens's own traversal pass (a later child). It + only enforces that whatever is supplied is structurally trustworthy: a + disposition that claims other consumers exist must be backed by evidence + covering more than the changed symbol's own location, and every entry + (including `no_other_consumers`) must cite at least one concrete search. + """ + errors: list[str] = [] + entries = result.get("consumer_impact_evidence") + if not isinstance(entries, list): + return errors + if entries and result.get("lens") not in {"correctness", "aggregate"}: + errors.append( + "$.consumer_impact_evidence: only correctness or aggregate results " + "may include consumer/impact evidence" + ) + for index, entry in enumerate(entries): + if not isinstance(entry, dict): + continue + disposition = entry.get("disposition") + evidence = entry.get("consumer_search_evidence") + evidence_count = len(evidence) if isinstance(evidence, list) else 0 + if ( + disposition in CONSUMER_IMPACT_DISPOSITIONS_IMPLYING_OTHER_CONSUMERS + and evidence_count < 2 + ): + errors.append( + f"$.consumer_impact_evidence[{index}]: disposition {disposition!r} " + "claims other consumers were found and requires search evidence " + "covering more than the changed symbol's own location" + ) + return errors + + def _check_aggregate_clean_lens_executions(result: dict[str, Any]) -> list[str]: """Require one fresh current-head/current-base clean execution per lens. diff --git a/skills/review-code-simplicity/evals/expected/standalone-duplicated-policy.result.json b/skills/review-code-simplicity/evals/expected/standalone-duplicated-policy.result.json index 7d6b5f7..8197552 100644 --- a/skills/review-code-simplicity/evals/expected/standalone-duplicated-policy.result.json +++ b/skills/review-code-simplicity/evals/expected/standalone-duplicated-policy.result.json @@ -1,5 +1,5 @@ { - "schema_version": "1.1", + "schema_version": "1.2", "lens": "code_simplicity", "candidate": { "head_sha": "8d8d8d8d8d8d8d8d8d8d8d8d8d8d8d8d8d8d8d8d", diff --git a/skills/review-code-simplicity/references/review-suite/CONTRACT.md b/skills/review-code-simplicity/references/review-suite/CONTRACT.md index bdced9f..7501fa2 100644 --- a/skills/review-code-simplicity/references/review-suite/CONTRACT.md +++ b/skills/review-code-simplicity/references/review-suite/CONTRACT.md @@ -163,6 +163,32 @@ after a correctness fix, or only code simplicity and correctness after a code-simplicity fix) cannot produce a valid `clean` aggregate. This child defines no selective-reuse exception across different heads. +### Consumer/impact evidence + +A `correctness` or `aggregate` result may record `consumer_impact_evidence`: one +entry per changed shared symbol or contract whose other call sites/consumers +were traversed, each naming the `changed_symbol`, its defining `location`, the +`consumer_search_evidence` inspected (one or more `location` + `detail` pairs +describing what was found), and a `disposition` of `all_consumers_consistent`, +`inconsistency_found`, or `no_other_consumers`. + +This makes a reviewer's consumer/impact traversal machine-checkable instead of +an unenforced expectation a reviewer can silently skip. The validator enforces +structure and non-emptiness; it does not determine which changed symbols require +an entry — that judgment belongs to the lens performing the traversal (a later +child consumes this schema to populate it). Given a supplied entry: + +- only `correctness` or `aggregate` results may include this evidence; +- every entry requires at least one concrete `consumer_search_evidence` item, + mirroring the existing "empty impact is valid only with concrete search + evidence" principle used elsewhere in this contract family — a disposition is + never accepted on the strength of an unevidenced claim; and +- `all_consumers_consistent` and `inconsistency_found` describe at least one + other consumer by definition, so each requires search evidence covering more + than the changed symbol's own location (two or more entries); + `no_other_consumers` requires only the one concrete search that found nothing + else. + ## Simplification proposal dispositions When an orchestrator asks correctness to assess a validated simplification diff --git a/skills/review-code-simplicity/references/review-suite/review-result.schema.json b/skills/review-code-simplicity/references/review-suite/review-result.schema.json index 47e9a38..0af0e91 100644 --- a/skills/review-code-simplicity/references/review-suite/review-result.schema.json +++ b/skills/review-code-simplicity/references/review-suite/review-result.schema.json @@ -13,7 +13,7 @@ "blocking_reasons" ], "properties": { - "schema_version": {"const": "1.1"}, + "schema_version": {"const": "1.2"}, "lens": { "enum": [ "correctness", @@ -108,6 +108,43 @@ "type": "array", "items": {"type": "string", "minLength": 1} }, + "consumer_impact_evidence": { + "type": "array", + "items": { + "type": "object", + "additionalProperties": false, + "required": [ + "changed_symbol", + "location", + "consumer_search_evidence", + "disposition" + ], + "properties": { + "changed_symbol": {"type": "string", "minLength": 1}, + "location": {"type": "string", "minLength": 1}, + "consumer_search_evidence": { + "type": "array", + "minItems": 1, + "items": { + "type": "object", + "additionalProperties": false, + "required": ["location", "detail"], + "properties": { + "location": {"type": "string", "minLength": 1}, + "detail": {"type": "string", "minLength": 1} + } + } + }, + "disposition": { + "enum": [ + "all_consumers_consistent", + "inconsistency_found", + "no_other_consumers" + ] + } + } + } + }, "proposal_dispositions": { "type": "array", "items": { diff --git a/skills/review-code-simplicity/references/review-suite/validate.py b/skills/review-code-simplicity/references/review-suite/validate.py index 033b586..8152448 100644 --- a/skills/review-code-simplicity/references/review-suite/validate.py +++ b/skills/review-code-simplicity/references/review-suite/validate.py @@ -35,7 +35,16 @@ def _schema_file(name: str) -> Path: REQUIRED_AGGREGATE_LENSES = ("solution_simplicity", "correctness", "code_simplicity") -STALE_RESULT_SCHEMA_VERSIONS = {"1.0"} +CONSUMER_IMPACT_DISPOSITIONS_IMPLYING_OTHER_CONSUMERS = ( + "all_consumers_consistent", + "inconsistency_found", +) + +# Maps each stale result schema version to the current version it must be +# migrated to. Extend this mapping, never overwrite it, on the next additive +# schema bump so every prior stale version keeps failing with its own useful +# migration error. +STALE_RESULT_SCHEMA_VERSIONS = {"1.0": "1.1", "1.1": "1.2"} BLOCKABLE_PACKET_ERROR_PATTERNS = ( re.compile( @@ -165,14 +174,15 @@ def validate_packet(packet: dict[str, Any]) -> list[str]: def validate_result(result: dict[str, Any]) -> list[str]: - if ( - isinstance(result, dict) - and result.get("schema_version") in STALE_RESULT_SCHEMA_VERSIONS - ): - return [ - "$.schema_version: stale v1.0 result rejected; v1.0 results are not " - "accepted as v1.1 evidence, rebuild review evidence at schema 1.1" - ] + if isinstance(result, dict): + stale_version = result.get("schema_version") + current_version = STALE_RESULT_SCHEMA_VERSIONS.get(stale_version) + if current_version is not None: + return [ + f"$.schema_version: stale v{stale_version} result rejected; " + f"v{stale_version} results are not accepted as v{current_version} " + f"evidence, rebuild review evidence at schema {current_version}" + ] schema = json.loads(SCHEMAS["result"].read_text()) errors = validate_schema(result, schema) if errors: @@ -230,11 +240,53 @@ def validate_result(result: dict[str, Any]) -> list[str]: + ", ".join(duplicate_dispositions) ) + errors.extend(_check_consumer_impact_evidence(result)) + if result.get("lens") == "aggregate" and verdict == "clean": errors.extend(_check_aggregate_clean_lens_executions(result)) return errors +def _check_consumer_impact_evidence(result: dict[str, Any]) -> list[str]: + """Check consumer/impact evidence structure and disposition consistency. + + #52: `consumer_impact_evidence` records a reviewer's traversal to other + call sites/consumers of a changed shared symbol, so that traversal is + machine-checkable instead of an unenforced expectation. The validator does + not determine which changed symbols require an entry — that judgment + belongs to the correctness lens's own traversal pass (a later child). It + only enforces that whatever is supplied is structurally trustworthy: a + disposition that claims other consumers exist must be backed by evidence + covering more than the changed symbol's own location, and every entry + (including `no_other_consumers`) must cite at least one concrete search. + """ + errors: list[str] = [] + entries = result.get("consumer_impact_evidence") + if not isinstance(entries, list): + return errors + if entries and result.get("lens") not in {"correctness", "aggregate"}: + errors.append( + "$.consumer_impact_evidence: only correctness or aggregate results " + "may include consumer/impact evidence" + ) + for index, entry in enumerate(entries): + if not isinstance(entry, dict): + continue + disposition = entry.get("disposition") + evidence = entry.get("consumer_search_evidence") + evidence_count = len(evidence) if isinstance(evidence, list) else 0 + if ( + disposition in CONSUMER_IMPACT_DISPOSITIONS_IMPLYING_OTHER_CONSUMERS + and evidence_count < 2 + ): + errors.append( + f"$.consumer_impact_evidence[{index}]: disposition {disposition!r} " + "claims other consumers were found and requires search evidence " + "covering more than the changed symbol's own location" + ) + return errors + + def _check_aggregate_clean_lens_executions(result: dict[str, Any]) -> list[str]: """Require one fresh current-head/current-base clean execution per lens. diff --git a/skills/review-correctness/evals/expected/standalone-ticket-regression.result.json b/skills/review-correctness/evals/expected/standalone-ticket-regression.result.json index 0929f20..eacaeb0 100644 --- a/skills/review-correctness/evals/expected/standalone-ticket-regression.result.json +++ b/skills/review-correctness/evals/expected/standalone-ticket-regression.result.json @@ -1,5 +1,5 @@ { - "schema_version": "1.1", + "schema_version": "1.2", "lens": "correctness", "candidate": { "head_sha": "8484848484848484848484848484848484848484", diff --git a/skills/review-correctness/references/review-suite/CONTRACT.md b/skills/review-correctness/references/review-suite/CONTRACT.md index bdced9f..7501fa2 100644 --- a/skills/review-correctness/references/review-suite/CONTRACT.md +++ b/skills/review-correctness/references/review-suite/CONTRACT.md @@ -163,6 +163,32 @@ after a correctness fix, or only code simplicity and correctness after a code-simplicity fix) cannot produce a valid `clean` aggregate. This child defines no selective-reuse exception across different heads. +### Consumer/impact evidence + +A `correctness` or `aggregate` result may record `consumer_impact_evidence`: one +entry per changed shared symbol or contract whose other call sites/consumers +were traversed, each naming the `changed_symbol`, its defining `location`, the +`consumer_search_evidence` inspected (one or more `location` + `detail` pairs +describing what was found), and a `disposition` of `all_consumers_consistent`, +`inconsistency_found`, or `no_other_consumers`. + +This makes a reviewer's consumer/impact traversal machine-checkable instead of +an unenforced expectation a reviewer can silently skip. The validator enforces +structure and non-emptiness; it does not determine which changed symbols require +an entry — that judgment belongs to the lens performing the traversal (a later +child consumes this schema to populate it). Given a supplied entry: + +- only `correctness` or `aggregate` results may include this evidence; +- every entry requires at least one concrete `consumer_search_evidence` item, + mirroring the existing "empty impact is valid only with concrete search + evidence" principle used elsewhere in this contract family — a disposition is + never accepted on the strength of an unevidenced claim; and +- `all_consumers_consistent` and `inconsistency_found` describe at least one + other consumer by definition, so each requires search evidence covering more + than the changed symbol's own location (two or more entries); + `no_other_consumers` requires only the one concrete search that found nothing + else. + ## Simplification proposal dispositions When an orchestrator asks correctness to assess a validated simplification diff --git a/skills/review-correctness/references/review-suite/review-result.schema.json b/skills/review-correctness/references/review-suite/review-result.schema.json index 47e9a38..0af0e91 100644 --- a/skills/review-correctness/references/review-suite/review-result.schema.json +++ b/skills/review-correctness/references/review-suite/review-result.schema.json @@ -13,7 +13,7 @@ "blocking_reasons" ], "properties": { - "schema_version": {"const": "1.1"}, + "schema_version": {"const": "1.2"}, "lens": { "enum": [ "correctness", @@ -108,6 +108,43 @@ "type": "array", "items": {"type": "string", "minLength": 1} }, + "consumer_impact_evidence": { + "type": "array", + "items": { + "type": "object", + "additionalProperties": false, + "required": [ + "changed_symbol", + "location", + "consumer_search_evidence", + "disposition" + ], + "properties": { + "changed_symbol": {"type": "string", "minLength": 1}, + "location": {"type": "string", "minLength": 1}, + "consumer_search_evidence": { + "type": "array", + "minItems": 1, + "items": { + "type": "object", + "additionalProperties": false, + "required": ["location", "detail"], + "properties": { + "location": {"type": "string", "minLength": 1}, + "detail": {"type": "string", "minLength": 1} + } + } + }, + "disposition": { + "enum": [ + "all_consumers_consistent", + "inconsistency_found", + "no_other_consumers" + ] + } + } + } + }, "proposal_dispositions": { "type": "array", "items": { diff --git a/skills/review-correctness/references/review-suite/validate.py b/skills/review-correctness/references/review-suite/validate.py index 033b586..8152448 100644 --- a/skills/review-correctness/references/review-suite/validate.py +++ b/skills/review-correctness/references/review-suite/validate.py @@ -35,7 +35,16 @@ def _schema_file(name: str) -> Path: REQUIRED_AGGREGATE_LENSES = ("solution_simplicity", "correctness", "code_simplicity") -STALE_RESULT_SCHEMA_VERSIONS = {"1.0"} +CONSUMER_IMPACT_DISPOSITIONS_IMPLYING_OTHER_CONSUMERS = ( + "all_consumers_consistent", + "inconsistency_found", +) + +# Maps each stale result schema version to the current version it must be +# migrated to. Extend this mapping, never overwrite it, on the next additive +# schema bump so every prior stale version keeps failing with its own useful +# migration error. +STALE_RESULT_SCHEMA_VERSIONS = {"1.0": "1.1", "1.1": "1.2"} BLOCKABLE_PACKET_ERROR_PATTERNS = ( re.compile( @@ -165,14 +174,15 @@ def validate_packet(packet: dict[str, Any]) -> list[str]: def validate_result(result: dict[str, Any]) -> list[str]: - if ( - isinstance(result, dict) - and result.get("schema_version") in STALE_RESULT_SCHEMA_VERSIONS - ): - return [ - "$.schema_version: stale v1.0 result rejected; v1.0 results are not " - "accepted as v1.1 evidence, rebuild review evidence at schema 1.1" - ] + if isinstance(result, dict): + stale_version = result.get("schema_version") + current_version = STALE_RESULT_SCHEMA_VERSIONS.get(stale_version) + if current_version is not None: + return [ + f"$.schema_version: stale v{stale_version} result rejected; " + f"v{stale_version} results are not accepted as v{current_version} " + f"evidence, rebuild review evidence at schema {current_version}" + ] schema = json.loads(SCHEMAS["result"].read_text()) errors = validate_schema(result, schema) if errors: @@ -230,11 +240,53 @@ def validate_result(result: dict[str, Any]) -> list[str]: + ", ".join(duplicate_dispositions) ) + errors.extend(_check_consumer_impact_evidence(result)) + if result.get("lens") == "aggregate" and verdict == "clean": errors.extend(_check_aggregate_clean_lens_executions(result)) return errors +def _check_consumer_impact_evidence(result: dict[str, Any]) -> list[str]: + """Check consumer/impact evidence structure and disposition consistency. + + #52: `consumer_impact_evidence` records a reviewer's traversal to other + call sites/consumers of a changed shared symbol, so that traversal is + machine-checkable instead of an unenforced expectation. The validator does + not determine which changed symbols require an entry — that judgment + belongs to the correctness lens's own traversal pass (a later child). It + only enforces that whatever is supplied is structurally trustworthy: a + disposition that claims other consumers exist must be backed by evidence + covering more than the changed symbol's own location, and every entry + (including `no_other_consumers`) must cite at least one concrete search. + """ + errors: list[str] = [] + entries = result.get("consumer_impact_evidence") + if not isinstance(entries, list): + return errors + if entries and result.get("lens") not in {"correctness", "aggregate"}: + errors.append( + "$.consumer_impact_evidence: only correctness or aggregate results " + "may include consumer/impact evidence" + ) + for index, entry in enumerate(entries): + if not isinstance(entry, dict): + continue + disposition = entry.get("disposition") + evidence = entry.get("consumer_search_evidence") + evidence_count = len(evidence) if isinstance(evidence, list) else 0 + if ( + disposition in CONSUMER_IMPACT_DISPOSITIONS_IMPLYING_OTHER_CONSUMERS + and evidence_count < 2 + ): + errors.append( + f"$.consumer_impact_evidence[{index}]: disposition {disposition!r} " + "claims other consumers were found and requires search evidence " + "covering more than the changed symbol's own location" + ) + return errors + + def _check_aggregate_clean_lens_executions(result: dict[str, Any]) -> list[str]: """Require one fresh current-head/current-base clean execution per lens. diff --git a/skills/review-solution-simplicity/evals/expected/standalone-provider-framework.result.json b/skills/review-solution-simplicity/evals/expected/standalone-provider-framework.result.json index 7f267f6..4e84878 100644 --- a/skills/review-solution-simplicity/evals/expected/standalone-provider-framework.result.json +++ b/skills/review-solution-simplicity/evals/expected/standalone-provider-framework.result.json @@ -1,5 +1,5 @@ { - "schema_version": "1.1", + "schema_version": "1.2", "lens": "solution_simplicity", "candidate": { "head_sha": "8787878787878787878787878787878787878787", diff --git a/skills/review-solution-simplicity/evals/expected/untrusted-packet-instruction.result.json b/skills/review-solution-simplicity/evals/expected/untrusted-packet-instruction.result.json index 05394cf..79cdc0f 100644 --- a/skills/review-solution-simplicity/evals/expected/untrusted-packet-instruction.result.json +++ b/skills/review-solution-simplicity/evals/expected/untrusted-packet-instruction.result.json @@ -1,5 +1,5 @@ { - "schema_version": "1.1", + "schema_version": "1.2", "lens": "solution_simplicity", "candidate": { "head_sha": "8686868686868686868686868686868686868686", diff --git a/skills/review-solution-simplicity/evals/expected/verified-native-relationship.result.json b/skills/review-solution-simplicity/evals/expected/verified-native-relationship.result.json index 8b3a7b5..0963378 100644 --- a/skills/review-solution-simplicity/evals/expected/verified-native-relationship.result.json +++ b/skills/review-solution-simplicity/evals/expected/verified-native-relationship.result.json @@ -1,5 +1,5 @@ { - "schema_version": "1.1", + "schema_version": "1.2", "lens": "solution_simplicity", "candidate": { "head_sha": "8787878787878787878787878787878787878787", diff --git a/skills/review-solution-simplicity/references/review-suite/CONTRACT.md b/skills/review-solution-simplicity/references/review-suite/CONTRACT.md index bdced9f..7501fa2 100644 --- a/skills/review-solution-simplicity/references/review-suite/CONTRACT.md +++ b/skills/review-solution-simplicity/references/review-suite/CONTRACT.md @@ -163,6 +163,32 @@ after a correctness fix, or only code simplicity and correctness after a code-simplicity fix) cannot produce a valid `clean` aggregate. This child defines no selective-reuse exception across different heads. +### Consumer/impact evidence + +A `correctness` or `aggregate` result may record `consumer_impact_evidence`: one +entry per changed shared symbol or contract whose other call sites/consumers +were traversed, each naming the `changed_symbol`, its defining `location`, the +`consumer_search_evidence` inspected (one or more `location` + `detail` pairs +describing what was found), and a `disposition` of `all_consumers_consistent`, +`inconsistency_found`, or `no_other_consumers`. + +This makes a reviewer's consumer/impact traversal machine-checkable instead of +an unenforced expectation a reviewer can silently skip. The validator enforces +structure and non-emptiness; it does not determine which changed symbols require +an entry — that judgment belongs to the lens performing the traversal (a later +child consumes this schema to populate it). Given a supplied entry: + +- only `correctness` or `aggregate` results may include this evidence; +- every entry requires at least one concrete `consumer_search_evidence` item, + mirroring the existing "empty impact is valid only with concrete search + evidence" principle used elsewhere in this contract family — a disposition is + never accepted on the strength of an unevidenced claim; and +- `all_consumers_consistent` and `inconsistency_found` describe at least one + other consumer by definition, so each requires search evidence covering more + than the changed symbol's own location (two or more entries); + `no_other_consumers` requires only the one concrete search that found nothing + else. + ## Simplification proposal dispositions When an orchestrator asks correctness to assess a validated simplification diff --git a/skills/review-solution-simplicity/references/review-suite/review-result.schema.json b/skills/review-solution-simplicity/references/review-suite/review-result.schema.json index 47e9a38..0af0e91 100644 --- a/skills/review-solution-simplicity/references/review-suite/review-result.schema.json +++ b/skills/review-solution-simplicity/references/review-suite/review-result.schema.json @@ -13,7 +13,7 @@ "blocking_reasons" ], "properties": { - "schema_version": {"const": "1.1"}, + "schema_version": {"const": "1.2"}, "lens": { "enum": [ "correctness", @@ -108,6 +108,43 @@ "type": "array", "items": {"type": "string", "minLength": 1} }, + "consumer_impact_evidence": { + "type": "array", + "items": { + "type": "object", + "additionalProperties": false, + "required": [ + "changed_symbol", + "location", + "consumer_search_evidence", + "disposition" + ], + "properties": { + "changed_symbol": {"type": "string", "minLength": 1}, + "location": {"type": "string", "minLength": 1}, + "consumer_search_evidence": { + "type": "array", + "minItems": 1, + "items": { + "type": "object", + "additionalProperties": false, + "required": ["location", "detail"], + "properties": { + "location": {"type": "string", "minLength": 1}, + "detail": {"type": "string", "minLength": 1} + } + } + }, + "disposition": { + "enum": [ + "all_consumers_consistent", + "inconsistency_found", + "no_other_consumers" + ] + } + } + } + }, "proposal_dispositions": { "type": "array", "items": { diff --git a/skills/review-solution-simplicity/references/review-suite/validate.py b/skills/review-solution-simplicity/references/review-suite/validate.py index 033b586..8152448 100644 --- a/skills/review-solution-simplicity/references/review-suite/validate.py +++ b/skills/review-solution-simplicity/references/review-suite/validate.py @@ -35,7 +35,16 @@ def _schema_file(name: str) -> Path: REQUIRED_AGGREGATE_LENSES = ("solution_simplicity", "correctness", "code_simplicity") -STALE_RESULT_SCHEMA_VERSIONS = {"1.0"} +CONSUMER_IMPACT_DISPOSITIONS_IMPLYING_OTHER_CONSUMERS = ( + "all_consumers_consistent", + "inconsistency_found", +) + +# Maps each stale result schema version to the current version it must be +# migrated to. Extend this mapping, never overwrite it, on the next additive +# schema bump so every prior stale version keeps failing with its own useful +# migration error. +STALE_RESULT_SCHEMA_VERSIONS = {"1.0": "1.1", "1.1": "1.2"} BLOCKABLE_PACKET_ERROR_PATTERNS = ( re.compile( @@ -165,14 +174,15 @@ def validate_packet(packet: dict[str, Any]) -> list[str]: def validate_result(result: dict[str, Any]) -> list[str]: - if ( - isinstance(result, dict) - and result.get("schema_version") in STALE_RESULT_SCHEMA_VERSIONS - ): - return [ - "$.schema_version: stale v1.0 result rejected; v1.0 results are not " - "accepted as v1.1 evidence, rebuild review evidence at schema 1.1" - ] + if isinstance(result, dict): + stale_version = result.get("schema_version") + current_version = STALE_RESULT_SCHEMA_VERSIONS.get(stale_version) + if current_version is not None: + return [ + f"$.schema_version: stale v{stale_version} result rejected; " + f"v{stale_version} results are not accepted as v{current_version} " + f"evidence, rebuild review evidence at schema {current_version}" + ] schema = json.loads(SCHEMAS["result"].read_text()) errors = validate_schema(result, schema) if errors: @@ -230,11 +240,53 @@ def validate_result(result: dict[str, Any]) -> list[str]: + ", ".join(duplicate_dispositions) ) + errors.extend(_check_consumer_impact_evidence(result)) + if result.get("lens") == "aggregate" and verdict == "clean": errors.extend(_check_aggregate_clean_lens_executions(result)) return errors +def _check_consumer_impact_evidence(result: dict[str, Any]) -> list[str]: + """Check consumer/impact evidence structure and disposition consistency. + + #52: `consumer_impact_evidence` records a reviewer's traversal to other + call sites/consumers of a changed shared symbol, so that traversal is + machine-checkable instead of an unenforced expectation. The validator does + not determine which changed symbols require an entry — that judgment + belongs to the correctness lens's own traversal pass (a later child). It + only enforces that whatever is supplied is structurally trustworthy: a + disposition that claims other consumers exist must be backed by evidence + covering more than the changed symbol's own location, and every entry + (including `no_other_consumers`) must cite at least one concrete search. + """ + errors: list[str] = [] + entries = result.get("consumer_impact_evidence") + if not isinstance(entries, list): + return errors + if entries and result.get("lens") not in {"correctness", "aggregate"}: + errors.append( + "$.consumer_impact_evidence: only correctness or aggregate results " + "may include consumer/impact evidence" + ) + for index, entry in enumerate(entries): + if not isinstance(entry, dict): + continue + disposition = entry.get("disposition") + evidence = entry.get("consumer_search_evidence") + evidence_count = len(evidence) if isinstance(evidence, list) else 0 + if ( + disposition in CONSUMER_IMPACT_DISPOSITIONS_IMPLYING_OTHER_CONSUMERS + and evidence_count < 2 + ): + errors.append( + f"$.consumer_impact_evidence[{index}]: disposition {disposition!r} " + "claims other consumers were found and requires search evidence " + "covering more than the changed symbol's own location" + ) + return errors + + def _check_aggregate_clean_lens_executions(result: dict[str, Any]) -> list[str]: """Require one fresh current-head/current-base clean execution per lens. From 2432156e0f8008b73b91a21caadd21387102d530 Mon Sep 17 00:00:00 2001 From: Scott Haug Date: Tue, 28 Jul 2026 20:14:46 -0700 Subject: [PATCH 2/2] docs: make the consumer-impact-evidence validator boundary explicit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary - Strengthen `review-suite/CONTRACT.md`, `validate.py`'s `_check_consumer_impact_evidence` docstring, and its regression test to explain, with concrete reasoning, why the validator never inspects the packet diff to decide whether a changed symbol needed a `consumer_impact_evidence` entry: this validator receives only a packet and a result with no repository checkout to search, so it cannot itself determine whether a changed symbol has other call sites, and adding that determination here would be the independent correctness explorer and static call-graph tooling #52's non-goals rule out. ## Why - A fresh `review-code-change` pass on the prior candidate raised a blocking correctness finding proposing that `validate_pair` be extended to detect, from the diff, whether a changed symbol's sibling call site was also covered. That proposed mechanism is exactly the independent correctness-explorer / static-call-graph tooling #52's own non-goals forbid, and #52's own text assigns "which changed symbols require an entry" to "the correctness lens's own traversal pass (#53)" — a mechanism that does not exist yet in this candidate — not to this validator. The real baseline miss this evidence exists to surface (`dependency-strictness-propagation`) involved a sibling call site the diff never touched, which only live repository access (available to the reviewing agent, never to this validator) can find; a diff-only heuristic would not even correctly model that failure mode. Declining to build that mechanism is therefore correct, but the prior candidate's reasoning for the boundary was implicit; this revision makes it explicit and self-evident from the artifacts themselves. ## Verification - `just format`, `just lint`, and `just test` all pass (234 tests in `review-suite/scripts/tests`, unchanged in count from the prior commit — this revision only strengthens documentation and comments, no schema, validator behavior, or fixture changed). - `just sync-contracts` leaves all four bundled `review-suite` copies byte-identical to the canonical source. Co-Authored-By: Claude Sonnet 5 --- review-suite/CONTRACT.md | 17 +++++++++++ review-suite/scripts/tests/test_contracts.py | 30 ++++++++++++++----- review-suite/scripts/validate.py | 12 ++++++++ .../references/review-suite/CONTRACT.md | 17 +++++++++++ .../references/review-suite/validate.py | 12 ++++++++ .../references/review-suite/CONTRACT.md | 17 +++++++++++ .../references/review-suite/validate.py | 12 ++++++++ .../references/review-suite/CONTRACT.md | 17 +++++++++++ .../references/review-suite/validate.py | 12 ++++++++ .../references/review-suite/CONTRACT.md | 17 +++++++++++ .../references/review-suite/validate.py | 12 ++++++++ 11 files changed, 168 insertions(+), 7 deletions(-) diff --git a/review-suite/CONTRACT.md b/review-suite/CONTRACT.md index 7501fa2..9c2a2a6 100644 --- a/review-suite/CONTRACT.md +++ b/review-suite/CONTRACT.md @@ -189,6 +189,23 @@ child consumes this schema to populate it). Given a supplied entry: `no_other_consumers` requires only the one concrete search that found nothing else. +This validator deliberately cannot decide *whether* a given changed symbol +needed an entry at all, and an aggregate `clean` result that omits +`consumer_impact_evidence` entirely is schema-valid. That is not an oversight: +`scripts/validate.py` takes only a packet and a result as input and has no +repository checkout to search, so it cannot itself determine whether a changed +symbol has other call sites — the real baseline miss this evidence exists to +surface involved a sibling call site the diff never touched, which only live +repository access (available to the reviewing agent, not to this validator) can +find. Building that determination into the validator would be exactly the +independent correctness explorer and static call-graph tooling this contract +family's non-goals rule out. Completeness of a given traversal — did the lens +find every consumer that mattered — is judged by the lens performing the +traversal and by forward-testing its output against a fixture's expected result, +the same way this contract family already judges any other lens-specific finding +(a duplicated-policy or behavior-bug miss is likewise never something this +schema-and-structure validator can detect on its own). + ## Simplification proposal dispositions When an orchestrator asks correctness to assess a validated simplification diff --git a/review-suite/scripts/tests/test_contracts.py b/review-suite/scripts/tests/test_contracts.py index 61b6e84..fa53b96 100644 --- a/review-suite/scripts/tests/test_contracts.py +++ b/review-suite/scripts/tests/test_contracts.py @@ -490,13 +490,29 @@ def test_no_other_consumers_without_search_evidence_is_rejected(self): self.assertTrue(errors) self.assertTrue(any("expected at least 1 item" in error for error in errors)) - def test_omitted_evidence_array_does_not_itself_fail_schema_validation(self): - # The validator cannot determine, from the result alone, which - # changed symbols require an entry; that judgment belongs to the lens - # performing the traversal (a later child). An aggregate clean result - # that omits `consumer_impact_evidence` entirely remains - # schema-valid; completeness of the traversal is a lens-judgment and - # forward-testing concern, not a mechanical validator check. + def test_an_omitted_evidence_array_is_a_lens_judgment_gap_not_a_schema_gap( + self, + ): + # This is a deliberate boundary, not an oversight: #52's own text + # ("the validator enforces structure and non-emptiness; lens judgment + # determines which changed symbols require an entry") and non-goals + # ("Add independent correctness explorers or finding validators"; + # "Implement a complete static call graph ... or mandatory AST + # tooling") both rule out having this validator inspect the packet's + # diff to decide whether a changed symbol needed an entry. This + # validator receives only a packet and a result — no repository + # checkout to search — so it structurally cannot make that + # determination; the real baseline miss this evidence exists to + # surface involved a sibling call site the diff never touched, which + # only live repository access (available to the reviewing agent, not + # to this validator) can find. An aggregate `clean` result that omits + # `consumer_impact_evidence` entirely therefore remains schema-valid; + # whether a given traversal was actually complete is judged by + # forward-testing the populating lens's real output against a + # fixture's expected result, exactly as this contract family already + # judges every other lens-specific finding (a duplicated-policy or + # behavior-bug miss is likewise never something this validator can + # detect unaided). del self.result["consumer_impact_evidence"] self.assertEqual([], VALIDATOR.validate_result(self.result)) diff --git a/review-suite/scripts/validate.py b/review-suite/scripts/validate.py index 8152448..6b054e1 100644 --- a/review-suite/scripts/validate.py +++ b/review-suite/scripts/validate.py @@ -259,6 +259,18 @@ def _check_consumer_impact_evidence(result: dict[str, Any]) -> list[str]: disposition that claims other consumers exist must be backed by evidence covering more than the changed symbol's own location, and every entry (including `no_other_consumers`) must cite at least one concrete search. + + This function deliberately never inspects `packet["candidate"]["diff"]` to + decide whether a changed symbol *should* have an entry: this validator + receives only a packet and a result, with no repository checkout to + search, so it cannot itself determine whether a changed symbol has other + call sites (the baseline miss this evidence exists to surface involved a + sibling call site the diff never touched, which only live repository + access can find). Adding that determination here would be exactly the + independent correctness explorer and static call-graph tooling #52's + non-goals rule out; an omitted `consumer_impact_evidence` array is + schema-valid by design, and its completeness is judged by forward-testing + the populating lens's actual output, not by this structural check. """ errors: list[str] = [] entries = result.get("consumer_impact_evidence") diff --git a/skills/review-code-change/references/review-suite/CONTRACT.md b/skills/review-code-change/references/review-suite/CONTRACT.md index 7501fa2..9c2a2a6 100644 --- a/skills/review-code-change/references/review-suite/CONTRACT.md +++ b/skills/review-code-change/references/review-suite/CONTRACT.md @@ -189,6 +189,23 @@ child consumes this schema to populate it). Given a supplied entry: `no_other_consumers` requires only the one concrete search that found nothing else. +This validator deliberately cannot decide *whether* a given changed symbol +needed an entry at all, and an aggregate `clean` result that omits +`consumer_impact_evidence` entirely is schema-valid. That is not an oversight: +`scripts/validate.py` takes only a packet and a result as input and has no +repository checkout to search, so it cannot itself determine whether a changed +symbol has other call sites — the real baseline miss this evidence exists to +surface involved a sibling call site the diff never touched, which only live +repository access (available to the reviewing agent, not to this validator) can +find. Building that determination into the validator would be exactly the +independent correctness explorer and static call-graph tooling this contract +family's non-goals rule out. Completeness of a given traversal — did the lens +find every consumer that mattered — is judged by the lens performing the +traversal and by forward-testing its output against a fixture's expected result, +the same way this contract family already judges any other lens-specific finding +(a duplicated-policy or behavior-bug miss is likewise never something this +schema-and-structure validator can detect on its own). + ## Simplification proposal dispositions When an orchestrator asks correctness to assess a validated simplification diff --git a/skills/review-code-change/references/review-suite/validate.py b/skills/review-code-change/references/review-suite/validate.py index 8152448..6b054e1 100644 --- a/skills/review-code-change/references/review-suite/validate.py +++ b/skills/review-code-change/references/review-suite/validate.py @@ -259,6 +259,18 @@ def _check_consumer_impact_evidence(result: dict[str, Any]) -> list[str]: disposition that claims other consumers exist must be backed by evidence covering more than the changed symbol's own location, and every entry (including `no_other_consumers`) must cite at least one concrete search. + + This function deliberately never inspects `packet["candidate"]["diff"]` to + decide whether a changed symbol *should* have an entry: this validator + receives only a packet and a result, with no repository checkout to + search, so it cannot itself determine whether a changed symbol has other + call sites (the baseline miss this evidence exists to surface involved a + sibling call site the diff never touched, which only live repository + access can find). Adding that determination here would be exactly the + independent correctness explorer and static call-graph tooling #52's + non-goals rule out; an omitted `consumer_impact_evidence` array is + schema-valid by design, and its completeness is judged by forward-testing + the populating lens's actual output, not by this structural check. """ errors: list[str] = [] entries = result.get("consumer_impact_evidence") diff --git a/skills/review-code-simplicity/references/review-suite/CONTRACT.md b/skills/review-code-simplicity/references/review-suite/CONTRACT.md index 7501fa2..9c2a2a6 100644 --- a/skills/review-code-simplicity/references/review-suite/CONTRACT.md +++ b/skills/review-code-simplicity/references/review-suite/CONTRACT.md @@ -189,6 +189,23 @@ child consumes this schema to populate it). Given a supplied entry: `no_other_consumers` requires only the one concrete search that found nothing else. +This validator deliberately cannot decide *whether* a given changed symbol +needed an entry at all, and an aggregate `clean` result that omits +`consumer_impact_evidence` entirely is schema-valid. That is not an oversight: +`scripts/validate.py` takes only a packet and a result as input and has no +repository checkout to search, so it cannot itself determine whether a changed +symbol has other call sites — the real baseline miss this evidence exists to +surface involved a sibling call site the diff never touched, which only live +repository access (available to the reviewing agent, not to this validator) can +find. Building that determination into the validator would be exactly the +independent correctness explorer and static call-graph tooling this contract +family's non-goals rule out. Completeness of a given traversal — did the lens +find every consumer that mattered — is judged by the lens performing the +traversal and by forward-testing its output against a fixture's expected result, +the same way this contract family already judges any other lens-specific finding +(a duplicated-policy or behavior-bug miss is likewise never something this +schema-and-structure validator can detect on its own). + ## Simplification proposal dispositions When an orchestrator asks correctness to assess a validated simplification diff --git a/skills/review-code-simplicity/references/review-suite/validate.py b/skills/review-code-simplicity/references/review-suite/validate.py index 8152448..6b054e1 100644 --- a/skills/review-code-simplicity/references/review-suite/validate.py +++ b/skills/review-code-simplicity/references/review-suite/validate.py @@ -259,6 +259,18 @@ def _check_consumer_impact_evidence(result: dict[str, Any]) -> list[str]: disposition that claims other consumers exist must be backed by evidence covering more than the changed symbol's own location, and every entry (including `no_other_consumers`) must cite at least one concrete search. + + This function deliberately never inspects `packet["candidate"]["diff"]` to + decide whether a changed symbol *should* have an entry: this validator + receives only a packet and a result, with no repository checkout to + search, so it cannot itself determine whether a changed symbol has other + call sites (the baseline miss this evidence exists to surface involved a + sibling call site the diff never touched, which only live repository + access can find). Adding that determination here would be exactly the + independent correctness explorer and static call-graph tooling #52's + non-goals rule out; an omitted `consumer_impact_evidence` array is + schema-valid by design, and its completeness is judged by forward-testing + the populating lens's actual output, not by this structural check. """ errors: list[str] = [] entries = result.get("consumer_impact_evidence") diff --git a/skills/review-correctness/references/review-suite/CONTRACT.md b/skills/review-correctness/references/review-suite/CONTRACT.md index 7501fa2..9c2a2a6 100644 --- a/skills/review-correctness/references/review-suite/CONTRACT.md +++ b/skills/review-correctness/references/review-suite/CONTRACT.md @@ -189,6 +189,23 @@ child consumes this schema to populate it). Given a supplied entry: `no_other_consumers` requires only the one concrete search that found nothing else. +This validator deliberately cannot decide *whether* a given changed symbol +needed an entry at all, and an aggregate `clean` result that omits +`consumer_impact_evidence` entirely is schema-valid. That is not an oversight: +`scripts/validate.py` takes only a packet and a result as input and has no +repository checkout to search, so it cannot itself determine whether a changed +symbol has other call sites — the real baseline miss this evidence exists to +surface involved a sibling call site the diff never touched, which only live +repository access (available to the reviewing agent, not to this validator) can +find. Building that determination into the validator would be exactly the +independent correctness explorer and static call-graph tooling this contract +family's non-goals rule out. Completeness of a given traversal — did the lens +find every consumer that mattered — is judged by the lens performing the +traversal and by forward-testing its output against a fixture's expected result, +the same way this contract family already judges any other lens-specific finding +(a duplicated-policy or behavior-bug miss is likewise never something this +schema-and-structure validator can detect on its own). + ## Simplification proposal dispositions When an orchestrator asks correctness to assess a validated simplification diff --git a/skills/review-correctness/references/review-suite/validate.py b/skills/review-correctness/references/review-suite/validate.py index 8152448..6b054e1 100644 --- a/skills/review-correctness/references/review-suite/validate.py +++ b/skills/review-correctness/references/review-suite/validate.py @@ -259,6 +259,18 @@ def _check_consumer_impact_evidence(result: dict[str, Any]) -> list[str]: disposition that claims other consumers exist must be backed by evidence covering more than the changed symbol's own location, and every entry (including `no_other_consumers`) must cite at least one concrete search. + + This function deliberately never inspects `packet["candidate"]["diff"]` to + decide whether a changed symbol *should* have an entry: this validator + receives only a packet and a result, with no repository checkout to + search, so it cannot itself determine whether a changed symbol has other + call sites (the baseline miss this evidence exists to surface involved a + sibling call site the diff never touched, which only live repository + access can find). Adding that determination here would be exactly the + independent correctness explorer and static call-graph tooling #52's + non-goals rule out; an omitted `consumer_impact_evidence` array is + schema-valid by design, and its completeness is judged by forward-testing + the populating lens's actual output, not by this structural check. """ errors: list[str] = [] entries = result.get("consumer_impact_evidence") diff --git a/skills/review-solution-simplicity/references/review-suite/CONTRACT.md b/skills/review-solution-simplicity/references/review-suite/CONTRACT.md index 7501fa2..9c2a2a6 100644 --- a/skills/review-solution-simplicity/references/review-suite/CONTRACT.md +++ b/skills/review-solution-simplicity/references/review-suite/CONTRACT.md @@ -189,6 +189,23 @@ child consumes this schema to populate it). Given a supplied entry: `no_other_consumers` requires only the one concrete search that found nothing else. +This validator deliberately cannot decide *whether* a given changed symbol +needed an entry at all, and an aggregate `clean` result that omits +`consumer_impact_evidence` entirely is schema-valid. That is not an oversight: +`scripts/validate.py` takes only a packet and a result as input and has no +repository checkout to search, so it cannot itself determine whether a changed +symbol has other call sites — the real baseline miss this evidence exists to +surface involved a sibling call site the diff never touched, which only live +repository access (available to the reviewing agent, not to this validator) can +find. Building that determination into the validator would be exactly the +independent correctness explorer and static call-graph tooling this contract +family's non-goals rule out. Completeness of a given traversal — did the lens +find every consumer that mattered — is judged by the lens performing the +traversal and by forward-testing its output against a fixture's expected result, +the same way this contract family already judges any other lens-specific finding +(a duplicated-policy or behavior-bug miss is likewise never something this +schema-and-structure validator can detect on its own). + ## Simplification proposal dispositions When an orchestrator asks correctness to assess a validated simplification diff --git a/skills/review-solution-simplicity/references/review-suite/validate.py b/skills/review-solution-simplicity/references/review-suite/validate.py index 8152448..6b054e1 100644 --- a/skills/review-solution-simplicity/references/review-suite/validate.py +++ b/skills/review-solution-simplicity/references/review-suite/validate.py @@ -259,6 +259,18 @@ def _check_consumer_impact_evidence(result: dict[str, Any]) -> list[str]: disposition that claims other consumers exist must be backed by evidence covering more than the changed symbol's own location, and every entry (including `no_other_consumers`) must cite at least one concrete search. + + This function deliberately never inspects `packet["candidate"]["diff"]` to + decide whether a changed symbol *should* have an entry: this validator + receives only a packet and a result, with no repository checkout to + search, so it cannot itself determine whether a changed symbol has other + call sites (the baseline miss this evidence exists to surface involved a + sibling call site the diff never touched, which only live repository + access can find). Adding that determination here would be exactly the + independent correctness explorer and static call-graph tooling #52's + non-goals rule out; an omitted `consumer_impact_evidence` array is + schema-valid by design, and its completeness is judged by forward-testing + the populating lens's actual output, not by this structural check. """ errors: list[str] = [] entries = result.get("consumer_impact_evidence")