diff --git a/docs/review-result/README.md b/docs/review-result/README.md
index 1680064..bb3d13b 100644
--- a/docs/review-result/README.md
+++ b/docs/review-result/README.md
@@ -30,6 +30,10 @@ archive, and no packaged Skill resource depends on them.
[`../../shared/templates/finding.md`](../../shared/templates/finding.md),
[`../../shared/policies/severity.md`](../../shared/policies/severity.md),
[`../findings/README.md`](../findings/README.md).
+- Cross-Skill contract tests and sample outputs
+ ([#71](https://github.com/amirbena/code-review-skill/issues/71)):
+ [`../../tests/reference/review/structured_output_contract.py`](../../tests/reference/review/structured_output_contract.py),
+ described in [`review-result-model.md`](review-result-model.md) section 8.
- Test-only version-rule reference:
[`../../tests/reference/review/review_result_version.py`](../../tests/reference/review/review_result_version.py).
- The architecture map: [`../ARCHITECTURE.md`](../ARCHITECTURE.md).
diff --git a/docs/review-result/review-result-model.md b/docs/review-result/review-result-model.md
index 1546213..c42d7ad 100644
--- a/docs/review-result/review-result-model.md
+++ b/docs/review-result/review-result-model.md
@@ -121,7 +121,8 @@ schema failing.
| Versioning policy and compatibility rules for `schema_version` | [`schema-versioning.md`](schema-versioning.md) ([#68](https://github.com/amirbena/code-review-skill/issues/68)) |
| Local Skill emission (opt-in `structured_review_result`; packaged restatement in [`structured-output.md`](../../shared/policies/structured-output.md), pinned to this schema by a drift test) | [#69](https://github.com/amirbena/code-review-skill/issues/69) |
| GitHub Skill emission | [#70](https://github.com/amirbena/code-review-skill/issues/70) |
-| Consumers of the result | [#71](https://github.com/amirbena/code-review-skill/issues/71) |
+| Cross-Skill contract tests (section 8) | [#71](https://github.com/amirbena/code-review-skill/issues/71) |
+| Consumers of the result | None in this repository; consumer handling is the guidance in [`schema-versioning.md`](schema-versioning.md) section 3 |
| Parent capability | [#44](https://github.com/amirbena/code-review-skill/issues/44) |
`github-pr-review` emits the result on explicit request, returned to the
@@ -135,3 +136,21 @@ and identity minting owned by the shared
[`finding.md`](../../shared/templates/finding.md)'s note that a
machine-readable renderer would be "another projection of the same fields"
is what this schema is the first instance of.
+
+## 8. Cross-Skill contract tests
+
+[#71](https://github.com/amirbena/code-review-skill/issues/71) checks
+sample outputs of both Skills against this record. The samples are
+[`../../tests/unit/review/findings/structured_output_samples/`](../../tests/unit/review/findings/structured_output_samples/);
+the checks are
+[`../../tests/reference/review/structured_output_contract.py`](../../tests/reference/review/structured_output_contract.py).
+
+| Check | Rule |
+| --- | --- |
+| Schema and version | Each result passes the section 5 validator. A missing or malformed `schema_version`, or a different `MAJOR`, fails closed ([`schema-versioning.md`](schema-versioning.md) section 3). A version newer than the published schema is also rejected. This is a producer rule: producers emit the version they were written for (section 4). A consumer would accept a newer `MINOR`. |
+| Surface population | `skill` names the producer. `reviewed_state.reviewed_head_sha` equals the known workspace head, or the PR head. It is `null` only for an uncommitted local target or an incomplete PR review. |
+| Shared-field parity | `skill` and `reviewed_state` are the only surface-specific fields. For the same review, every other field is identical across the two Skills. |
+| Report ↔ result agreement | The rendered decision label maps to `decision.outcome` through the section 4 table. The rendered counts, coverage, reviewed head, finding set (severity and title), finding ids, and affected locations equal the result's. The comparator reads both surfaces and never re-derives the decision. |
+
+The benchmark does not read the result. It invokes `local-code-review` with
+the option off and scores only the Markdown report.
diff --git a/tests/README.md b/tests/README.md
index fc49537..917093b 100644
--- a/tests/README.md
+++ b/tests/README.md
@@ -24,7 +24,7 @@ none of the four top-level kinds (`unit`, `policy`, `reference`,
| `unit/benchmark/` | Unit coverage for the `runtime_platform/benchmark/reference/` models: `test_benchmark_corpus.py` validates the `docs/benchmark/corpus/` fixtures (#51) through the `benchmark_fixture.py` reference validator; `test_benchmark_runner.py` drives the reference runner (#52) over the corpus and asserts per-case isolation, cleanup, and byte-for-byte preservation of a deliberately dirty source repository; `test_benchmark_report.py` drives the reference report (#53) and asserts a seeded regression is classified distinctly from an improvement and that output is byte-identical for identical inputs; `test_benchmark_match.py` drives the reference matcher (#54) and asserts every `match-criteria.md` §8 worked example classifies as documented; `test_benchmark_metrics.py` drives the reference metric (#55) and asserts every `missed-and-incorrect-findings.md` §8 worked example counts as documented; `test_benchmark_severity.py` drives the reference metric (#56) and asserts every `severity-accuracy.md` §7 worked example classifies as documented; `test_benchmark_dupes.py` drives the reference metric (#57) and asserts every `duplicate-noise.md` §7 worked example clusters as documented; `test_benchmark_citation.py` drives the reference citation-existence check (#349) and asserts every `citation-fidelity.md` §7 worked example is classified as documented. |
| `unit/governance/` | Repository automation and hygiene: issue-claim reconciliation, issue-label sync, PR-description length, and the repository-wide Markdown-link validator. |
| `unit/release/` | Coverage for `scripts/release/release_worthiness.py`, split along its path-classification, CHANGELOG coverage, Unreleased roll, main()/`$GITHUB_OUTPUT` contract, pure SemVer/ref helpers, and preflight/verify responsibilities (`_shared.py` holds the fake Git/GitHub runner and CHANGELOG fixtures they share), plus `test_changelog_generation.py` and `test_release_intent.py`. |
-| `unit/review/` | Unit coverage for the `reference/review/` models. Sub-packages give the specific policy/capability each module protects a predictable home (#293): `findings/` (finding confidence/contract/identity, the machine-readable review result schema #67, plus `test_finding_identity_regression.py` — the data-driven finding-identity regression corpus, #61), `stateful_review/` (delta re-review, review-status enforcement, and `test_rereview_regression_fixtures.py` — the data-driven stateful delta re-review regression corpus of paired before/after review histories, #66), `specialist_depth/` (risk-based review depth corpus and scenarios, #90), `root_cause/` (root-cause consolidation, semantic-implication and null-absence corpora, `test_architectural_placement_fixtures.py` — paired local-only vs. bounded context-expansion outcomes for architecturally misplaced behavior, #153 — `test_candidate_finding_validation.py` — the observation → candidate claim → validated finding → severity reasoning-gate corpus, #382 — and `test_candidate_finding_validation_corpus.py` — the `docs/benchmark/corpus/candidate-finding-validation/` precision sub-corpus pinning that contract's false-escalation and non-suppression outcomes, plus a PR-#390-derived real-world scenario, #383), `repository_intelligence/` (repository expansion, instructions, and the repository-intelligence model plus its corpus), and `stacked_pr/` (stacked-PR topology and end-to-end checkout). Everything else — decision semantics, invocation options, PR/Jira context, parallel review, and other modules with no second clustering member — stays directly under `unit/review/`. |
+| `unit/review/` | Unit coverage for the `reference/review/` models. Sub-packages give the specific policy/capability each module protects a predictable home (#293): `findings/` (finding confidence/contract/identity, the machine-readable review result schema #67, `test_structured_output_contract.py` — cross-Skill contract tests over the paired sample outputs in `structured_output_samples/`, #71, plus `test_finding_identity_regression.py` — the data-driven finding-identity regression corpus, #61), `stateful_review/` (delta re-review, review-status enforcement, and `test_rereview_regression_fixtures.py` — the data-driven stateful delta re-review regression corpus of paired before/after review histories, #66), `specialist_depth/` (risk-based review depth corpus and scenarios, #90), `root_cause/` (root-cause consolidation, semantic-implication and null-absence corpora, `test_architectural_placement_fixtures.py` — paired local-only vs. bounded context-expansion outcomes for architecturally misplaced behavior, #153 — `test_candidate_finding_validation.py` — the observation → candidate claim → validated finding → severity reasoning-gate corpus, #382 — and `test_candidate_finding_validation_corpus.py` — the `docs/benchmark/corpus/candidate-finding-validation/` precision sub-corpus pinning that contract's false-escalation and non-suppression outcomes, plus a PR-#390-derived real-world scenario, #383), `repository_intelligence/` (repository expansion, instructions, and the repository-intelligence model plus its corpus), and `stacked_pr/` (stacked-PR topology and end-to-end checkout). Everything else — decision semantics, invocation options, PR/Jira context, parallel review, and other modules with no second clustering member — stays directly under `unit/review/`. |
| `integration/github/` | Coverage that shells out to real Git for the GitHub PR checkout lifecycle. |
| `integration/packaging/` | End-to-end packaging-boundary guard: manifest/path-safety, script parity, hidden-runtime-dependency + disclaimer prose, and the built local/GitHub archive contents (`_shared.py` holds the shared paths, manifest helpers, and the reference-module list). `test_distribution_consumer_install.py` (#511) checks that the published distribution tree is discoverable and installable through the `skills` CLI with self-contained copies; its `npx` tests run only with `DISTRIBUTION_INSTALL_CHECK=1`. |
| `integration/release/` | Coverage that builds the packaged archives to exercise the release-worthiness PR boundary. |
diff --git a/tests/integration/packaging/_shared.py b/tests/integration/packaging/_shared.py
index 8f573a9..1d6261a 100644
--- a/tests/integration/packaging/_shared.py
+++ b/tests/integration/packaging/_shared.py
@@ -58,6 +58,7 @@ def _reference_module_path(name: str) -> Path:
"finding_confidence.py",
"review_telemetry.py",
"review_result.py",
+ "structured_output_contract.py",
"invocation_options.py",
"finding_identity.py",
"runtime_validation.py",
diff --git a/tests/policy/review/findings/test_review_result_docs.py b/tests/policy/review/findings/test_review_result_docs.py
index 7b59d69..42c9598 100644
--- a/tests/policy/review/findings/test_review_result_docs.py
+++ b/tests/policy/review/findings/test_review_result_docs.py
@@ -156,6 +156,28 @@ def test_model_defers_versioning_and_emission(self) -> None:
self.assertRegex(text, rf"issues/{issue[1:]}\)")
+class ContractTestRecordTests(unittest.TestCase):
+ def _boundaries(self) -> str:
+ return MODEL.read_text(encoding="utf-8").split("## 7. Boundaries", 1)[1].split("## 8.", 1)[0]
+
+ def test_consumers_are_not_attributed_to_the_contract_test_issue(self) -> None:
+ row = next(line for line in self._boundaries().splitlines() if line.startswith("| Consumers of the result"))
+ self.assertNotIn("issues/71", row)
+
+ def test_contract_tests_are_recorded_and_linked(self) -> None:
+ self.assertIn("| Cross-Skill contract tests (section 8) | [#71]", self._boundaries())
+ section = MODEL.read_text(encoding="utf-8").split("## 8. Cross-Skill contract tests", 1)[1]
+ for target in (
+ "../../tests/reference/review/structured_output_contract.py",
+ "../../tests/unit/review/findings/structured_output_samples/",
+ ):
+ with self.subTest(target=target):
+ self.assertIn(f"]({target})", section)
+
+ def test_contract_module_is_registered_as_test_only(self) -> None:
+ self.assertIn("structured_output_contract.py", _shared.REFERENCE_TEST_MODULES)
+
+
class WiringAndPackagingTests(unittest.TestCase):
def test_architecture_map_links_the_record(self) -> None:
self.assertIn("review-result/README.md", ARCHITECTURE.read_text(encoding="utf-8"))
diff --git a/tests/reference/review/structured_output_contract.py b/tests/reference/review/structured_output_contract.py
new file mode 100644
index 0000000..09e3552
--- /dev/null
+++ b/tests/reference/review/structured_output_contract.py
@@ -0,0 +1,276 @@
+#!/usr/bin/env python3
+"""Test-only cross-Skill contract checks for the structured review result (Issue #71).
+
+Contract: docs/review-result/review-result-model.md section 8. Splits one
+Skill output into its human report and its structured result, checks the
+result against the versioned schema (fail closed), checks each Skill's
+surface-specific population, and compares the result with the human report
+without re-deriving the decision. Not runtime logic, not packaged.
+"""
+
+from __future__ import annotations
+
+import json
+import re
+from collections import Counter
+from dataclasses import dataclass
+from typing import Any, Mapping, Optional
+
+from tests.reference.review import review_result as rr
+from tests.reference.review import review_result_version as rv
+from tests.reference.review.verdict_consistency import RenderedSignal
+
+LOCAL = "local-code-review"
+GITHUB = "github-pr-review"
+SKILLS = (LOCAL, GITHUB)
+
+# Only these top-level fields are populated per surface; every other field is shared.
+SURFACE_SPECIFIC_FIELDS = ("skill", "reviewed_state")
+
+STRUCTURED_HEADING = "### Structured Review Result"
+NOT_EMITTED_PREFIX = "Structured review result not emitted:"
+
+# review-result-model.md section 4: rendered decision label -> machine outcome.
+_SIGNAL_OUTCOME = {
+ RenderedSignal.REVIEW_CLEAN: "clean",
+ RenderedSignal.APPROVE: "clean",
+ RenderedSignal.CHANGES_REQUIRED: "blocking",
+ RenderedSignal.REQUEST_CHANGES: "blocking",
+ RenderedSignal.REVIEW_INCOMPLETE: "incomplete",
+}
+# A GitHub self-review renders the local-style labels (external-review-summary.md).
+_SKILL_SIGNALS = {
+ LOCAL: (RenderedSignal.REVIEW_CLEAN, RenderedSignal.CHANGES_REQUIRED, RenderedSignal.REVIEW_INCOMPLETE),
+ GITHUB: tuple(RenderedSignal),
+}
+RENDERED_OUTCOME: dict[str, dict[str, str]] = {
+ skill: {signal.value.upper(): _SIGNAL_OUTCOME[signal] for signal in signals}
+ for skill, signals in _SKILL_SIGNALS.items()
+}
+
+_JSON_FENCE = re.compile(r"^```json[ \t]*\n(.*?)\n```[ \t]*$", re.M | re.S)
+_DECISION = re.compile(r"^### Decision[ \t]*\n+\*\*([^*\n]+)\*\*", re.M)
+_SEVERITY = r"(P[012])(?: \([^)\n]*\))?"
+_FULL_FINDING = re.compile(rf"^#### (F\d+) (?:\[{_SEVERITY}\]|{_SEVERITY}:) (.+?)[ \t]*$", re.M)
+_POINTER_FINDING = re.compile(rf"^- \*\*{_SEVERITY} — (.+?)\*\*", re.M)
+_SECTION_END = re.compile(r"^#{2,4} ", re.M)
+_AFFECTED = re.compile(r"^- \*\*Affected locations:\*\*[ \t]*\n((?:[ \t]+- .*\n?)+)", re.M)
+_AFFECTED_ENTRY = re.compile(r"^[ \t]+- `([^`]+)` — (.+?)[ \t]*$", re.M)
+_LOCAL_COUNTS = re.compile(r"^- P0: (\d+), P1: (\d+), P2: (\d+)[ \t]*$", re.M)
+_GITHUB_COUNT = re.compile(r"^- (P[012]): (\d+)[ \t]*$", re.M)
+_COVERAGE = {
+ LOCAL: re.compile(r"^- Coverage: (complete|incomplete)\b", re.M),
+ GITHUB: re.compile(r"^- coverage: `(complete|incomplete)\b", re.M),
+}
+_HEAD = {
+ LOCAL: re.compile(r"^- Local HEAD: `([0-9a-f]{40})`", re.M),
+ GITHUB: re.compile(r"^- reviewed_head: `([0-9a-f]{40})`", re.M),
+}
+
+
+class ContractError(ValueError):
+ """The output cannot be consumed: missing, ambiguous, or malformed result."""
+
+
+@dataclass(frozen=True)
+class SplitOutput:
+ human: str
+ result: Optional[dict]
+ not_emitted_reason: Optional[str] = None
+
+
+@dataclass(frozen=True)
+class RenderedFinding:
+ id: Optional[str]
+ severity: str
+ title: str
+ # None when the finding was not rendered in full (a GitHub pointer line).
+ affected_locations: Optional[tuple[tuple[str, str], ...]] = None
+
+
+@dataclass(frozen=True)
+class HumanReport:
+ decision_label: Optional[str]
+ counts: Optional[dict[str, int]]
+ coverage: Optional[str]
+ head: Optional[str]
+ findings: tuple[RenderedFinding, ...]
+
+
+def split_output(text: str, skill: str) -> SplitOutput:
+ """The human part precedes the single fenced `json` result; the result is last."""
+ fences = list(_JSON_FENCE.finditer(text))
+ if not fences:
+ for line in text.splitlines():
+ if line.startswith(NOT_EMITTED_PREFIX):
+ return SplitOutput(text, None, line[len(NOT_EMITTED_PREFIX):].strip())
+ raise ContractError("no structured result block and no not-emitted statement")
+ if len(fences) > 1:
+ raise ContractError(f"expected exactly one json block, found {len(fences)}")
+ fence = fences[0]
+ if skill == GITHUB:
+ # Only "after the caller-facing report": the Reviewer Brief may precede or follow it.
+ if not _DECISION.search(text, 0, fence.start()):
+ raise ContractError("the structured result must follow the report's Decision")
+ return SplitOutput(text[: fence.start()] + text[fence.end():], _load(fence.group(1)))
+ if text[fence.end():].strip():
+ raise ContractError("the structured result must be the last part of the output")
+ human = text[: fence.start()]
+ if skill == LOCAL:
+ stripped = human.rstrip()
+ if not stripped.endswith(STRUCTURED_HEADING) or human.count(STRUCTURED_HEADING) != 1:
+ raise ContractError(f"local result must directly follow one {STRUCTURED_HEADING!r} heading")
+ human = stripped[: -len(STRUCTURED_HEADING)]
+ return SplitOutput(human, _load(fence.group(1)))
+
+
+def _load(block: str) -> Any:
+ try:
+ return json.loads(block)
+ except json.JSONDecodeError as exc:
+ raise ContractError(f"structured result is not valid JSON: {exc}") from exc
+
+
+def producer_errors(result: Any) -> tuple[str, ...]:
+ """Fail closed on a missing/malformed or other-MAJOR version (schema-versioning.md §3) and on
+ a version newer than the published schema (§4: producers emit the version they target)."""
+ if not isinstance(result, dict):
+ return ("$: structured result is not a JSON object",)
+ major, minor, _patch = rv.parse_version(rr.SCHEMA_VERSION)
+ decision = rv.consumer_decision(result.get("schema_version"), major, minor)
+ if decision is rv.VersionDecision.REJECT_INVALID:
+ return ("$.schema_version: missing or not MAJOR.MINOR.PATCH",)
+ if decision is rv.VersionDecision.REJECT_UNSUPPORTED_MAJOR:
+ return (f"$.schema_version: unsupported major in {result['schema_version']!r}",)
+ if decision is rv.VersionDecision.ACCEPT_KNOWN_FIELDS_ONLY:
+ return (f"$.schema_version: {result['schema_version']!r} is newer than the published schema",)
+ return rr.validate_review_result(result)
+
+
+def surface_errors(
+ result: Mapping, skill: str, known_head: Optional[str], target_committed: bool = True
+) -> tuple[str, ...]:
+ """Each Skill's structured-output policy for the surface-specific fields."""
+ errors: list[str] = []
+ state = result["reviewed_state"]
+ if result["skill"] != skill:
+ errors.append(f"$.skill: {result['skill']!r}, produced by {skill!r}")
+ head = state["reviewed_head_sha"]
+ if skill == LOCAL:
+ expected = known_head if target_committed else None
+ if head != expected:
+ errors.append(f"$.reviewed_state.reviewed_head_sha: {head!r}, workspace gives {expected!r}")
+ if state["completeness"] != "full":
+ errors.append("$.reviewed_state.completeness: a local review is always 'full'")
+ if state["prior_reviewed_sha"] is not None:
+ errors.append("$.reviewed_state.prior_reviewed_sha: a stateless local review has none")
+ elif head is None:
+ if result["coverage"] != "incomplete":
+ errors.append("$.reviewed_state.reviewed_head_sha: null only when the review is incomplete")
+ elif head != known_head:
+ errors.append(f"$.reviewed_state.reviewed_head_sha: {head!r}, PR head is {known_head!r}")
+ return tuple(errors)
+
+
+def shared_projection(result: Mapping) -> dict:
+ """The result with its surface-specific fields removed."""
+ return {key: value for key, value in result.items() if key not in SURFACE_SPECIFIC_FIELDS}
+
+
+def parse_human_report(text: str, skill: str) -> HumanReport:
+ decision = _DECISION.search(text)
+ coverage = _COVERAGE[skill].search(text)
+ head = _HEAD[skill].search(text)
+ return HumanReport(
+ decision_label=decision.group(1).strip() if decision else None,
+ counts=_counts(text, skill),
+ coverage=coverage.group(1) if coverage else None,
+ head=head.group(1) if head else None,
+ findings=_rendered_findings(text, skill),
+ )
+
+
+def _counts(text: str, skill: str) -> Optional[dict[str, int]]:
+ if skill == LOCAL:
+ match = _LOCAL_COUNTS.search(text)
+ return dict(zip(("p0", "p1", "p2"), map(int, match.groups()))) if match else None
+ found = {level.lower(): int(n) for level, n in _GITHUB_COUNT.findall(text)}
+ return found if len(found) == 3 else None
+
+
+def _rendered_findings(text: str, skill: str) -> tuple[RenderedFinding, ...]:
+ full = [
+ RenderedFinding(m.group(1), m.group(2) or m.group(3), m.group(4), _affected(text, m.end()))
+ for m in _FULL_FINDING.finditer(text)
+ ]
+ if skill == LOCAL:
+ return tuple(full)
+ # A GitHub pointer line and a body block for the same finding count once.
+ pointers = [RenderedFinding(None, m.group(1), m.group(2)) for m in _POINTER_FINDING.finditer(text)]
+ in_body = {(f.severity, f.title) for f in full}
+ return tuple(full + [p for p in pointers if (p.severity, p.title) not in in_body])
+
+
+def _affected(text: str, start: int) -> tuple[tuple[str, str], ...]:
+ end = _SECTION_END.search(text, start)
+ block = text[start : end.start() if end else len(text)]
+ listing = _AFFECTED.search(block)
+ return tuple(_AFFECTED_ENTRY.findall(listing.group(1))) if listing else ()
+
+
+def agreement_errors(report: HumanReport, result: Mapping, skill: str) -> tuple[str, ...]:
+ """Compares rendered facts with the result; never re-derives the decision."""
+ errors: list[str] = []
+ label = (report.decision_label or "").split(" — ", 1)[0].strip().upper()
+ outcome = RENDERED_OUTCOME[skill].get(label)
+ if outcome is None:
+ errors.append(f"human report: unrecognized decision label {report.decision_label!r}")
+ elif outcome != result["decision"]["outcome"]:
+ errors.append(f"decision: report renders {label!r}, result says {result['decision']['outcome']!r}")
+
+ if report.counts != dict(result["counts"]):
+ errors.append(f"counts: report {report.counts}, result {dict(result['counts'])}")
+ if report.coverage != result["coverage"]:
+ errors.append(f"coverage: report {report.coverage!r}, result {result['coverage']!r}")
+ head = result["reviewed_state"]["reviewed_head_sha"]
+ if head is not None and report.head != head:
+ errors.append(f"reviewed head: report {report.head!r}, result {head!r}")
+
+ rendered = Counter((f.severity, f.title) for f in report.findings)
+ structured = Counter((f["severity"], f["title"]) for f in result["findings"])
+ if rendered != structured:
+ errors.append(
+ f"findings: only in report {sorted((rendered - structured).elements())}, "
+ f"only in result {sorted((structured - rendered).elements())}"
+ )
+ by_key = {(f["severity"], f["title"]): f for f in result["findings"]}
+ for finding in report.findings:
+ match = by_key.get((finding.severity, finding.title))
+ if match is None:
+ continue
+ if finding.id is not None and match["id"] != finding.id:
+ errors.append(f"finding id: report {finding.id!r}, result {match['id']!r} for {finding.title!r}")
+ structured_sites = tuple((a["location"], a["note"]) for a in match.get("affected_locations", ()))
+ if finding.affected_locations is not None and finding.affected_locations != structured_sites:
+ errors.append(f"affected locations: report {finding.affected_locations}, result {structured_sites}")
+ if skill == LOCAL and [f.id for f in report.findings] != [f["id"] for f in result["findings"]]:
+ errors.append("findings: result order differs from report order")
+ return tuple(errors)
+
+
+def contract_errors(
+ text: str, skill: str, known_head: Optional[str], target_committed: bool = True
+) -> tuple[str, ...]:
+ """Every check for one emitted output; empty means the output honors the contract."""
+ try:
+ split = split_output(text, skill)
+ except ContractError as exc:
+ return (str(exc),)
+ if split.result is None:
+ return ("structured result was not emitted",)
+ errors = producer_errors(split.result)
+ if errors:
+ return errors
+ return surface_errors(split.result, skill, known_head, target_committed) + agreement_errors(
+ parse_human_report(split.human, skill), split.result, skill
+ )
diff --git a/tests/unit/benchmark/test_production_adapter.py b/tests/unit/benchmark/test_production_adapter.py
index 0e9a361..5b41ef1 100644
--- a/tests/unit/benchmark/test_production_adapter.py
+++ b/tests/unit/benchmark/test_production_adapter.py
@@ -492,6 +492,31 @@ def test_decision_heading_without_label_is_none(self) -> None:
self.assertIsNone(outcome.decision_label)
+
+class StructuredReviewResultBoundaryTests(unittest.TestCase):
+ """#71 audit: the benchmark scores only the Markdown report; the opt-in
+ structured result (#69) stays off and cannot leak into parsed findings."""
+
+ SAMPLE = (
+ Path(__file__).resolve().parents[1]
+ / "review" / "findings" / "structured_output_samples"
+ / "local-code-review" / "blocking-committed.md"
+ )
+
+ def test_benchmark_prompt_leaves_the_structured_result_off(self) -> None:
+ from runtime_platform.benchmark.scripts.benchmark_review_adapter import _REVIEW_PROMPT
+ from tests.reference.review.invocation_options import OPTION_CONCEPTS, normalize
+
+ resolved = normalize(_REVIEW_PROMPT, defaults=dict.fromkeys(OPTION_CONCEPTS, False))
+ self.assertIs(resolved["structured_review_result"], False)
+
+ def test_appended_structured_result_does_not_change_parsed_output(self) -> None:
+ text = self.SAMPLE.read_text(encoding="utf-8")
+ report_only = text.split("### Structured Review Result", 1)[0]
+ self.assertEqual(parse_review_output(text), parse_review_output(report_only))
+ self.assertEqual(parse_rendered_outcome(text), parse_rendered_outcome(report_only))
+ self.assertEqual([f.severity for f in parse_review_output(text)], ["P1", "P1", "P2"])
+
class _StubCliMixin:
"""Writes an executable stub script standing in for the real `claude`
CLI, so the subprocess-invocation boundary is tested without depending
diff --git a/tests/unit/review/findings/structured_output_samples/github-pr-review/blocking.md b/tests/unit/review/findings/structured_output_samples/github-pr-review/blocking.md
new file mode 100644
index 0000000..5212ced
--- /dev/null
+++ b/tests/unit/review/findings/structured_output_samples/github-pr-review/blocking.md
@@ -0,0 +1,148 @@
+## Review Summary
+
+**Result: ⚠️ CHANGES REQUIRED**
+
+Not safe to merge at `b81d4a9` yet. Two blocking issues need to be addressed; see the inline comments for detail.
+
+### Findings
+
+- **P1 — Retry loop reports a timed-out charge as successful**
+ `src/payments/retry.py:88`
+- **P2 — Timeout path of the retry loop is untested**
+ `tests/unit/payments/test_retry.py`
+
+#### F2 [P1] Idempotency key is dropped on both charge paths
+
+- **Location:** `src/payments/client.py:41`
+- **Affected locations:**
+ - `src/payments/charge.py:charge` — a retried charge is submitted twice
+ - `src/payments/refund.py:refund` — a retried refund is submitted twice
+- **Evidence:** `PaymentsClient.post` rebuilds `headers` without the `Idempotency-Key` the callers pass in.
+- **Impact:** Any retried charge or refund is processed twice by the provider.
+- **Fix:** Forward the caller's `Idempotency-Key` header in `PaymentsClient.post`.
+
+Validation: `executed` — `python -m pytest tests/unit/payments` (declared in `Makefile`, exit 0).
+
+### Decision
+**REQUEST CHANGES**
+
+
+Review metadata
+
+- reviewed_head: `b81d4a9e0c7f3625d1e8a4b6c90f2e7d5a31c84f`
+- review_mode: `full`
+- stacked_pr: `none detected` (base is the repository's default branch)
+- change_risk_depth: `elevated`
+- change_risk_signals: `payment-path (elevated) — src/payments/retry.py`
+- repository_expansion_triggers: `none`
+- coverage: `complete`
+- P0: 0
+- P1: 2
+- P2: 1
+- decision: `request_changes`
+- publication_mode: `active`
+- mutation: `submitted (REQUEST_CHANGES)`
+
+
+
+## Reviewer Brief
+
+- **What changed:** Adds bounded retries around the charge call and routes charges and refunds through a shared client.
+- **User-provided focus:** none provided.
+- **Manual review focus:**
+ - Confirm the provider deduplicates on `Idempotency-Key` for refunds as well as charges.
+
+```json
+{
+ "schema_version": "1.0.0",
+ "skill": "github-pr-review",
+ "reviewed_state": {
+ "repository": "acme/payments",
+ "base_branch": "main",
+ "base_sha": "3f9c1e2a7b4d5c60918273645a1b2c3d4e5f6071",
+ "merge_base_sha": "3f9c1e2a7b4d5c60918273645a1b2c3d4e5f6071",
+ "reviewed_head_sha": "b81d4a9e0c7f3625d1e8a4b6c90f2e7d5a31c84f",
+ "reviewer_identity": "review-bot",
+ "completeness": "full",
+ "prior_reviewed_sha": null
+ },
+ "coverage": "complete",
+ "decision": {
+ "derived": "blocking",
+ "outcome": "blocking"
+ },
+ "counts": {
+ "p0": 0,
+ "p1": 2,
+ "p2": 1
+ },
+ "summary": "Adds bounded retries around the charge call and routes charges and refunds through a shared client.",
+ "findings": [
+ {
+ "id": "F1",
+ "severity": "P1",
+ "title": "Retry loop reports a timed-out charge as successful",
+ "location": "src/payments/retry.py:88",
+ "fix_location_resolved": true,
+ "evidence": "`charge_with_retry` catches `TimeoutError` and `continue`s; after the last attempt it falls through to `return ChargeResult.ok()`.",
+ "impact": "A charge that never completed is recorded as paid, so the order ships without payment.",
+ "fix": "Return a failure result when every attempt timed out.",
+ "runtime_validation": "reasoned",
+ "confidence": "credible",
+ "defect_kind": "swallowed-exception",
+ "identity": {
+ "stable_id": "fid_v1_2c7a0d6b2c80a885a1752c05f15d70c0",
+ "matching_eligible": true
+ }
+ },
+ {
+ "id": "F2",
+ "severity": "P1",
+ "title": "Idempotency key is dropped on both charge paths",
+ "location": "src/payments/client.py:41",
+ "fix_location_resolved": true,
+ "affected_locations": [
+ {
+ "location": "src/payments/charge.py:charge",
+ "note": "a retried charge is submitted twice"
+ },
+ {
+ "location": "src/payments/refund.py:refund",
+ "note": "a retried refund is submitted twice"
+ }
+ ],
+ "evidence": "`PaymentsClient.post` rebuilds `headers` without the `Idempotency-Key` the callers pass in.",
+ "impact": "Any retried charge or refund is processed twice by the provider.",
+ "fix": "Forward the caller's `Idempotency-Key` header in `PaymentsClient.post`.",
+ "runtime_validation": "runtime-confirmed",
+ "confidence": "confirmed",
+ "defect_kind": "missing-idempotency-key",
+ "identity": {
+ "stable_id": "fid_v1_43694218c49a2784e413ea3dfd660bc3",
+ "matching_eligible": true
+ }
+ },
+ {
+ "id": "F3",
+ "severity": "P2",
+ "title": "Timeout path of the retry loop is untested",
+ "location": "tests/unit/payments/test_retry.py",
+ "fix_location_resolved": true,
+ "evidence_location": "src/payments/retry.py:80-92",
+ "evidence": "The only new test, `test_charge_retries_on_failure`, exercises a non-timeout failure.",
+ "impact": "The timeout regression would not be caught by the suite.",
+ "fix": "Add a test that makes every attempt time out and asserts a failure result.",
+ "runtime_validation": "attempted-inconclusive",
+ "contextual_evidence": [
+ "acceptance criterion: failed charges must never be recorded as paid"
+ ],
+ "confidence": "runtime-validation-unavailable",
+ "defect_kind": "missing-test-coverage",
+ "identity": {
+ "stable_id": "fid_v1_d3b3f8809dcaa5f1dfd78b9cbed739ba",
+ "matching_eligible": true
+ }
+ }
+ ]
+}
+```
diff --git a/tests/unit/review/findings/structured_output_samples/github-pr-review/clean-p2-delta.md b/tests/unit/review/findings/structured_output_samples/github-pr-review/clean-p2-delta.md
new file mode 100644
index 0000000..f37d610
--- /dev/null
+++ b/tests/unit/review/findings/structured_output_samples/github-pr-review/clean-p2-delta.md
@@ -0,0 +1,81 @@
+## Review Summary
+
+**Result: ✅ REVIEW CLEAN**
+
+No blocking findings at `5e0f7c2`.
+
+### Findings
+
+- **P2 — Recovered retries log at error level**
+ `src/payments/retry.py:95`
+
+Validation: `skipped` — no declared command (no validation executed).
+
+### Decision
+**APPROVE**
+
+
+Review metadata
+
+- reviewed_head: `5e0f7c2d9a1b4e3f6c8d0a2b4c6e8f0a1b3c5d7e`
+- review_mode: `delta (previous reviewed SHA c4a9e1f07b3d5a2c8e6f0b1d3a5c7e9f2b4d6a80, current HEAD 5e0f7c2d9a1b4e3f6c8d0a2b4c6e8f0a1b3c5d7e)`
+- stacked_pr: `none detected` (base is the repository's default branch)
+- change_risk_depth: `standard`
+- change_risk_signals: `none`
+- repository_expansion_triggers: `none`
+- coverage: `complete`
+- P0: 0
+- P1: 0
+- P2: 1
+- decision: `comment`
+- publication_mode: `passive`
+- mutation: `not_requested`
+
+
+
+```json
+{
+ "schema_version": "1.0.0",
+ "skill": "github-pr-review",
+ "reviewed_state": {
+ "repository": "acme/payments",
+ "base_branch": "main",
+ "base_sha": "3f9c1e2a7b4d5c60918273645a1b2c3d4e5f6071",
+ "merge_base_sha": "3f9c1e2a7b4d5c60918273645a1b2c3d4e5f6071",
+ "reviewed_head_sha": "5e0f7c2d9a1b4e3f6c8d0a2b4c6e8f0a1b3c5d7e",
+ "reviewer_identity": "review-bot",
+ "completeness": "delta-re-review",
+ "prior_reviewed_sha": "c4a9e1f07b3d5a2c8e6f0b1d3a5c7e9f2b4d6a80"
+ },
+ "coverage": "complete",
+ "decision": {
+ "derived": "clean",
+ "outcome": "clean"
+ },
+ "counts": {
+ "p0": 0,
+ "p1": 0,
+ "p2": 1
+ },
+ "summary": "Adds bounded retries around the charge call.",
+ "findings": [
+ {
+ "id": "F1",
+ "severity": "P2",
+ "title": "Recovered retries log at error level",
+ "location": "src/payments/retry.py:95",
+ "fix_location_resolved": true,
+ "evidence": "`charge_with_retry` calls `log.error` on every failed attempt, including ones a later attempt recovers.",
+ "impact": "Transient provider blips page on-call even though the charge succeeded.",
+ "fix": "Log intermediate attempts at warning level and reserve error for exhaustion.",
+ "runtime_validation": "reasoned",
+ "confidence": "credible",
+ "defect_kind": "log-level-misuse",
+ "identity": {
+ "stable_id": "fid_v1_099a6840e5ea0da08b641f054051f011",
+ "matching_eligible": true
+ }
+ }
+ ]
+}
+```
diff --git a/tests/unit/review/findings/structured_output_samples/github-pr-review/incomplete-head-unknown.md b/tests/unit/review/findings/structured_output_samples/github-pr-review/incomplete-head-unknown.md
new file mode 100644
index 0000000..873282d
--- /dev/null
+++ b/tests/unit/review/findings/structured_output_samples/github-pr-review/incomplete-head-unknown.md
@@ -0,0 +1,56 @@
+## Review Summary
+
+**Result: ⚠️ REVIEW INCOMPLETE**
+
+Not reviewed: the PR head could not be established, so no reviewed revision can be named.
+
+### Decision
+**REVIEW INCOMPLETE**
+
+
+Review metadata
+
+- reviewed_head: unknown
+- review_mode: `full`
+- stacked_pr: `none detected` (base is the repository's default branch)
+- change_risk_depth: `standard`
+- change_risk_signals: `none`
+- repository_expansion_triggers: `none`
+- coverage: `incomplete — PR head could not be established`
+- P0: 0
+- P1: 0
+- P2: 0
+- decision: `comment`
+- publication_mode: `passive`
+- mutation: `not_requested`
+
+
+
+```json
+{
+ "schema_version": "1.0.0",
+ "skill": "github-pr-review",
+ "reviewed_state": {
+ "repository": "acme/payments",
+ "base_branch": "main",
+ "base_sha": "3f9c1e2a7b4d5c60918273645a1b2c3d4e5f6071",
+ "merge_base_sha": "3f9c1e2a7b4d5c60918273645a1b2c3d4e5f6071",
+ "reviewed_head_sha": null,
+ "reviewer_identity": "review-bot",
+ "completeness": "full",
+ "prior_reviewed_sha": null
+ },
+ "coverage": "incomplete",
+ "decision": {
+ "derived": "clean",
+ "outcome": "incomplete"
+ },
+ "counts": {
+ "p0": 0,
+ "p1": 0,
+ "p2": 0
+ },
+ "summary": "Adds bounded retries around the charge call.",
+ "findings": []
+}
+```
diff --git a/tests/unit/review/findings/structured_output_samples/local-code-review/blocking-committed.md b/tests/unit/review/findings/structured_output_samples/local-code-review/blocking-committed.md
new file mode 100644
index 0000000..190c100
--- /dev/null
+++ b/tests/unit/review/findings/structured_output_samples/local-code-review/blocking-committed.md
@@ -0,0 +1,163 @@
+## Code Review
+
+**Result: ⚠️ Changes Requested**
+
+Not safe to proceed: a timed-out charge is recorded as paid, and retried requests are processed twice.
+
+### What changed
+Adds bounded retries around the charge call and routes charges and refunds through a shared client.
+
+### What was done well
+- **Bounded retries:** the attempt limit is a named constant with a test.
+
+### Findings
+
+#### F1 [P1] Retry loop reports a timed-out charge as successful
+
+- **Location:** `src/payments/retry.py:88` _(committed)_
+- **Evidence:** `charge_with_retry` catches `TimeoutError` and `continue`s; after the last attempt it falls through to `return ChargeResult.ok()`.
+- **Impact:** A charge that never completed is recorded as paid, so the order ships without payment.
+- **Fix:** Return a failure result when every attempt timed out.
+
+#### F2 [P1] Idempotency key is dropped on both charge paths
+
+- **Location:** `src/payments/client.py:41` _(committed)_
+- **Affected locations:**
+ - `src/payments/charge.py:charge` — a retried charge is submitted twice
+ - `src/payments/refund.py:refund` — a retried refund is submitted twice
+- **Evidence:** `PaymentsClient.post` rebuilds `headers` without the `Idempotency-Key` the callers pass in.
+- **Impact:** Any retried charge or refund is processed twice by the provider.
+- **Fix:** Forward the caller's `Idempotency-Key` header in `PaymentsClient.post`.
+
+#### F3 [P2] Timeout path of the retry loop is untested
+
+- **Location:** `tests/unit/payments/test_retry.py` _(committed)_
+- **Evidence:** The only new test, `test_charge_retries_on_failure`, exercises a non-timeout failure.
+- **Impact:** The timeout regression would not be caught by the suite.
+- **Fix:** Add a test that makes every attempt time out and asserts a failure result.
+
+### Validation
+- `executed` — `python -m pytest tests/unit/payments` (declared in `Makefile`, exit 0).
+
+### Decision
+**CHANGES REQUIRED**
+
+2 P1 findings must be addressed before this implementation should proceed.
+
+### Review Metadata
+
+- Base branch: `main`
+- Base SHA: `3f9c1e2a7b4d5c60918273645a1b2c3d4e5f6071`
+- Local HEAD: `b81d4a9e0c7f3625d1e8a4b6c90f2e7d5a31c84f`
+- Remote HEAD: none
+- Synchronization status: no tracking branch
+- P0: 0, P1: 2, P2: 1
+- Change-risk depth: elevated
+- Change-risk signals: payment-path (elevated) — `src/payments/retry.py`
+- Repository expansion: none
+- Coverage: complete
+
+**Review scope contract**:
+
+- Committed delta relative to base: included, `main..HEAD` (3 files)
+- Staged: excluded, empty
+- Unstaged: excluded, empty
+- Untracked: excluded, empty
+- Review kind: initial review
+
+### Structured Review Result
+
+```json
+{
+ "schema_version": "1.0.0",
+ "skill": "local-code-review",
+ "reviewed_state": {
+ "repository": "acme/payments",
+ "base_branch": "main",
+ "base_sha": "3f9c1e2a7b4d5c60918273645a1b2c3d4e5f6071",
+ "merge_base_sha": "3f9c1e2a7b4d5c60918273645a1b2c3d4e5f6071",
+ "reviewed_head_sha": "b81d4a9e0c7f3625d1e8a4b6c90f2e7d5a31c84f",
+ "reviewer_identity": null,
+ "completeness": "full",
+ "prior_reviewed_sha": null
+ },
+ "coverage": "complete",
+ "decision": {
+ "derived": "blocking",
+ "outcome": "blocking"
+ },
+ "counts": {
+ "p0": 0,
+ "p1": 2,
+ "p2": 1
+ },
+ "summary": "Adds bounded retries around the charge call and routes charges and refunds through a shared client.",
+ "findings": [
+ {
+ "id": "F1",
+ "severity": "P1",
+ "title": "Retry loop reports a timed-out charge as successful",
+ "location": "src/payments/retry.py:88",
+ "fix_location_resolved": true,
+ "evidence": "`charge_with_retry` catches `TimeoutError` and `continue`s; after the last attempt it falls through to `return ChargeResult.ok()`.",
+ "impact": "A charge that never completed is recorded as paid, so the order ships without payment.",
+ "fix": "Return a failure result when every attempt timed out.",
+ "runtime_validation": "reasoned",
+ "confidence": "credible",
+ "defect_kind": "swallowed-exception",
+ "identity": {
+ "stable_id": "fid_v1_2c7a0d6b2c80a885a1752c05f15d70c0",
+ "matching_eligible": true
+ }
+ },
+ {
+ "id": "F2",
+ "severity": "P1",
+ "title": "Idempotency key is dropped on both charge paths",
+ "location": "src/payments/client.py:41",
+ "fix_location_resolved": true,
+ "affected_locations": [
+ {
+ "location": "src/payments/charge.py:charge",
+ "note": "a retried charge is submitted twice"
+ },
+ {
+ "location": "src/payments/refund.py:refund",
+ "note": "a retried refund is submitted twice"
+ }
+ ],
+ "evidence": "`PaymentsClient.post` rebuilds `headers` without the `Idempotency-Key` the callers pass in.",
+ "impact": "Any retried charge or refund is processed twice by the provider.",
+ "fix": "Forward the caller's `Idempotency-Key` header in `PaymentsClient.post`.",
+ "runtime_validation": "runtime-confirmed",
+ "confidence": "confirmed",
+ "defect_kind": "missing-idempotency-key",
+ "identity": {
+ "stable_id": "fid_v1_43694218c49a2784e413ea3dfd660bc3",
+ "matching_eligible": true
+ }
+ },
+ {
+ "id": "F3",
+ "severity": "P2",
+ "title": "Timeout path of the retry loop is untested",
+ "location": "tests/unit/payments/test_retry.py",
+ "fix_location_resolved": true,
+ "evidence_location": "src/payments/retry.py:80-92",
+ "evidence": "The only new test, `test_charge_retries_on_failure`, exercises a non-timeout failure.",
+ "impact": "The timeout regression would not be caught by the suite.",
+ "fix": "Add a test that makes every attempt time out and asserts a failure result.",
+ "runtime_validation": "attempted-inconclusive",
+ "contextual_evidence": [
+ "acceptance criterion: failed charges must never be recorded as paid"
+ ],
+ "confidence": "runtime-validation-unavailable",
+ "defect_kind": "missing-test-coverage",
+ "identity": {
+ "stable_id": "fid_v1_d3b3f8809dcaa5f1dfd78b9cbed739ba",
+ "matching_eligible": true
+ }
+ }
+ ]
+}
+```
diff --git a/tests/unit/review/findings/structured_output_samples/local-code-review/clean-p2-uncommitted.md b/tests/unit/review/findings/structured_output_samples/local-code-review/clean-p2-uncommitted.md
new file mode 100644
index 0000000..011fac8
--- /dev/null
+++ b/tests/unit/review/findings/structured_output_samples/local-code-review/clean-p2-uncommitted.md
@@ -0,0 +1,95 @@
+## Code Review
+
+**Result: ✅ Review Clean**
+
+Safe to proceed: one non-blocking recommendation below.
+
+### What changed
+Adds bounded retries around the charge call.
+
+### Findings
+
+#### F1 [P2] Recovered retries log at error level
+
+- **Location:** `src/payments/retry.py:95` _(staged)_
+- **Evidence:** `charge_with_retry` calls `log.error` on every failed attempt, including ones a later attempt recovers.
+- **Impact:** Transient provider blips page on-call even though the charge succeeded.
+- **Fix:** Log intermediate attempts at warning level and reserve error for exhaustion.
+
+### Validation
+- `skipped` — no declared command covers the staged change.
+
+### Decision
+**REVIEW CLEAN**
+
+No P0 or P1 (blocking) findings were identified; the P2 finding above is a non-blocking recommendation and does not change this decision.
+
+### Review Metadata
+
+- Base branch: `main`
+- Base SHA: `3f9c1e2a7b4d5c60918273645a1b2c3d4e5f6071`
+- Local HEAD: `5e0f7c2d9a1b4e3f6c8d0a2b4c6e8f0a1b3c5d7e`
+- Remote HEAD: none
+- Synchronization status: no tracking branch
+- P0: 0, P1: 0, P2: 1
+- Change-risk depth: elevated
+- Change-risk signals: payment-path (elevated) — `src/payments/retry.py`
+- Repository expansion: none
+- Coverage: complete
+
+**Review scope contract**:
+
+- Committed delta relative to base: excluded, HEAD equals base
+- Staged: included, `src/payments/retry.py`
+- Unstaged: excluded, empty
+- Untracked: excluded, empty
+- Review kind: initial review
+
+### Structured Review Result
+
+```json
+{
+ "schema_version": "1.0.0",
+ "skill": "local-code-review",
+ "reviewed_state": {
+ "repository": "acme/payments",
+ "base_branch": "main",
+ "base_sha": "3f9c1e2a7b4d5c60918273645a1b2c3d4e5f6071",
+ "merge_base_sha": "3f9c1e2a7b4d5c60918273645a1b2c3d4e5f6071",
+ "reviewed_head_sha": null,
+ "reviewer_identity": null,
+ "completeness": "full",
+ "prior_reviewed_sha": null
+ },
+ "coverage": "complete",
+ "decision": {
+ "derived": "clean",
+ "outcome": "clean"
+ },
+ "counts": {
+ "p0": 0,
+ "p1": 0,
+ "p2": 1
+ },
+ "summary": "Adds bounded retries around the charge call.",
+ "findings": [
+ {
+ "id": "F1",
+ "severity": "P2",
+ "title": "Recovered retries log at error level",
+ "location": "src/payments/retry.py:95",
+ "fix_location_resolved": true,
+ "evidence": "`charge_with_retry` calls `log.error` on every failed attempt, including ones a later attempt recovers.",
+ "impact": "Transient provider blips page on-call even though the charge succeeded.",
+ "fix": "Log intermediate attempts at warning level and reserve error for exhaustion.",
+ "runtime_validation": "reasoned",
+ "confidence": "credible",
+ "defect_kind": "log-level-misuse",
+ "identity": {
+ "stable_id": "fid_v1_099a6840e5ea0da08b641f054051f011",
+ "matching_eligible": true
+ }
+ }
+ ]
+}
+```
diff --git a/tests/unit/review/findings/structured_output_samples/local-code-review/incomplete.md b/tests/unit/review/findings/structured_output_samples/local-code-review/incomplete.md
new file mode 100644
index 0000000..74c6ca6
--- /dev/null
+++ b/tests/unit/review/findings/structured_output_samples/local-code-review/incomplete.md
@@ -0,0 +1,95 @@
+## Code Review
+
+**Result: ⚠️ Review Incomplete**
+
+Not fully reviewed: the `src/payments/providers/` partition could not be completed. Do not treat this as safe to proceed.
+
+### What changed
+Adds bounded retries around the charge call.
+
+### Findings
+
+#### F1 [P1] Retry loop reports a timed-out charge as successful
+
+- **Location:** `src/payments/retry.py:88` _(committed)_
+- **Evidence:** `charge_with_retry` catches `TimeoutError` and `continue`s; after the last attempt it falls through to `return ChargeResult.ok()`.
+- **Impact:** A charge that never completed is recorded as paid, so the order ships without payment.
+- **Fix:** Return a failure result when every attempt timed out.
+
+### Validation
+- `unavailable` — the test runner could not start in this workspace.
+
+### Decision
+**REVIEW INCOMPLETE**
+
+The `src/payments/providers/` partition was not reviewed.
+
+### Review Metadata
+
+- Base branch: `main`
+- Base SHA: `3f9c1e2a7b4d5c60918273645a1b2c3d4e5f6071`
+- Local HEAD: `c4a9e1f07b3d5a2c8e6f0b1d3a5c7e9f2b4d6a80`
+- Remote HEAD: none
+- Synchronization status: no tracking branch
+- P0: 0, P1: 1, P2: 0
+- Change-risk depth: elevated
+- Change-risk signals: payment-path (elevated) — `src/payments/retry.py`
+- Repository expansion: none
+- Coverage: incomplete — partition `src/payments/providers/` not completed
+
+**Review scope contract**:
+
+- Committed delta relative to base: included, `main..HEAD` (41 files)
+- Staged: excluded, empty
+- Unstaged: excluded, empty
+- Untracked: excluded, empty
+- Review kind: initial review
+
+### Structured Review Result
+
+```json
+{
+ "schema_version": "1.0.0",
+ "skill": "local-code-review",
+ "reviewed_state": {
+ "repository": "acme/payments",
+ "base_branch": "main",
+ "base_sha": "3f9c1e2a7b4d5c60918273645a1b2c3d4e5f6071",
+ "merge_base_sha": "3f9c1e2a7b4d5c60918273645a1b2c3d4e5f6071",
+ "reviewed_head_sha": "c4a9e1f07b3d5a2c8e6f0b1d3a5c7e9f2b4d6a80",
+ "reviewer_identity": null,
+ "completeness": "full",
+ "prior_reviewed_sha": null
+ },
+ "coverage": "incomplete",
+ "decision": {
+ "derived": "blocking",
+ "outcome": "incomplete"
+ },
+ "counts": {
+ "p0": 0,
+ "p1": 1,
+ "p2": 0
+ },
+ "summary": "Adds bounded retries around the charge call.",
+ "findings": [
+ {
+ "id": "F1",
+ "severity": "P1",
+ "title": "Retry loop reports a timed-out charge as successful",
+ "location": "src/payments/retry.py:88",
+ "fix_location_resolved": true,
+ "evidence": "`charge_with_retry` catches `TimeoutError` and `continue`s; after the last attempt it falls through to `return ChargeResult.ok()`.",
+ "impact": "A charge that never completed is recorded as paid, so the order ships without payment.",
+ "fix": "Return a failure result when every attempt timed out.",
+ "runtime_validation": "reasoned",
+ "confidence": "credible",
+ "defect_kind": "swallowed-exception",
+ "identity": {
+ "stable_id": "fid_v1_2c7a0d6b2c80a885a1752c05f15d70c0",
+ "matching_eligible": true
+ }
+ }
+ ]
+}
+```
diff --git a/tests/unit/review/findings/structured_output_samples/manifest.json b/tests/unit/review/findings/structured_output_samples/manifest.json
new file mode 100644
index 0000000..0e1c5fb
--- /dev/null
+++ b/tests/unit/review/findings/structured_output_samples/manifest.json
@@ -0,0 +1,80 @@
+{
+ "identity_inputs": {
+ "retry-timeout": {
+ "repository": "acme/payments",
+ "location": "src/payments/retry.py:88",
+ "symbol": "charge_with_retry",
+ "behavioral_claim_text": "the retry loop swallows TimeoutError, so a charge that never completed is reported as successful",
+ "anchor_fragment": "except TimeoutError: continue",
+ "defect_kind_text": "swallowed-exception"
+ },
+ "retry-untested": {
+ "repository": "acme/payments",
+ "location": "tests/unit/payments/test_retry.py",
+ "behavioral_claim_text": "no test covers the timeout path, so the retry regression is not caught",
+ "anchor_fragment": "def test_charge_retries_on_failure",
+ "defect_kind_text": "missing-test-coverage"
+ },
+ "idempotency-dropped": {
+ "repository": "acme/payments",
+ "location": "src/payments/client.py:41",
+ "symbol": "PaymentsClient.post",
+ "behavioral_claim_text": "the client drops the idempotency key header, so a retried request is processed twice",
+ "anchor_fragment": "headers = {'Content-Type': 'application/json'}",
+ "defect_kind_text": "missing-idempotency-key"
+ },
+ "log-level": {
+ "repository": "acme/payments",
+ "location": "src/payments/retry.py:95",
+ "symbol": "charge_with_retry",
+ "behavioral_claim_text": "each retry logs at error level, so a recovered transient failure pages on-call",
+ "anchor_fragment": "log.error('charge attempt failed')",
+ "defect_kind_text": "log-level-misuse"
+ }
+ },
+ "samples": [
+ {
+ "file": "local-code-review/blocking-committed.md",
+ "skill": "local-code-review",
+ "known_head": "b81d4a9e0c7f3625d1e8a4b6c90f2e7d5a31c84f",
+ "target_committed": true,
+ "parity_group": "blocking",
+ "findings": {"F1": "retry-timeout", "F2": "idempotency-dropped", "F3": "retry-untested"}
+ },
+ {
+ "file": "github-pr-review/blocking.md",
+ "skill": "github-pr-review",
+ "known_head": "b81d4a9e0c7f3625d1e8a4b6c90f2e7d5a31c84f",
+ "parity_group": "blocking",
+ "findings": {"F1": "retry-timeout", "F2": "idempotency-dropped", "F3": "retry-untested"}
+ },
+ {
+ "file": "local-code-review/clean-p2-uncommitted.md",
+ "skill": "local-code-review",
+ "known_head": "5e0f7c2d9a1b4e3f6c8d0a2b4c6e8f0a1b3c5d7e",
+ "target_committed": false,
+ "parity_group": "clean-p2",
+ "findings": {"F1": "log-level"}
+ },
+ {
+ "file": "github-pr-review/clean-p2-delta.md",
+ "skill": "github-pr-review",
+ "known_head": "5e0f7c2d9a1b4e3f6c8d0a2b4c6e8f0a1b3c5d7e",
+ "parity_group": "clean-p2",
+ "findings": {"F1": "log-level"}
+ },
+ {
+ "file": "local-code-review/incomplete.md",
+ "skill": "local-code-review",
+ "known_head": "c4a9e1f07b3d5a2c8e6f0b1d3a5c7e9f2b4d6a80",
+ "target_committed": true,
+ "findings": {"F1": "retry-timeout"}
+ },
+ {
+ "file": "github-pr-review/incomplete-head-unknown.md",
+ "skill": "github-pr-review",
+ "known_head": null,
+ "findings": {}
+ }
+ ]
+}
diff --git a/tests/unit/review/findings/test_structured_output_contract.py b/tests/unit/review/findings/test_structured_output_contract.py
new file mode 100644
index 0000000..c6d0527
--- /dev/null
+++ b/tests/unit/review/findings/test_structured_output_contract.py
@@ -0,0 +1,413 @@
+#!/usr/bin/env python3
+"""Cross-Skill contract tests for the structured review result (Issue #71).
+
+Contract: docs/review-result/review-result-model.md section 8. Sample outputs
+from both Skills must validate against the versioned schema, fail closed on
+unversioned/unsupported/invalid results, keep shared fields identical across
+Skills, and agree with the human report produced by the same review.
+
+Run with:
+ python3 -m unittest tests.unit.review.findings.test_structured_output_contract
+"""
+
+from __future__ import annotations
+
+import copy
+import json
+import re
+import unittest
+from pathlib import Path
+
+from tests.reference.review import finding_identity as fi
+from tests.reference.review import review_result as rr
+from tests.reference.review import structured_output_contract as soc
+from tests.support.paths import REPO_ROOT
+
+SAMPLES_DIR = Path(__file__).resolve().parent / "structured_output_samples"
+MANIFEST = json.loads((SAMPLES_DIR / "manifest.json").read_text(encoding="utf-8"))
+SAMPLES = {entry["file"]: entry for entry in MANIFEST["samples"]}
+MODEL = REPO_ROOT / "docs" / "review-result" / "review-result-model.md"
+LOCAL_POLICY = REPO_ROOT / "shared" / "policies" / "structured-output.md"
+GITHUB_POLICY = REPO_ROOT / "skills" / "github-pr-review" / "policies" / "structured-output.md"
+
+
+def _text(name: str) -> str:
+ return (SAMPLES_DIR / name).read_text(encoding="utf-8")
+
+
+def _split(name: str) -> soc.SplitOutput:
+ return soc.split_output(_text(name), SAMPLES[name]["skill"])
+
+
+def _with_result(text: str, result: object) -> str:
+ body = json.dumps(result, indent=2)
+ return re.sub(r"```json\n.*?\n```", lambda _m: f"```json\n{body}\n```", text, flags=re.S)
+
+
+def _errors(name: str, text: str | None = None) -> tuple[str, ...]:
+ entry = SAMPLES[name]
+ return soc.contract_errors(
+ _text(name) if text is None else text,
+ entry["skill"],
+ entry["known_head"],
+ entry.get("target_committed", True),
+ )
+
+
+def _by_skill(skill: str) -> list[str]:
+ return [name for name, entry in SAMPLES.items() if entry["skill"] == skill]
+
+
+class SampleCorpusTests(unittest.TestCase):
+ def test_manifest_lists_every_sample_on_disk(self) -> None:
+ on_disk = {p.relative_to(SAMPLES_DIR).as_posix() for p in SAMPLES_DIR.rglob("*.md")}
+ self.assertEqual(on_disk, set(SAMPLES))
+
+ def test_each_skill_covers_every_outcome(self) -> None:
+ for skill in soc.SKILLS:
+ outcomes = {_split(name).result["decision"]["outcome"] for name in _by_skill(skill)}
+ with self.subTest(skill=skill):
+ self.assertEqual(outcomes, {"clean", "blocking", "incomplete"})
+
+ def test_sample_file_skill_matches_its_directory(self) -> None:
+ for name, entry in SAMPLES.items():
+ with self.subTest(sample=name):
+ self.assertEqual(name.split("/", 1)[0], entry["skill"])
+
+
+class SchemaConformanceTests(unittest.TestCase):
+ def test_every_sample_honors_the_contract(self) -> None:
+ for name in SAMPLES:
+ with self.subTest(sample=name):
+ self.assertEqual(_errors(name), ())
+
+ def test_every_sample_carries_the_published_version(self) -> None:
+ for name in SAMPLES:
+ with self.subTest(sample=name):
+ self.assertEqual(_split(name).result["schema_version"], rr.SCHEMA_VERSION)
+
+ def test_identities_are_the_canonical_minted_values(self) -> None:
+ for name, entry in SAMPLES.items():
+ findings = {f["id"]: f["identity"] for f in _split(name).result["findings"]}
+ self.assertEqual(set(findings), set(entry["findings"]), name)
+ for finding_id, key in entry["findings"].items():
+ descriptor = fi.build_descriptor(**MANIFEST["identity_inputs"][key])
+ with self.subTest(sample=name, finding=finding_id):
+ self.assertEqual(findings[finding_id]["stable_id"], fi.mint_identity(descriptor))
+ self.assertEqual(findings[finding_id]["matching_eligible"], fi.is_matchable(descriptor))
+
+ def test_fabricated_identity_is_invisible_to_output_checks(self) -> None:
+ # Boundary: the descriptor is not in the result, so only live runs can
+ # show an unminted stable_id (#529, not these checks).
+ name = "local-code-review/blocking-committed.md"
+ result = copy.deepcopy(_split(name).result)
+ for finding in result["findings"]:
+ finding["identity"]["stable_id"] = "fid_v1_" + "0" * 32
+ self.assertEqual(_errors(name, _with_result(_text(name), result)), ())
+
+
+class FailClosedTests(unittest.TestCase):
+ """schema-versioning.md section 3: nothing is consumed from an unsupported result."""
+
+ NAME = "github-pr-review/blocking.md"
+
+ def _mutated(self, mutate) -> tuple[str, ...]:
+ result = copy.deepcopy(_split(self.NAME).result)
+ mutate(result)
+ return _errors(self.NAME, _with_result(_text(self.NAME), result))
+
+ def assertFails(self, mutate, fragment: str) -> None:
+ errors = self._mutated(mutate)
+ self.assertTrue(any(fragment in e for e in errors), f"{fragment!r} not in {errors}")
+
+ def test_unversioned_result(self) -> None:
+ self.assertFails(lambda r: r.pop("schema_version"), "missing or not MAJOR.MINOR.PATCH")
+
+ def test_malformed_versions(self) -> None:
+ for bad in ("1.0", "v1.0.0", "1.0.0-rc1", 1, None):
+ with self.subTest(version=bad):
+ self.assertFails(lambda r, v=bad: r.update(schema_version=v), "missing or not MAJOR.MINOR.PATCH")
+
+ def test_unsupported_major(self) -> None:
+ for bad in ("2.0.0", "0.9.0"):
+ with self.subTest(version=bad):
+ self.assertFails(lambda r, v=bad: r.update(schema_version=v), "unsupported major")
+
+ def test_newer_minor_than_the_published_schema(self) -> None:
+ self.assertFails(lambda r: r.update(schema_version="1.1.0"), "newer than the published schema")
+
+ def test_unpublished_patch_is_schema_invalid(self) -> None:
+ self.assertFails(lambda r: r.update(schema_version="1.0.1"), "$.schema_version")
+
+ def test_schema_invalid_result(self) -> None:
+ self.assertFails(lambda r: r.update(verdict="ok"), "unexpected property 'verdict'")
+ self.assertFails(lambda r: r["findings"][0].pop("identity"), "missing required property 'identity'")
+
+ def test_owner_inconsistent_result(self) -> None:
+ self.assertFails(lambda r: r["counts"].update(p2=0), "$.counts")
+
+ def test_result_that_is_not_an_object(self) -> None:
+ errors = _errors(self.NAME, _with_result(_text(self.NAME), [1, 2]))
+ self.assertEqual(errors, ("$: structured result is not a JSON object",))
+
+ def test_unparseable_json(self) -> None:
+ text = _text(self.NAME).replace('"schema_version"', "schema_version", 1)
+ self.assertIn("not valid JSON", _errors(self.NAME, text)[0])
+
+ def test_missing_block_without_a_not_emitted_statement(self) -> None:
+ text = re.sub(r"```json\n.*?\n```\n", "", _text(self.NAME), flags=re.S)
+ self.assertIn("no structured result block", _errors(self.NAME, text)[0])
+
+ def test_second_json_block(self) -> None:
+ text = _text(self.NAME)
+ block = re.search(r"```json\n.*?\n```\n", text, re.S).group(0)
+ self.assertIn("exactly one json block", _errors(self.NAME, text + "\n" + block)[0])
+
+ def test_local_content_after_the_block(self) -> None:
+ name = "local-code-review/blocking-committed.md"
+ self.assertIn("must be the last part", _errors(name, _text(name) + "\nTrailing prose.\n")[0])
+
+ def test_github_brief_may_follow_the_block(self) -> None:
+ text = _text(self.NAME)
+ brief_start = text.index("## Reviewer Brief")
+ block_start = text.index("```json")
+ reordered = text[:brief_start] + text[block_start:] + "\n" + text[brief_start:block_start]
+ self.assertEqual(_errors(self.NAME, reordered), ())
+
+ def test_github_block_before_the_decision(self) -> None:
+ text = _text(self.NAME)
+ block = re.search(r"```json\n.*?\n```\n", text, re.S).group(0)
+ moved = text.replace(block, "").replace("### Decision", block + "\n### Decision", 1)
+ self.assertIn("must follow the report's Decision", _errors(self.NAME, moved)[0])
+
+ def test_local_result_needs_its_heading(self) -> None:
+ name = "local-code-review/blocking-committed.md"
+ text = _text(name).replace(soc.STRUCTURED_HEADING, "### Machine Output")
+ self.assertIn(soc.STRUCTURED_HEADING, _errors(name, text)[0])
+
+ def test_not_emitted_statement_is_recognized_but_not_a_result(self) -> None:
+ name = "local-code-review/blocking-committed.md"
+ text = _text(name).split(soc.STRUCTURED_HEADING, 1)[0]
+ text += f"{soc.NOT_EMITTED_PREFIX} verdict-consistency check withheld the report\n"
+ split = soc.split_output(text, soc.LOCAL)
+ self.assertIsNone(split.result)
+ self.assertEqual(split.not_emitted_reason, "verdict-consistency check withheld the report")
+ self.assertEqual(_errors(name, text), ("structured result was not emitted",))
+
+
+class SurfacePopulationTests(unittest.TestCase):
+ def _state_errors(self, name: str, **state) -> tuple[str, ...]:
+ entry = SAMPLES[name]
+ result = copy.deepcopy(_split(name).result)
+ result["reviewed_state"].update(state)
+ return soc.surface_errors(result, entry["skill"], entry["known_head"], entry.get("target_committed", True))
+
+ def test_local_uncommitted_target_never_claims_a_head(self) -> None:
+ name = "local-code-review/clean-p2-uncommitted.md"
+ self.assertTrue(self._state_errors(name, reviewed_head_sha=SAMPLES[name]["known_head"]))
+
+ def test_local_committed_head_is_the_workspace_head(self) -> None:
+ name = "local-code-review/blocking-committed.md"
+ self.assertTrue(self._state_errors(name, reviewed_head_sha="a" * 40))
+ self.assertTrue(self._state_errors(name, reviewed_head_sha=None))
+
+ def test_local_review_is_full_and_stateless(self) -> None:
+ name = "local-code-review/blocking-committed.md"
+ self.assertTrue(self._state_errors(name, completeness="delta-re-review"))
+ self.assertTrue(self._state_errors(name, prior_reviewed_sha="a" * 40))
+
+ def test_github_head_is_the_pr_head(self) -> None:
+ self.assertTrue(self._state_errors("github-pr-review/blocking.md", reviewed_head_sha="a" * 40))
+
+ def test_github_null_head_only_when_incomplete(self) -> None:
+ self.assertTrue(self._state_errors("github-pr-review/blocking.md", reviewed_head_sha=None))
+ self.assertEqual(self._state_errors("github-pr-review/incomplete-head-unknown.md"), ())
+
+ def test_skill_field_names_the_producer(self) -> None:
+ result = copy.deepcopy(_split("github-pr-review/blocking.md").result)
+ result["skill"] = soc.LOCAL
+ self.assertTrue(soc.surface_errors(result, soc.GITHUB, result["reviewed_state"]["reviewed_head_sha"]))
+
+
+class CrossSkillParityTests(unittest.TestCase):
+ def _groups(self) -> dict[str, dict[str, str]]:
+ groups: dict[str, dict[str, str]] = {}
+ for name, entry in SAMPLES.items():
+ if "parity_group" in entry:
+ groups.setdefault(entry["parity_group"], {})[entry["skill"]] = name
+ return groups
+
+ def test_every_group_pairs_both_skills(self) -> None:
+ groups = self._groups()
+ self.assertTrue(groups)
+ for group, members in groups.items():
+ with self.subTest(group=group):
+ self.assertEqual(set(members), set(soc.SKILLS))
+
+ def test_shared_fields_are_identical_for_the_same_review(self) -> None:
+ for group, members in self._groups().items():
+ local, github = (_split(members[s]).result for s in soc.SKILLS)
+ with self.subTest(group=group):
+ self.assertEqual(soc.shared_projection(local), soc.shared_projection(github))
+
+ def test_only_declared_surface_fields_differ(self) -> None:
+ for group, members in self._groups().items():
+ local, github = (_split(members[s]).result for s in soc.SKILLS)
+ differing = {key for key in local if local[key] != github[key]}
+ with self.subTest(group=group):
+ self.assertLessEqual(differing, set(soc.SURFACE_SPECIFIC_FIELDS))
+ self.assertEqual(set(local["reviewed_state"]), set(github["reviewed_state"]))
+
+ def test_surface_fields_are_schema_fields(self) -> None:
+ self.assertLessEqual(set(soc.SURFACE_SPECIFIC_FIELDS), set(rr.load_schema()["required"]))
+
+ def test_projection_detects_a_shared_field_drift(self) -> None:
+ local = _split("local-code-review/blocking-committed.md").result
+ github = copy.deepcopy(_split("github-pr-review/blocking.md").result)
+ github["findings"][0]["confidence"] = "confirmed"
+ self.assertNotEqual(soc.shared_projection(local), soc.shared_projection(github))
+
+ def test_decision_renderings_match_the_model_and_both_policies(self) -> None:
+ model = MODEL.read_text(encoding="utf-8").split("## 4. Decision codes", 1)[1].split("## 5.", 1)[0]
+ rows = re.findall(r"^\| `(\w+)`(?: \(outcome only\))? \| `([A-Z ]+)` \| `([A-Za-z ]+)` \|", model, re.M)
+ self.assertEqual({code for code, _l, _g in rows}, {"clean", "blocking", "incomplete"})
+ local_policy = " ".join(LOCAL_POLICY.read_text(encoding="utf-8").split())
+ github_policy = GITHUB_POLICY.read_text(encoding="utf-8")
+ for code, local_label, github_label in rows:
+ with self.subTest(code=code):
+ self.assertEqual(soc.RENDERED_OUTCOME[soc.LOCAL][local_label.upper()], code)
+ self.assertEqual(soc.RENDERED_OUTCOME[soc.GITHUB][github_label.upper()], code)
+ self.assertIn(f"`{code}` → `{local_label}`", local_policy)
+ self.assertRegex(github_policy, rf"\| `{code}`(?: \(outcome only\))? \| `{github_label}`")
+
+ def test_both_policies_emit_the_published_version(self) -> None:
+ for policy in (LOCAL_POLICY, GITHUB_POLICY):
+ with self.subTest(policy=policy.parent.parent.name):
+ self.assertIn(f'"schema_version": "{rr.SCHEMA_VERSION}"', policy.read_text(encoding="utf-8"))
+
+
+class ReportAgreementTests(unittest.TestCase):
+ """The comparator reads rendered facts; it never re-derives the decision."""
+
+ def _agreement(self, name: str, human: str | None = None, result: dict | None = None) -> tuple[str, ...]:
+ split = _split(name)
+ skill = SAMPLES[name]["skill"]
+ report = soc.parse_human_report(split.human if human is None else human, skill)
+ return soc.agreement_errors(report, split.result if result is None else result, skill)
+
+ def assertDisagrees(self, errors: tuple[str, ...], fragment: str) -> None:
+ self.assertTrue(any(e.startswith(fragment) for e in errors), f"{fragment!r} not in {errors}")
+
+ def test_every_sample_agrees(self) -> None:
+ for name in SAMPLES:
+ with self.subTest(sample=name):
+ self.assertEqual(self._agreement(name), ())
+
+ def test_flipped_rendered_decision(self) -> None:
+ flips = {
+ "local-code-review/blocking-committed.md": ("**CHANGES REQUIRED**", "**REVIEW CLEAN**"),
+ "github-pr-review/blocking.md": ("**REQUEST CHANGES**", "**APPROVE**"),
+ "local-code-review/incomplete.md": ("**REVIEW INCOMPLETE**", "**CHANGES REQUIRED**"),
+ "github-pr-review/clean-p2-delta.md": ("**APPROVE**", "**REVIEW INCOMPLETE**"),
+ }
+ for name, (old, new) in flips.items():
+ with self.subTest(sample=name):
+ self.assertDisagrees(self._agreement(name, human=_split(name).human.replace(old, new)), "decision")
+
+ def test_other_skill_label_is_not_recognized_locally(self) -> None:
+ name = "local-code-review/blocking-committed.md"
+ human = _split(name).human.replace("**CHANGES REQUIRED**", "**REQUEST CHANGES**")
+ self.assertDisagrees(self._agreement(name, human=human), "human report: unrecognized")
+
+ def test_github_self_review_label_maps_to_the_same_outcome(self) -> None:
+ name = "github-pr-review/blocking.md"
+ human = _split(name).human.replace(
+ "**REQUEST CHANGES**", "**CHANGES REQUIRED** — GitHub review mutation withheld: reviewer is the PR author"
+ )
+ self.assertEqual(self._agreement(name, human=human), ())
+
+ def test_counts_disagree(self) -> None:
+ for name, old, new in (
+ ("local-code-review/blocking-committed.md", "- P0: 0, P1: 2, P2: 1", "- P0: 0, P1: 1, P2: 1"),
+ ("github-pr-review/blocking.md", "- P1: 2", "- P1: 1"),
+ ):
+ with self.subTest(sample=name):
+ self.assertDisagrees(self._agreement(name, human=_split(name).human.replace(old, new)), "counts")
+
+ def test_finding_severity_or_title_disagrees(self) -> None:
+ for name in ("local-code-review/blocking-committed.md", "github-pr-review/blocking.md"):
+ for field, value in (("severity", "P0"), ("title", "Something else")):
+ result = copy.deepcopy(_split(name).result)
+ result["findings"][0][field] = value
+ with self.subTest(sample=name, field=field):
+ self.assertDisagrees(self._agreement(name, result=result), "findings")
+
+ def test_finding_missing_from_the_result(self) -> None:
+ for name in ("local-code-review/clean-p2-uncommitted.md", "github-pr-review/clean-p2-delta.md"):
+ result = copy.deepcopy(_split(name).result)
+ result["findings"] = []
+ with self.subTest(sample=name):
+ self.assertDisagrees(self._agreement(name, result=result), "findings")
+
+ def test_finding_id_disagrees(self) -> None:
+ for name in ("local-code-review/blocking-committed.md", "github-pr-review/blocking.md"):
+ result = copy.deepcopy(_split(name).result)
+ result["findings"][1]["id"] = "F9"
+ with self.subTest(sample=name):
+ self.assertDisagrees(self._agreement(name, result=result), "finding id")
+
+ def test_affected_locations_disagree(self) -> None:
+ for name in ("local-code-review/blocking-committed.md", "github-pr-review/blocking.md"):
+ result = copy.deepcopy(_split(name).result)
+ result["findings"][1]["affected_locations"].pop()
+ result["findings"][1]["affected_locations"].append({"location": "src/x.py:1", "note": "n"})
+ with self.subTest(sample=name):
+ self.assertDisagrees(self._agreement(name, result=result), "affected locations")
+
+ def test_consolidated_finding_rendering_is_parsed(self) -> None:
+ for name in ("local-code-review/blocking-committed.md", "github-pr-review/blocking.md"):
+ report = soc.parse_human_report(_split(name).human, SAMPLES[name]["skill"])
+ consolidated = next(f for f in report.findings if f.id == "F2")
+ with self.subTest(sample=name):
+ self.assertEqual(len(consolidated.affected_locations), 2)
+
+ def test_local_order_disagrees(self) -> None:
+ name = "local-code-review/blocking-committed.md"
+ result = copy.deepcopy(_split(name).result)
+ result["findings"].reverse()
+ self.assertDisagrees(self._agreement(name, result=result), "findings: result order")
+
+ def test_coverage_disagrees(self) -> None:
+ name = "github-pr-review/clean-p2-delta.md"
+ human = _split(name).human.replace("- coverage: `complete`", "- coverage: `incomplete — partition skipped`")
+ self.assertDisagrees(self._agreement(name, human=human), "coverage")
+
+ def test_reviewed_head_disagrees(self) -> None:
+ for name in ("local-code-review/blocking-committed.md", "github-pr-review/blocking.md"):
+ head = SAMPLES[name]["known_head"]
+ with self.subTest(sample=name):
+ self.assertDisagrees(self._agreement(name, human=_split(name).human.replace(head, "a" * 40)), "reviewed head")
+
+ def test_human_voice_heading_is_parsed(self) -> None:
+ name = "github-pr-review/blocking.md"
+ human = _split(name).human.replace("#### F2 [P1] ", "#### F2 P1 (Blocking): ")
+ self.assertEqual(self._agreement(name, human=human), ())
+
+ def test_comparator_does_not_derive_the_decision(self) -> None:
+ name = "github-pr-review/clean-p2-delta.md"
+ result = copy.deepcopy(_split(name).result)
+ result["findings"][0]["severity"] = "P1"
+ result["counts"] = {"p0": 0, "p1": 1, "p2": 0}
+ human = _split(name).human.replace("- **P2 — ", "- **P1 — ").replace("- P1: 0\n- P2: 1", "- P1: 1\n- P2: 0")
+ self.assertEqual(self._agreement(name, human=human, result=result), ())
+ self.assertTrue(any("$.decision.derived" in e for e in soc.producer_errors(result)))
+
+ def test_comparison_is_deterministic(self) -> None:
+ name = "github-pr-review/blocking.md"
+ result = copy.deepcopy(_split(name).result)
+ result["findings"][0]["title"] = "Other"
+ self.assertEqual(self._agreement(name, result=result), self._agreement(name, result=result))
+
+
+if __name__ == "__main__":
+ unittest.main()