From 677f3cfaa61f737c185b677f7ba0d8daaea40423 Mon Sep 17 00:00:00 2001 From: amirbena Date: Thu, 24 Sep 2026 17:16:40 +0300 Subject: [PATCH 1/2] Add cross-Skill structured review result contract tests (#71) Validate paired local-code-review and github-pr-review sample outputs against the versioned review-result schema (fail closed on missing, malformed, unsupported or newer versions), assert shared-field parity and per-surface reviewed_state population, and compare each structured result with its human report without re-deriving the decision. Correct review-result-model.md section 7 and record the checks in section 8. Pin that the benchmark prompt leaves the structured result off and that an appended result block does not change parsed findings. Co-Authored-By: Claude Opus 5.5 --- docs/review-result/README.md | 4 + docs/review-result/review-result-model.md | 21 +- tests/README.md | 2 +- tests/integration/packaging/_shared.py | 1 + .../findings/test_review_result_docs.py | 22 + .../review/structured_output_contract.py | 267 ++++++++++++ .../unit/benchmark/test_production_adapter.py | 25 ++ .../github-pr-review/blocking.md | 148 +++++++ .../github-pr-review/clean-p2-delta.md | 81 ++++ .../incomplete-head-unknown.md | 56 +++ .../local-code-review/blocking-committed.md | 163 +++++++ .../local-code-review/clean-p2-uncommitted.md | 95 +++++ .../local-code-review/incomplete.md | 95 +++++ .../structured_output_samples/manifest.json | 80 ++++ .../test_structured_output_contract.py | 399 ++++++++++++++++++ 15 files changed, 1457 insertions(+), 2 deletions(-) create mode 100644 tests/reference/review/structured_output_contract.py create mode 100644 tests/unit/review/findings/structured_output_samples/github-pr-review/blocking.md create mode 100644 tests/unit/review/findings/structured_output_samples/github-pr-review/clean-p2-delta.md create mode 100644 tests/unit/review/findings/structured_output_samples/github-pr-review/incomplete-head-unknown.md create mode 100644 tests/unit/review/findings/structured_output_samples/local-code-review/blocking-committed.md create mode 100644 tests/unit/review/findings/structured_output_samples/local-code-review/clean-p2-uncommitted.md create mode 100644 tests/unit/review/findings/structured_output_samples/local-code-review/incomplete.md create mode 100644 tests/unit/review/findings/structured_output_samples/manifest.json create mode 100644 tests/unit/review/findings/test_structured_output_contract.py 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..5d1bb63 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`, a different `MAJOR`, or a version newer than the published schema fails closed ([`schema-versioning.md`](schema-versioning.md) section 3). | +| 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..8a2ef1d --- /dev/null +++ b/tests/reference/review/structured_output_contract.py @@ -0,0 +1,267 @@ +#!/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 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)] + try: + result = json.loads(fence.group(1)) + except json.JSONDecodeError as exc: + raise ContractError(f"structured result is not valid JSON: {exc}") from exc + return SplitOutput(human, result) + + +def producer_errors(result: Any) -> tuple[str, ...]: + """Fail closed per schema-versioning.md section 3, then schema + owner consistency.""" + 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..6da8610 --- /dev/null +++ b/tests/unit/review/findings/test_structured_output_contract.py @@ -0,0 +1,399 @@ +#!/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_content_after_the_block(self) -> None: + self.assertIn("must be the last part", _errors(self.NAME, _text(self.NAME) + "\nTrailing prose.\n")[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() From b45b27eddbc2f597ee6b371e34d9a6ad438df439 Mon Sep 17 00:00:00 2001 From: amirbena Date: Thu, 24 Sep 2026 17:20:51 +0300 Subject: [PATCH 2/2] Address local review: GitHub result placement and versioning citation (#71) github-pr-review's policy only requires the structured result after the caller-facing report, so the splitter now accepts a Reviewer Brief after the JSON block and instead requires the block to follow the Decision. The newer-than-published version rejection is a producer rule (schema-versioning.md section 4), not section 3 consumer guidance; the model record and producer_errors now cite it correctly. Co-Authored-By: Claude Opus 5.5 --- docs/review-result/review-result-model.md | 2 +- .../review/structured_output_contract.py | 15 ++++++++++++--- .../test_structured_output_contract.py | 18 ++++++++++++++++-- 3 files changed, 29 insertions(+), 6 deletions(-) diff --git a/docs/review-result/review-result-model.md b/docs/review-result/review-result-model.md index 5d1bb63..c42d7ad 100644 --- a/docs/review-result/review-result-model.md +++ b/docs/review-result/review-result-model.md @@ -147,7 +147,7 @@ the checks are | Check | Rule | | --- | --- | -| Schema and version | Each result passes the section 5 validator. A missing or malformed `schema_version`, a different `MAJOR`, or a version newer than the published schema fails closed ([`schema-versioning.md`](schema-versioning.md) section 3). | +| 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. | diff --git a/tests/reference/review/structured_output_contract.py b/tests/reference/review/structured_output_contract.py index 8a2ef1d..09e3552 100644 --- a/tests/reference/review/structured_output_contract.py +++ b/tests/reference/review/structured_output_contract.py @@ -108,6 +108,11 @@ def split_output(text: str, skill: str) -> SplitOutput: 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()] @@ -116,15 +121,19 @@ def split_output(text: str, skill: str) -> SplitOutput: 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: - result = json.loads(fence.group(1)) + return json.loads(block) except json.JSONDecodeError as exc: raise ContractError(f"structured result is not valid JSON: {exc}") from exc - return SplitOutput(human, result) def producer_errors(result: Any) -> tuple[str, ...]: - """Fail closed per schema-versioning.md section 3, then schema + owner consistency.""" + """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) diff --git a/tests/unit/review/findings/test_structured_output_contract.py b/tests/unit/review/findings/test_structured_output_contract.py index 6da8610..c6d0527 100644 --- a/tests/unit/review/findings/test_structured_output_contract.py +++ b/tests/unit/review/findings/test_structured_output_contract.py @@ -163,8 +163,22 @@ def test_second_json_block(self) -> None: 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_content_after_the_block(self) -> None: - self.assertIn("must be the last part", _errors(self.NAME, _text(self.NAME) + "\nTrailing prose.\n")[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"