Add cross-Skill structured review result contract tests (#71)

[amirbena]

Fixes #71

What

• Behavior / contracts: Adds test-only cross-Skill contract checks (tests/reference/review/structured_output_contract.py) and paired sample outputs from both Skills (tests/unit/review/findings/structured_output_samples/). They check:

  • schema validity, failing closed on missing, malformed, unsupported-major, or newer-than-published versions;
  • which fields each Skill populates, including that reviewed_head_sha equals the known workspace or PR head;
  • shared-field parity, where only skill and reviewed_state may differ;
  • that the human report and the result agree on decision, counts, coverage, head, finding set, ids, and affected locations, without re-deriving the decision.

  Corrects review-result-model.md §7, which listed Add schema contract tests #71 as the owner of consumers, and adds §8 describing these checks. Adds a benchmark adapter regression test: the benchmark prompt leaves structured_review_result off, and an appended result block does not change parsed findings or verdict labels.

• Governance / policy: None

• Packaging / portability: None. The new files are test-only, and the new reference module is registered as test-only.

• Changelog: generated at release from the two lines below — never edit CHANGELOG.md (see docs/RELEASE.md)

• Release category: none

• Release entry:

Benchmark assessment (against landed #69/#70)

Property	Classification	Evidence
Schema validity, schema_version, required fields, fail-closed	Deterministic test (done here)	FailClosedTests. The benchmark never reads JSON.
reviewed_state SHAs	Deterministic test (done here)	SurfacePopulationTests, plus the head check in report ↔ result agreement
identity.stable_id format and derivation	Deterministic test (done here)	Sample IDs are recomputed from the reference model.
stable_id actually computed, not invented	Benchmark coverage gap → #529	Invented IDs pass every output check (test_fabricated_identity_is_invisible_to_output_checks). Only repeated live runs can show this.
Option does not change the review	Benchmark coverage gap → #529	Required by the policy. Checking it needs one live run with the option on and one with it off.
Cross-Skill shared-field semantics	Deterministic test (done here)	CrossSkillParityTests, including the decision labels in model §4 and both policies
Human report ↔ structured result	Deterministic test (done here)	ReportAgreementTests. Applying it to live output is part of #529.
Root-cause consolidation, affected_locations	Already covered (count); agreement tested here	The consolidation corpus covers the count. The rendered ↔ result list is checked here.
Rendered decision in the benchmark	Out of scope	The Markdown scan stays until #529 decides on live option-on runs.
Parser robustness to an appended result block	Already covered; now pinned by a test	The adapter regexes are anchored; see StructuredReviewResultBoundaryTests.
benchmark-result/v1, lanes, corpus, sentinel	Out of scope	None of them references the structured result. The sentinel cases are unchanged.
Local vs PR results for equivalent scope	Out of scope	A pre-existing design choice (the benchmark invokes only local-code-review)

Validation

• Relevant validation was run, or the reason it could not be run is stated.
• New and touched modules pass locally (contract, review-result docs, production adapter, packaging disclaimer). The full suite runs in CI.

Review

• local-code-review of 677f3cf: REVIEW CLEAN (P0: 0, P1: 0, P2: 2). Both P2 findings are fixed in b45b27e:
  • The contract check rejected a valid github-pr-review output whose Reviewer Brief follows the JSON block. It now requires only that the block follows the report's Decision.
  • Model §8 cited schema-versioning.md §3 for rejecting a newer-than-published version. §3 is consumer guidance; the rejection is a producer rule from §4, and it now cites §4.

🤖 Generated with Claude Code